fix(fonts): stabilize next/font/google runtime registrations#956
Conversation
Repeated next/font/google calls currently mint counter-based class names and append new SSR style rules. During dev HMR, the app module can re-evaluate while the shim module stays alive, so stale classes and links accumulate and the first emitted :root variable keeps winning. The bad invariant was treating each runtime call as a new font registration. Use a deterministic identity for each family and options pair, remove the non-Next :root variable rule, and keep variable declarations scoped to the returned variable class. Regression coverage verifies stable class identities, no :root emission, and no SSR style growth for repeated equivalent calls.
commit: |
There was a problem hiding this comment.
Pull request overview
Stabilizes the next/font/google runtime shim so equivalent (family, options) registrations reuse deterministic class identities, preventing SSR/head growth during dev HMR and aligning variable behavior more closely with Next.js.
Changes:
- Replace the module-level incrementing
classCounterwith a deterministic hash-based identity forclassName/variablegeneration. - Remove shim-only
:root { --font-... }variable emission so variables live only on the returned.variableclass. - Add/update regression tests for stable identities, lack of
:rootemission, and no SSR-style growth across repeated equivalent calls.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
packages/vinext/src/shims/font-google-base.ts |
Implements deterministic font registration identities and removes :root variable injection path. |
tests/font-google.test.ts |
Updates class-name assertions and adds regressions covering stable identity + no :root emission + no SSR style growth. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function fontClassSegment(family: string): string { | ||
| return family.toLowerCase().replace(/\s+/g, "_"); |
| const styles = mod.getSSRFontStyles(); | ||
| const newStyles = styles.slice(beforeStyles); | ||
| const hasVar = newStyles.some((s: string) => s.includes("--font-inter")); | ||
| const hasVar = styles.some((s: string) => s.includes("--font-inter")); | ||
| expect(hasVar).toBe(true); |
| normalizeStringOption(options.style), | ||
| normalizeStringOption(options.subsets), | ||
| options.display ?? "", | ||
| normalizeBooleanOption(options.preload), | ||
| normalizeStringOption(options.fallback), | ||
| normalizeStringOrBooleanOption(options.adjustFontFallback), |
The first PR version left two edge cases in the new stable identity path: crafted font family names could still leak unsafe selector characters into class names, and omitted display hashed differently from explicit display: swap even though both generate the same font URL. Sanitize the readable family segment used in selectors, normalize display to the same default used by buildGoogleFontsUrl, and tighten the CSS variable regression so it checks CSS added by the current call instead of accumulated module state.
There was a problem hiding this comment.
Pull request overview
Stabilizes the next/font/google runtime shim in vinext by generating deterministic class identities per (family, options) registration, preventing SSR <style> growth during dev/HMR re-evaluations and aligning variable behavior with Next.js (no shim-only :root emission).
Changes:
- Replace counter-based class naming with a deterministic hashed identity for class/variable names.
- Remove shim-only
:root { --font-... }variable emission; variables live only on the returned.variableclass. - Update/add regression tests for stable identity, no
:rootemission, and no SSR style growth on repeated equivalent calls.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
packages/vinext/src/shims/font-google-base.ts |
Implements deterministic identity hashing, sanitizes class segments, removes :root variable rule path, updates proxy trap typing. |
tests/font-google.test.ts |
Updates class-name shape assertions and adds regression tests for stable identity / SSR style dedupe / no :root variables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| normalizeStringOption(options.weight), | ||
| normalizeStringOption(options.style), | ||
| normalizeStringOption(options.subsets), | ||
| options.display ?? "swap", | ||
| normalizeBooleanOption(options.preload), |
Equivalent Google font loader options could generate the same CSS and font URL while still producing different class identities. That reintroduced duplicate SSR style rules when option arrays used a different order or representation. The identity hash now canonicalizes unordered option sets, default-style equivalence, and fallback whitespace before hashing. A focused regression test covers reordered weights and subsets, explicit normal style, and trimmed fallback names.
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good fix — the root cause analysis is solid and the deterministic identity approach is the right call. The normalization work and test coverage are well thought out. A few items to address:
Issues to fix:
- Fallback normalization inconsistency:
normalizeFallbackOptionpreserves order (which is correct for CSS semantics) but doesn't normalize the default fallback. CallingInter({})andInter({ fallback: ["sans-serif"] })produce identicalfontFamilystrings and identical CSS output, but different hashes, so they won't dedupe. - The
fontFamilyinput tocreateFontIdentityis redundant with the individual normalized option inputs — it's computed fromfamily+fallback, both of which are already in the hash. This isn't a bug but it couples the hash to sanitization implementation details (e.g. ifescapeCSSStringchanges, hashes change).
Nits:
3. The normalization test at line 248 expects fallback: [" Arial ", "Helvetica"] to match fallback: ["Arial", "Helvetica"], but this only works because fontFamily (which includes the trimmed/sanitized fallback from sanitizeFallback) is in the hash AND normalizeFallbackOption also trims. It works, but the dual-path normalization is fragile — if someone changes sanitizeFallback to stop trimming, the fontFamily component diverges but normalizeFallbackOption still trims, and the test still passes by accident.
4. FNV-1a with 32 bits and base-36 gives ~2 billion possible values, which is fine for class name uniqueness in practice, but worth a brief comment noting the collision probability is acceptable for this use case.
Overall this is in good shape. The two substantive items are the default-fallback normalization gap (#1) and the redundant fontFamily hash input (#2).
|
|
||
| function normalizeFallbackOption(value: string[] | undefined): string { | ||
| if (!value) return ""; | ||
| return value.map((item) => item.trim()).join(","); |
There was a problem hiding this comment.
This normalizes whitespace but doesn't normalize the absence of the fallback option vs an explicit default. When options.fallback is undefined, fontLoader defaults it to ["sans-serif"] (line 442), so both Inter({}) and Inter({ fallback: ["sans-serif"] }) produce identical fontFamily strings and identical CSS — but normalizeFallbackOption(undefined) returns "" while normalizeFallbackOption(["sans-serif"]) returns "sans-serif", giving them different hashes.
This means the dedupe goal is partially defeated for the most common case (omitting fallback since ["sans-serif"] is the default).
| return value.map((item) => item.trim()).join(","); | |
| function normalizeFallbackOption(value: string[] | undefined, defaultFallback: string[] = ["sans-serif"]): string { | |
| const resolved = value ?? defaultFallback; | |
| return resolved.map((item) => item.trim()).join(","); | |
| } |
Or alternatively, resolve the default before calling createFontIdentity and pass the resolved array.
| return hashString( | ||
| [ | ||
| family, | ||
| fontFamily, |
There was a problem hiding this comment.
Including fontFamily in the hash is redundant — it's fully determined by family + options.fallback, which are already hashed individually (lines 173 and 182). This creates a fragile coupling: the hash now depends on the exact output format of sanitizeFallback/escapeCSSString. If those ever change their quoting style (e.g., switching to double quotes), all hashes change and previously-cached identities break.
Consider dropping fontFamily from the hash input and relying on the already-normalized family + normalizeFallbackOption(options.fallback) instead.
| fontFamily, | |
| return hashString( | |
| [ | |
| family, | |
| cssVarName, | |
| normalizeWeightOption(options.weight), | |
| normalizeStyleOption(options.style), | |
| normalizeStringSetOption(options.subsets), | |
| options.display ?? "swap", | |
| normalizeBooleanOption(options.preload), | |
| normalizeFallbackOption(options.fallback), | |
| normalizeStringOrBooleanOption(options.adjustFontFallback), | |
| normalizeStringSetOption(options.axes), | |
| options._selfHostedCSS ?? "", | |
| ].join("\0"), | |
| ); |
| const cssVarName = options.variable | ||
| ? (sanitizeCSSVarName(options.variable) ?? defaultVarName) | ||
| : defaultVarName; | ||
| const id = createFontIdentity(family, options, cssVarName, fontFamily); |
There was a problem hiding this comment.
The fontFamily passed here is already resolved with the default fallback (options.fallback ?? ["sans-serif"]), but options.fallback inside createFontIdentity is still the raw undefined. This means the hash sees the resolved fontFamily string (with sans-serif) but normalizeFallbackOption(options.fallback) returns "" when fallback was omitted.
Two options:
- Resolve
options.fallbackto the default before callingcreateFontIdentity - Drop
fontFamilyfrom the hash (see my other comment) and normalizefallbackwith its default
Either way, the pre-resolution and post-resolution views of fallback should be consistent in the hash.
| Sen({ subsets: ["latin"], variable: "--font-primary" }); | ||
| Outfit({ subsets: ["latin"], variable: "--font-primary" }); | ||
|
|
||
| const styles = getSSRFontStyles().join("\n"); |
There was a problem hiding this comment.
As Copilot flagged — this test's :root assertion operates on the full accumulated ssrFontStyles, which includes styles from all prior tests in the file. The assertion expect(styles).not.toContain(":root") only proves no prior test injected :root either, not specifically that Sen/Outfit didn't.
This isn't a false positive today because the :root path is fully removed, but if a future change reintroduces :root emission for a different code path, this test would start failing for the wrong reason. Consider capturing getSSRFontStyles().length before the Sen/Outfit calls and asserting only on the newly-added slice:
| const styles = getSSRFontStyles().join("\n"); | |
| const { createFontLoader, getSSRFontStyles } = | |
| await import("../packages/vinext/src/shims/font-google.js"); | |
| const before = getSSRFontStyles(); | |
| const Sen = createFontLoader("Sen"); | |
| const Outfit = createFontLoader("Outfit"); | |
| Sen({ subsets: ["latin"], variable: "--font-primary" }); | |
| Outfit({ subsets: ["latin"], variable: "--font-primary" }); | |
| const added = getSSRFontStyles().slice(before.length).join("\n"); | |
| expect(added).not.toContain(":root"); | |
| expect(added).toContain("--font-primary: 'Sen'"); | |
| expect(added).toContain("--font-primary: 'Outfit'"); |
| expect(explicit.variable).toBe(implicit.variable); | ||
| }); | ||
|
|
||
| it("normalizes identity inputs that emit equivalent Google font output", async () => { |
There was a problem hiding this comment.
Nice test — this covers the key normalization cases (weight reorder, style ["normal"] collapse, subset reorder, fallback whitespace). Consider also adding an explicit case for the default-fallback equivalence, which is arguably the most common real-world scenario:
const withDefault = CanonicalIdentity({ weight: ["400"] });
const withExplicit = CanonicalIdentity({ weight: ["400"], fallback: ["sans-serif"] });
expect(withExplicit.className).toBe(withDefault.className);(This would currently fail due to the normalization gap I flagged on the implementation.)
| hash ^= value.charCodeAt(i); | ||
| hash = Math.imul(hash, 0x01000193) >>> 0; | ||
| } | ||
| return hash.toString(36).padStart(7, "0"); |
There was a problem hiding this comment.
Minor: padStart(7, "0") means hashes always have 7 characters, but (2^32).toString(36) is "1z141z4" — also 7 characters. So padding only matters for small hash values. The max is 7 chars, which is fine. Just noting this is a cosmetic choice that works correctly.
|
Review posted successfully. Here's a summary of what I flagged: Key issues:
Test feedback:
|
Omitting next/font/google fallback produced the same CSS as fallback: ["sans-serif"] but still hashed a different raw option shape. That left the most common default call path outside the deterministic identity dedupe. The identity hash now uses the resolved fallback list and no longer includes the serialized font-family CSS string. Regression coverage verifies implicit and explicit default fallback calls share class and variable identities, and the :root assertion now checks only styles added by its own test.
|
/bigbonk review |
There was a problem hiding this comment.
Good fix. The root cause analysis is correct — counter-based identity was the single invariant violation behind all three reported symptoms. The deterministic FNV hash approach is the right call for Phase 1, and the normalization work is thorough.
All previous review feedback has been addressed:
- Default fallback normalization gap: fixed by passing the resolved
fallback(already defaulted to["sans-serif"]) intocreateFontIdentityas a separate parameter, sonormalizeFallbackOptionalways sees the resolved array. - Redundant
fontFamilyin hash: removed. :roottest accumulated state: fixed withbeforeStylesslice.- Default-fallback equivalence test: added (line 256).
display: "swap"default normalization: handled viaoptions.display ?? "swap".- CSS injection via class selectors: handled by
fontClassSegmentsanitization. - Proxy
gettyping: improved withstring | symbol+ early guard.
One minor nit below (dead code). Not blocking.
| // The proxy intercepts property access and returns a loader for that font. | ||
| const googleFonts = new Proxy({} as Record<string, (options?: FontOptions) => FontResult>, { | ||
| get(_target, prop: string) { | ||
| const googleFontLoaders: Record<string, FontLoader> = {}; |
There was a problem hiding this comment.
Nit: googleFontLoaders is declared and used as the Proxy target, but the get trap never populates it — every property access creates a fresh FontLoader closure via createFontLoader(family). This makes the typed record effectively dead code.
Not a correctness issue since identity stability is at the fontLoader() call level. But you could cache on the target to avoid re-creating the closure on every property access (e.g., HMR re-evaluation hitting const inter = Inter(...) re-evaluates Inter from the proxy each time):
get(_target, prop: string | symbol) {
if (typeof prop !== "string") return undefined;
if (prop === "__esModule") return true;
if (prop === "default") return googleFonts;
if (prop in _target) return _target[prop];
const family = prop.replace(/_/g, " ").replace(/([a-z])([A-Z])/g, "$1 $2");
return (_target[prop] = createFontLoader(family));
}
|
Review submitted successfully. Here's a summary: Approved PR #956 with one minor nit. All previous review feedback from both Copilot and the earlier bonk review has been addressed across the 4 commits. The key fixes:
The one nit I flagged: |
What this changes
Fixes #883.
The
next/font/googleruntime shim now gives each(family, options)registration a deterministic class identity instead of using a module-level counter. Repeated equivalent calls now reuse the sameclassNameandvariableclass, so dev HMR re-evaluation no longer appends a new pair of SSR style rules every time the app module reloads.This also removes the shim-only
:root { --font-... }emission. Google font variables now live only on the returned.variableclass, matching Next.js behavior.Why
The issue is not really three separate bugs. Counter runaway, stale head rules, and pinned
:rootvariables all come from the same bad invariant: the shim treated every runtime call as a new font registration even when the call represented the same font and options.Next.js avoids this shape entirely:
Inter({ subsets: ["latin"] })into a font CSS module request:next-font-loader/index.tslines 18-30.next-font-loader/index.tslines 128-134.classNameandvariableexports into__${exportName}_${fontFamilyHash}:next-font.tslines 44-53..classNamerule and, when requested, a.variablerule. It does not emit a:rootrule:postcss-next-font.tslines 138-174.Approach
classCounter++with a deterministic FNV-based identity over the font family, effective font-family string, CSS variable name, public font options, and self-hosted CSS.injectedRootVariablesand the:rootrule path.Validation
:rootfont variable emission, and no SSR style growth for repeated equivalent calls.vp test run tests/font-google.test.tsvp test run tests/shims.test.ts -t "next/font/google shim"vp checkvp run vinext#buildRisks / follow-ups
Apps that accidentally relied on vinext injecting font variables at
:rootwithout applying the returned.variableclass will need to applyfont.variable, which is the Next.js-compatible usage. This PR intentionally does not attempt the larger Phase 2 architecture where dev font calls become Vite-owned virtual CSS modules.