fix: resolve #standard-fonts/* to one file under every condition - #1782
Open
mendrinos wants to merge 1 commit into
Open
fix: resolve #standard-fonts/* to one file under every condition#1782mendrinos wants to merge 1 commit into
#standard-fonts/* to one file under every condition#1782mendrinos wants to merge 1 commit into
Conversation
The node ESM build loads the standard font metrics lazily through `createRequire`, so the require condition is the only one ever taken at runtime. Bundlers and file tracers walk `js/pdfkit.node.mjs` as ESM and resolve the same specifier under the import condition, landing on the `.mjs` twin instead. Anything that packages a traced dependency set ships the modules pdfkit never loads and omits the ones it does, and the first `new PDFDocument()` throws `Cannot find module '#standard-fonts/Helvetica'`. Nothing reachable at runtime uses the import condition here: the browser builds register their fonts through `registerStdFonts` and never reference `#standard-fonts`, and both node builds arrive via `require`. Point the internal mapping at the CommonJS files so every resolver agrees with Node. The public `./standard-fonts/*` export is untouched, so consumers importing `pdfkit/standard-fonts/Helvetica` still get the ESM build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
js/pdfkit.node.mjsloads the standard font metrics lazily throughcreateRequire:package.json#importsmaps that specifier two ways:The call is a
require, so at runtime Node and Bun both loadjs/standard-fonts/Helvetica.cjs—tests/package-resolution.mjsalready assertsexactly that. But a bundler or file tracer walks the same file as ESM and resolves
the same specifier under
default, landing on the.mjstwin. The two disagree,and for anything that packages a traced dependency set, the tracer wins.
With
@vercel/nft— the tracer behind Vercel,Nitro/Nuxt and
@vercel/ncc— tracing an ESM entry that imports pdfkit collects all14
js/standard-fonts/*.mjsand none of the.cjsfiles:The build succeeds and the package looks complete in the output. Then the first
document throws:
Reproducible by copying that traced file list into a directory with no ancestor
node_modulesand constructing aPDFDocument— fails under both Node 23 and Bun 1.4.Fix
Nothing reachable at runtime takes the import condition here. The browser builds
register fonts through
registerStdFontsand never reference#standard-fonts(
grep -c standard-fonts js/pdfkit.browser.mjs→ 0); the only consumer islib/document.node.js, reached byrequirefrom both node builds. Thedefaultbranch is therefore unreachable and serves only to point resolvers at a file pdfkit
never loads.
Runtime behaviour is unchanged — same file, still lazy, still one font at a time.
Tracers now resolve what Node resolves, and traced output gets slightly smaller
because the unused ESM twins are no longer dragged along.
The public
./standard-fonts/*export is untouched: consumers whoimport 'pdfkit/standard-fonts/Helvetica'still get the ESM build, and rollup still emitsboth variants.
Alternative
If the conditional mapping should stay, the other way out is to have the build emit
explicit relative specifiers (
require('./standard-fonts/Helvetica.cjs')) in the nodeESM bundle — unambiguous to every analyser, no condition resolution involved. Happy
to redo it that way if you prefer; it is a rollup-config change rather than a manifest
one.
Test
tests/package-resolution.mjsgains an assertion that both conditions land on thesame file. It fails on
masterwith the exact shape of the bug:yarn test:package,yarn test:unit(432 passing),yarn lintandyarn prettierare all green with the change.
Context
Hit downstream on a Nuxt/Nitro app: receipt PDFs quietly stopped being attached to
payment emails (the generation error was caught and logged) and every other PDF route
500'd, while CI stayed green — nothing in a normal test run exercises the packaged
output. Same class as #1779, one level further down.