feat(font): replace generated font catalog with Vite import-rewriting transform#262
feat(font): replace generated font catalog with Vite import-rewriting transform#262yunus25jmi1 wants to merge 1 commit intocloudflare:mainfrom
Conversation
commit: |
|
@southpolesteve @threepointone @elithrar Kindly review the changes. The workflow is completely green. All checks are passed with 12 successful checks & 3 skipped. |
|
/bigbonk review |
There was a problem hiding this comment.
Review: feat(font): replace generated font catalog with Vite import-rewriting transform
The core idea is great — replacing a 1928-line generated catalog with a compile-time import rewrite is a clear win for maintenance and bundle size. The import-rewriting logic is well-structured and handles edge cases like type imports, as aliases, and utility re-exports.
However, there are several issues that need to be addressed before merging.
Critical Issues
-
Build-mode self-hosting runs regex on the original code, but MagicString has already rewritten the import. The
fontCallReregex runs on the originalcodestring (line 2978), buts.overwrite()has already replaced the import region. The indices from the original regex may now overlap with or conflict with the overwritten region. If a font constructor call likeInter({ weight: '400' })appears on the same line as the import, MagicString will throw or silently produce corrupted output. The self-hosting section should either regex-match on the already-transformed code or ensure the regions don't overlap. -
Only the first
import { ... } from 'next/font/google'statement is handled. The regex uses.exec()without thegflag, so a file with two separate import statements fromnext/font/google(which is valid JS) will silently ignore the second one. This is an existing limitation that the old code also had, but given that this PR is rewriting the import handling, it's worth fixing now. -
The
vinext:nitro-compatplugin is unrelated to the font transform. This is a completely separate feature/fix that should be in its own PR. It also containsconsole.logdebug statements that should not ship to production.
Minor Issues
-
Font family derivation inconsistency between import rewrite and build-mode self-hosting. The import rewrite uses
f.imported.replace(/_/g, " ")(underscore-to-space only), but the build-mode self-hosting usesfontSpec.imported.replace(/_/g, " ").replace(/([a-z])([A-Z])/g, "$1 $2")(underscore-to-space + PascalCase-to-space). Since Google Fonts uses underscores in their Next.js export names (not PascalCase), the PascalCase transform in the build path is dead code that could introduce bugs if someone used PascalCase names. These should be consistent. -
Missing test for
import { Inter as MyInter } from 'next/font/google'— the alias handling code is present but untested.
|
Review submitted. Here's a summary of the findings: Critical Issues (3)
Minor Issues (3)
|
… transform Closes cloudflare#200 Instead of a 1928-line generated font catalog (font-google.generated.ts) that re-exports every Google Font as a createFontLoader call, the vinext:google-fonts Vite plugin now rewrites next/font/google import statements at compile time. Address review feedback: - Fix MagicString index corruption: add overlap guard for self-hosting regex so font constructor calls that share a region with imported statements are safely skipped - Handle multiple import statements: use matchAll with gm flags to process all import { ... } from 'next/font/google' in a single file - Remove unrelated vinext:nitro-compat plugin (belongs in separate PR) - Fix font family derivation consistency: remove dead PascalCase-to-space transform in build-mode path to match dev-mode (underscore-to-space only) - Fix font-google guard: use shimsDir path check instead of id.includes() to avoid false positives on user files like font-google-helpers.ts - Fix fontCallRe regex: allow digits and lowercase after underscores to match fonts like Baloo_2 and M_PLUS_1p - Add utilityExports sync comment referencing font-google.ts barrel - Add tests: aliased imports, multiple import statements, digit-underscore font names
aae8f42 to
8c1d17f
Compare
|
@southpolesteve @james-elicx The fixes is pushed. Kindly review the changes. |
|
/bigbonk review |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review |
There was a problem hiding this comment.
Review: feat(font): replace generated font catalog with Vite import-rewriting transform
The core approach is sound — replacing a 1928-line generated catalog with a compile-time import rewrite is a clear maintenance and bundle-size win. The author has addressed the critical issues from the previous review (nitro-compat plugin removed, multiple import statements handled via gm flags, MagicString overlap detection added, alias test added, Baloo_2 test added). The implementation is well-structured with good separation between the import rewriting phase and the build-only self-hosting phase.
The remaining items are minor and don't block merge, but would be good to address.
| // indices, so overlapping overwrites would throw. | ||
| const callStart = callMatch.index; | ||
| const callEnd = callStart + fullCallMatch.length; | ||
| if (overwrittenRanges.some(([s, e]) => callStart < e && callEnd > s)) continue; |
There was a problem hiding this comment.
Nit: variable shadowing — The destructured [s, e] shadows the outer const s = new MagicString(code) on line 2947. This doesn't cause a functional bug (the arrow function only uses s for index comparison, not as a MagicString), but it's a readability hazard — a future reader might think s here refers to the MagicString.
| if (overwrittenRanges.some(([s, e]) => callStart < e && callEnd > s)) continue; | |
| if (overwrittenRanges.some(([start, end]) => callStart < end && callEnd > start)) continue; |
| // The ^ anchor with 'm' flag ensures we only match real import statements | ||
| // at the start of a line, not occurrences inside comments or strings. | ||
| // Uses the 'gm' flags to handle multiple import statements in one file. | ||
| const importRe = /^[ \t]*import\s*\{([^}]+)\}\s*from\s*['"]next\/font\/google['"]\s*;?/gm; |
There was a problem hiding this comment.
Minor: ^ anchor with m flag can match inside template literals. A multiline template string like:
const x = `
import { Inter } from 'next/font/google'
`;would be matched by this regex because the m flag makes ^ match at the start of any line, even inside template literals. In practice this is extremely unlikely to matter (who puts a next/font/google import inside a template string?), but worth noting if you want maximum robustness. The typical fix is to use an AST-based approach, but for a Vite transform this is a reasonable tradeoff.
| // indices, so overlapping overwrites would throw. | ||
| const callStart = callMatch.index; | ||
| const callEnd = callStart + fullCallMatch.length; | ||
| if (overwrittenRanges.some(([s, e]) => callStart < e && callEnd > s)) continue; |
There was a problem hiding this comment.
Design note: self-hosting silently skipped for same-line import+call. When the import and constructor call are on the same line:
import { Inter } from 'next/font/google'; const inter = Inter({ weight: ['400'] });The import rewriting works correctly (Inter gets replaced with createFontLoader call), but the self-hosting CSS injection is skipped because the call overlaps with the overwritten import region. The font will still work in production but will load from CDN instead of being self-hosted.
This is a fine tradeoff since the single-line pattern is rare, but a comment documenting this intentional limitation would help future readers understand why the overlap check exists and what the user-facing consequence is.
| // Utility exports that are NOT font names — keep as regular imports. | ||
| // IMPORTANT: keep this set in sync with the non-default exports from | ||
| // packages/vinext/src/shims/font-google.ts (and its re-export barrel). | ||
| const utilityExports = new Set([ |
There was a problem hiding this comment.
Good call on the IMPORTANT: keep this set in sync comment. For even more robustness, consider deriving this set programmatically from the actual exports of font-google.ts (e.g., in a test) rather than relying on a manual sync. A test like:
it('utilityExports matches actual non-font exports', async () => {
const mod = await import('../packages/vinext/src/shims/font-google.js');
const actual = Object.keys(mod).filter(k => k !== 'default');
// Verify they match the hardcoded set in the plugin
});would catch drift automatically.
| const fontCallRe = /\b([A-Z][A-Za-z]*(?:_[A-Z][A-Za-z]*)*)\s*\(\s*(\{[^}]*\})\s*\)/g; | ||
| // Skip our own shim files — they contain 'next/font/google' in comments | ||
| // and re-export statements that should not be rewritten. | ||
| if (id.startsWith(path.resolve(__dirname, "shims") + path.sep)) return null; |
There was a problem hiding this comment.
Using path.resolve(__dirname, 'shims') is more robust than the previous id.includes('font-google') guard that was flagged by the earlier review. Good fix.
| // Convert export-style names to proper font family names: | ||
| // - Underscores to spaces: "Roboto_Mono" -> "Roboto Mono" | ||
| // - PascalCase to spaces: "RobotoMono" -> "Roboto Mono" | ||
| const family = prop.replace(/_/g, " ").replace(/([a-z])([A-Z])/g, "$1 $2"); |
There was a problem hiding this comment.
Good — the Proxy now handles both underscore-style names (Roboto_Mono) and PascalCase names (RobotoMono). This keeps the Proxy as a correct fallback for cases where the Vite plugin doesn't run (e.g., direct runtime access via mod.default.SomeFontName).
| plugin._fontCache.clear(); | ||
| }); | ||
|
|
||
| it("rewrites aliased font imports (import { Inter as MyFont })", async () => { |
There was a problem hiding this comment.
Good — this alias test was called out as missing in the previous review and has been added. Coverage for the import { X as Y } pattern is important since the rewriting code has separate imported vs local handling.
| expect(result.code).toContain("merged into first"); | ||
| }); | ||
|
|
||
| it("handles font names with digits after underscore (e.g. Baloo_2)", async () => { |
There was a problem hiding this comment.
Good — test for fonts with digits after underscore (Baloo_2) was also called out as missing and has been added. The updated fontCallRe regex on line 2998 of index.ts now matches [A-Za-z0-9] after underscore, which handles this correctly.
|
Review submitted successfully. Here's a summary of my findings: Review Summary for PR #262Overall assessment: The PR is in good shape. The core approach of replacing a 1928-line generated font catalog with a compile-time Vite import rewrite is a clear win. The author addressed all critical issues from the previous review. Key findings:
|
Summary
Closes #200
Instead of a 1928-line generated font catalog (
font-google.generated.ts) that re-exports every Google Font as acreateFontLoadercall, thevinext:google-fontsVite plugin now rewritesnext/font/googleimport statements at compile time.Before
After
The plugin rewrites imports at compile time across all Vite environments (RSC, SSR, client):
Self-hosted font fetching during production builds continues to work as before.
Changes
packages/vinext/src/shims/font-google.generated.ts(1928 lines / ~70 KB)font-google.tsto exportcreateFontLoaderinstead of re-exporting the catalogcreateFontLoaderexport tonext-shims-font-google.generated.d.tsfor IDE autocompletevinext:google-fontsVite plugin to perform import-time transforms in dev + buildfont-google-base.tsto handle underscore-style names (Roboto_Mono→'Roboto Mono') for users who skip the Vite plugingenerate-google-fonts.jsto only generate.d.ts(no longer generates.tscatalog)createFontLoaderAPI instead of named exportsTesting
font-google.test.tstests passshims.test.tspassentry-templates.test.tstests pass