fix(font): resolve next/font/local paths inside node_modules packages - #1780
Conversation
The vinext:local-fonts transform excluded node_modules files, so npm packages that wrap next/font/local (e.g. geist) never had their relative font src paths promoted to Vite asset imports. The raw relative path (e.g. ./fonts/geist-mono/GeistMono-Variable.woff2) leaked into the runtime @font-face src and 404'd at request time. Remove the node_modules exclusion from the filter and defensive guard so package files are transformed like user source, matching Next.js's font loader. The code filter plus the required default-import check keep the transform from touching unrelated modules, and the font-local shim guard still skips vinext's own shim when installed as a dependency.
|
/bigbonk review for issues |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(font): resolve next/font/local paths inside node_modules packages
The fix is correct and well-targeted. Removing the node_modules exclusion brings vinext:local-fonts in line with vinext:google-fonts (which never excluded node_modules — see its filter at packages/vinext/src/plugins/fonts.ts:717-722), and matches Next.js's font loader, which runs on package files too. The code: "next/font/local" filter + the required default-import check (fonts.ts:1154-1156) keep the transform from touching unrelated modules. Tests pass and vp check is clean.
One correctness concern worth considering before merge:
The id.includes("font-local") shim guard is now a broader false-positive surface (fonts.ts:1149)
if (id.includes("font-local")) return null;Before this PR, this substring check only ran against user source (node_modules was excluded). Now it runs against every node_modules file that imports next/font/local. The intent is to skip vinext's own shim at node_modules/vinext/.../shims/font-local.js, but includes("font-local") is a loose substring match anywhere in the path. Any legitimate third-party package that both wraps next/font/local and has font-local somewhere in its resolved path would now be silently skipped, and its relative font paths would leak again — the exact bug this PR fixes. Realistic examples:
- a package named
font-local-loaderor@scope/font-local-something - a package that ships fonts under a directory like
dist/font-local/...(theidis the importing module path, but a wrapper module living in such a dir would match)
Compare with the sibling vinext:google-fonts plugin, which avoids this by taking shimsDir as a parameter and using a precise prefix check:
// createGoogleFontsPlugin(fontGoogleShimPath, shimsDir)
if (id.startsWith(shimsDir)) return null; // fonts.ts:728createLocalFontsPlugin() currently takes no arguments (fonts.ts:1117, wired at index.ts:3927), so it can't do the same precise check. Two options:
- Preferred — mirror google-fonts: pass
shimsDir(or the resolvedfont-localshim path) intocreateLocalFontsPlugin(shimsDir)and guard withid.startsWith(shimsDir)/ exact-path match. This eliminates the false-positive class entirely and keeps the two font plugins symmetric. - Minimal: tighten the substring to something that can only be the shim, e.g. matching a path segment like
/shims/font-local.rather than the barefont-localsubstring.
This isn't hypothetical-only: the whole point of the PR is that real packages wrap next/font/local, so the guard is now exercised against arbitrary package paths rather than just user source. Worth tightening in the same PR since it's the one introducing the exposure.
Non-blocking notes
- The dep-optimizer caveat called out in the PR description (transform is bypassed unless the package is bundled via
noExternal) is accurate. SSR/RSC environments setnoExternal: true(index.ts:1792,2020,2045), so the App Router server path works as verified. A font package imported only from a"use client"component on the client environment could still be pre-bundled by the dep optimizer without thisenforce: "pre"transform running — but that's a pre-existing limitation shared with google-fonts and not a regression here. Fine to track separately if it comes up. - Test coverage is good. Consider adding one negative case to lock in whichever guard fix you choose — e.g. a
node_modules/font-local-something/dist/index.jspackage that should be transformed (option 1) — so a future loosening of the guard is caught.
Overall: the core change is right and safe to ship; I'd just tighten the shim guard before merge given it now sees all node_modules paths.
|
Review posted to PR #1780. SummaryThe fix is correct and well-targeted. Removing the I flagged one correctness concern worth addressing before merge:
Non-blocking notes:
|
Now that node_modules is no longer excluded, the shim guard runs against
arbitrary third-party package paths. Replace the loose
`id.includes("font-local")` substring check with a precise
`id.startsWith(shimsDir)` prefix check, mirroring createGoogleFontsPlugin,
so legitimate packages whose path contains "font-local" (e.g.
font-local-loader) are still transformed instead of silently skipped.
Pass _shimsDir into createLocalFontsPlugin and add a regression test for a
third-party font-local-named package that must still be transformed.
|
Addressed the shim-guard concern in d0a0517. Replaced the loose
Added a regression test ( Left the dep-optimizer / client-only |
|
/bigbonk re-review for issues |
Re-review: shim-guard concern fully addressed ✅Re-reviewed after What was fixed correctly
Verification
Non-blocking (unchanged from prior review)
Core change is right, the guard is now tight and symmetric with the sibling plugin, and the regression coverage is solid. LGTM to ship. |
|
Re-review posted to PR #1780. SummaryThe shim-guard concern from the prior review was fully addressed in commit
No new issues found. The change is correct, tight, symmetric with the sibling plugin, and well-tested. LGTM to ship. |
1 participant
Problem
vinext's
localFontshim works (CSS classes +@font-faceare generated), but for npm packages that wrapnext/font/localthe font file URL was the raw relative path from the package module rather than a Vite-resolvable URL.For example,
geist/dist/mono.jscalls:The generated
@font-face { src: url(...) }referenced./fonts/geist-mono/GeistMono-Variable.woff2(relative to the package'sdist/dir), which 404s at request time.Root cause
createLocalFontsPlugin(packages/vinext/src/plugins/fonts.ts) excludednode_modulesfiles — both via the filter (filter.id.exclude: /node_modules/) and a defensiveif (id.includes("node_modules")) return null;guard. So the transform that promotes relativesrcpaths to Vite asset imports never ran on package files. This affects any npm package wrappingnext/font/local.Fix
Remove the
node_modulesexclusion from both the filter and the defensive guard so package files are transformed like user source — matching Next.js, whose font loader also runs on these package files. Thecode: "next/font/local"filter plus the required default-import check keep the transform from touching unrelated modules, and thefont-localshim guard still skips vinext's own shim when it lives undernode_modules/vinext/.../shims/font-local.*.Verification
nullfor anode_modules/geist/dist/mono.jsid.node_modulesfont package built through the full App Router pipeline: before the fix the relative path leaked; after the fix the font is emitted as a hashed asset (_next/static/MyMono-Variable-<hash>.woff2) and the raw relative path no longer appears in any bundle. (The App Router renders the package server-side vianoExternal, so the asset import resolves rather than being bypassed by the dep optimizer.)Tests
returns null for node_modules files) with a regression test asserting geist-stylenode_modulespackages are transformed.font-localshim is still skipped insidenode_modules.vp checkis clean.