fix(next/font/google): use real axis range, validate options at build time (#885)#893
Conversation
commit: |
Replace the inline URL builder in shims/font-google-base.ts with a thin
wrapper around the pure buildGoogleFontsUrl helper from PR 1
(packages/vinext/src/build/google-fonts/build-url.ts). The shim still
takes a FontOptions object and still returns a CSS URL, so external
callers and the existing 84 unit tests are unaffected.
Two visible behaviour changes:
1. Empty options no longer emit `:wght@100..900`. They emit no axis
segment at all, which makes Google return the default static face
(HTTP 200) for every font, including narrow-axis families like Sen
that previously got HTTP 400. This is the dev fallback only; the
build plugin always pre-resolves the real axis range from metadata
in production.
2. Italic-only requests no longer drop the ital axis. Pre-fix the outer
guard `weights.length > 0 || styles.length > 0` entered the block
but the inner branch only handled the wght path, so a call like
`Inter({ style: ['italic'] })` produced `family=Inter&display=swap`
with no italic anywhere in the URL. The shim now derives an ital
array from style and falls back to weight '400' when ital is set
without an explicit weight, so Google has somewhere to attach the
ital axis in the URL.
The shim deliberately does not import font-data.json or validate.ts: a
388 KB metadata file in the Worker bundle would dwarf the rest of the
shim, and the production path already has the metadata-aware variant.
Refs cloudflare#885.
…ld errors Replace the inline URL builder in plugins/fonts.ts with the pipeline added in PR 1: validateGoogleFontOptions parses the call's options against the bundled font metadata, getFontAxes resolves the actual `wght` range for variable fonts, and buildGoogleFontsUrl emits the URL. Three behaviour changes: 1. Variable fonts called with no `weight` now request the font's real axis range (Sen 400..800, Inter 100..900) instead of a hardcoded 100..900. This is the user-visible fix for issue cloudflare#885: Sen returns HTTP 200 with all five face files instead of HTTP 400. 2. Validation errors surface as build errors with the source file path attached. Unknown family, missing weight on a static font (Anton, Archivo Black, etc.), unknown weight, mixing 'variable' with explicit weights, axes on non-variable fonts, invalid display values, and missing/unknown subsets are now all caught at transform time. Pre-fix these silently produced URLs that Google rejected at request time. 3. HTTP non-2xx responses from Google are now distinguished from network errors. A 4xx/5xx is wrapped in GoogleFontsHttpError and surfaced with the failing URL plus the response body, because pre-fix the silent `catch { return; }` skipped self-hosting and let the runtime CDN path retry the same broken URL in the browser. Network errors (offline dev) still fall through silently to keep `vp dev` usable without a connection. The fontCache key is the URL, so existing on-disk caches under `.vinext/fonts/<hash>/` will be stale for affected fonts after this change. One refetch per font on the next build. Refs cloudflare#885.
… validator throws
Five new tests in tests/font-google.test.ts plus subset additions to
existing fixtures whose Inter() / Roboto() calls were missing them.
Shim regressions:
- buildGoogleFontsUrl with empty options must not emit `:wght@100..900`
- buildGoogleFontsUrl with style-only options must keep the ital axis
visible in the URL
Plugin regressions:
- Sen() with no weight must fetch `:wght@400..800`, not `:wght@100..900`
- unknown families throw a build error with the file path attached
- non-variable fonts called without an explicit weight throw at build
time instead of silently emitting a broken URL
- HTTP 4xx/5xx from Google surfaces as a build error containing the URL
and the response body
- network errors (DNS, ECONNREFUSED, AbortError) still fall through
silently so offline dev keeps working
The existing plugin transform tests passed `Inter({ weight: '400' })`
without subsets; the validator now treats that as a missing-subsets
error (matches Next.js behaviour), so those fixtures gain
`subsets: ['latin']`. The two nested-brace regression tests still
exercise the parser's balanced-brace logic but use a benign
`_placeholder` field instead of `axes: { wght: 400 }`, which is
semantically invalid for Inter under the new validator.
Refs cloudflare#885.
NathanDrake2406
force-pushed
the
nathan/fix-font-google-narrow-axis
branch
from
5d82825 to
5f68e02
Compare
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nice fix — wiring the metadata-driven pipeline into the plugin is the right shape, the error layering (HTTP error vs network error) is clean, and the regression tests target the actual reported bug paths.
A few things to address before merge:
1. Real (non-test) example will break the build. The PR description says "All in-tree examples already pass subsets explicitly," but examples/app-router-nitro/app/layout.tsx calls Geist_Mono({ variable: "--font-mono" }) with no subsets and no preload: false. Under the new validator that throws Preload is enabled but no subsets were specified for font \Geist Mono``. Anyone who runs this example after the upgrade hits a hard build error. Either fix the example in this PR or call it out in the risks section.
2. Italic-only test is too weak to catch the bug it documents. expect(url).toContain("ital") passes for family=Inter:ital,wght@0,400 too (i.e. ital=0, no italic served). Worth pinning the exact axis values so a future regression on the dev fallback doesn't sneak past.
3. Behavioral change for italic-only worth flagging in release notes. Pre-port code emitted both ital=0 and ital=1 for italic-only requests; the new code emits only ital=1. This matches Next.js (and is the documented intent of style: ['italic']), but it is user-visible and not the same set of glyphs they're getting today.
4. Lost coverage for the documented axes option. The two existing brace-balancing tests previously used axes: { wght: 400 } as the nested-object payload. Renaming to _placeholder keeps the brace coverage but removes any end-to-end exercise of the axes option through the plugin → validator → URL builder path. The unit tests under tests/google-fonts/ cover the helpers, but a single plugin-level test that actually validates and emits a URL with a real axes: ['opsz'] (or similar) for a font that has non-wght variable axes would close the gap.
5. Minor — axes: { wght: 400 } (object form) is now an opaque programmer error. It's valid syntax, parses cleanly, but the validator throws Invalid axes value for font \X`, expected an array of axesfrom insidegetFontAxesonly whenweights[0] === 'variable'. For non-variable callers (the most common case), it throws Axes can only be defined for variable fonts.which is misleading when the real problem is the shape, not the font. Not a blocker, but a user upgrading from old vinext code may hit this and the message could be more explicit about "expectedstring[]`".
All of (2)–(5) are polish; (1) is the only one I'd consider a hard blocker since it ships broken-out-of-the-box example code.
| // dropped. Italic-only must now leave a visible ital axis in the URL. | ||
| const { buildGoogleFontsUrl } = await import("../packages/vinext/src/shims/font-google.js"); | ||
| const url = buildGoogleFontsUrl("Inter", { style: ["italic"] }); | ||
| expect(url).toContain("ital"); |
There was a problem hiding this comment.
This assertion is too loose — :ital,wght@0,400 (ital=0, the regular face) would also satisfy toContain("ital"), so the test would still pass even if the bug it documents (italic dropped, regular served) regressed in the dev fallback. Tighten it to assert the actual axis values:
| expect(url).toContain("ital"); | |
| const { buildGoogleFontsUrl } = await import("../packages/vinext/src/shims/font-google.js"); | |
| const url = buildGoogleFontsUrl("Inter", { style: ["italic"] }); | |
| const decoded = decodeURIComponent(url); | |
| // Italic-only must produce ital=1 (italic), not ital=0 (regular). | |
| expect(decoded).toContain(":ital,wght@1,400"); | |
| expect(decoded).not.toContain("ital,wght@0,"); |
| const code = [ | ||
| `import { Inter } from 'next/font/google';`, | ||
| `const inter = Inter({ subsets: ["latin"], axes: { wght: 400 } });`, | ||
| `const inter = Inter({ subsets: ["latin"], _placeholder: { wght: 400 } });`, |
There was a problem hiding this comment.
Renaming axes → _placeholder here keeps the brace-balancing coverage but quietly removes the only end-to-end test that exercised a real axes: ... option through the plugin → validator → URL builder path. Consider adding a separate test that uses a valid axes: ['opsz'] (or another non-wght variable axis) on a variable font that has it, asserting the URL contains the expected variable-axis segment. That keeps the brace-nesting coverage you have here and also locks in the documented axes option behavior.
| // with the file path attached so Vite reports the offending | ||
| // call site instead of a generic plugin error. | ||
| const message = err instanceof Error ? err.message : String(err); | ||
| throw new Error(`[vinext:google-fonts] ${id}: ${message}`); |
There was a problem hiding this comment.
Two small ergonomic improvements worth considering:
-
The
[vinext:google-fonts] ${id}: ${message}prefix is good, but Vite already prefixes plugin errors with the plugin name. You'll end up with something like[plugin vinext:google-fonts] [vinext:google-fonts] /app/layout.tsx: Unknown font.... Consider dropping the manual prefix or usingthis.error(...)inside the transform hook so Vite formats the location automatically. -
For users upgrading old code that passed
axes: { wght: 400 }(object form rather thanstring[]), the resulting error isAxes can only be defined for variable fonts.for non-variable fonts (misleading) orInvalid axes value for font X, expected an array of axesfor variable fonts (better, but only after the variable check passes). A pre-flight check on the option shape would give a clearer migration message — not blocking, but would smooth the upgrade for anyone who copy-pasted from old Next.js examples that occasionally show the object form.
| // through to a CDN URL that ships the same bad request | ||
| // to the browser. | ||
| throw new Error( | ||
| `[vinext:google-fonts] ${id}: Google Fonts returned HTTP ${err.status} for ${err.url}.\n${err.responseBody.trim() || "(empty response body)"}`, |
There was a problem hiding this comment.
Including err.responseBody directly in the thrown message is helpful for the URL+axis case (Google's CSS comment is short and informative), but for some failure modes Google returns an HTML error page that is several KB. Consider truncating the body to a sane length (e.g. 500 chars) to avoid hostile noise in the build output:
| `[vinext:google-fonts] ${id}: Google Fonts returned HTTP ${err.status} for ${err.url}.\n${err.responseBody.trim() || "(empty response body)"}`, | |
| const body = err.responseBody.trim(); | |
| const truncated = body.length > 500 ? `${body.slice(0, 500)}\n... (truncated ${body.length - 500} bytes)` : body; | |
| throw new Error( | |
| `[vinext:google-fonts] ${id}: Google Fonts returned HTTP ${err.status} for ${err.url}.\n${truncated || "(empty response body)"}`, | |
| ); |
| // The plugin's metadata-aware path covers the variable-font case in | ||
| // production. | ||
| const wght = weights.length > 0 ? weights : ital ? ["400"] : undefined; | ||
|
|
There was a problem hiding this comment.
Edge case worth a comment or guard: if a caller in dev passes weight: 'variable' (the same sentinel the plugin uses), this code emits :wght@variable which Google rejects with HTTP 400 — same failure mode as the bug being fixed, just on a different code path. Pre-port code had the same behavior so it isn't a regression, but since this whole rewrite is about not producing URLs Google rejects, it would be worth either:
- Special-casing
weights = ['variable']to drop the wght axis entirely (matches "empty options" behavior), or - Adding a one-line comment that this case is intentionally unhandled in the dev fallback because the shim has no metadata.
The production path covers it correctly via getFontAxes, so this is purely about the dev experience.
| const wght = weights.length > 0 ? weights : ital ? ["400"] : undefined; | ||
|
|
||
| return buildUrlFromAxes(family, { wght, ital }, options.display ?? "swap"); | ||
| } |
There was a problem hiding this comment.
Behavioral note for the changelog: pre-port code emitted both ital=0 and ital=1 for italic-only requests (so the regular face also shipped); the new code emits only ital=1. This matches Next.js and is the documented intent of style: ['italic'], but it is a user-visible diff — anyone relying on the side effect of also getting the regular face will see different rendering. Worth calling out in release notes.
|
Review posted successfully. Summary of what I flagged: Hard blocker:
Polish / non-blocking: Review URL: #893 (review) |
|
/bigbonk review |
There was a problem hiding this comment.
Re-review after the address google font review feedback commit (50e0b79). The previous bonk review's hard blocker and all polish items have been addressed:
- (blocker) Nitro example:
Geist_Mono({ ..., subsets: ["latin"] })— fixed. - (polish) italic-only test assertion: now pins
:ital,wght@1,400and negatesital,wght@0,— fixed. - (polish) lost
axesend-to-end coverage: new testself-hosts variable font axes through the plugin pipelineexercisesaxes: ['opsz']onRoboto_Flexthrough validate → getFontAxes → buildGoogleFontsUrl, assertingRoboto+Flex:opsz,wght@8..144,100..1000. The expected key/value ordering matches the URL builder's lowercase-first sort and the variant-value sort, so this is a real exercise of the documentedaxesoption, not just brace-balancing — fixed. - (polish) HTTP error body truncation: extracted into
formatGoogleFontsErrorBodywith a 500-char cap and an(empty response body)sentinel — fixed. - (polish)
weight: 'variable'dev fallback: shim now drops the unresolved sentinel (normalizedWeights = weights.length === 1 && weights[0] === 'variable' ? [] : weights) instead of emitting:wght@variable. Covered by the new dev-fallback test — fixed. - (extra) clearer
axesshape error: validator now rejects non-arrayaxesbefore the variable-font check, with a separate test intests/google-fonts/validate.test.ts. Earlier this surfaced as the misleadingAxes can only be defined for variable fonts.for non-variable callers — fixed.
The behavioural changes (italic-only no longer ships ital=0, dev fallback emits no axis segment for empty options, stricter subsets validation) are listed in the PR description's risks section.
Verified the math in the HTTP-error test: '/* axis range out of bounds */' + 'x'.repeat(600) is 629 chars, no whitespace to trim, so truncatedLength = 629 - 500 = 129 matches the regex.
One small non-blocking nit remaining; see inline comment. LGTM otherwise.
| // through to a CDN URL that ships the same bad request | ||
| // to the browser. | ||
| throw new Error( | ||
| `[vinext:google-fonts] ${id}: Google Fonts returned HTTP ${err.status} for ${err.url}.\n${formatGoogleFontsErrorBody(err.responseBody)}`, |
There was a problem hiding this comment.
Non-blocking nit, carry-over from the prior review: the manual [vinext:google-fonts] ${id}: prefix duplicates Vite's own plugin-error formatting, so the user sees [plugin vinext:google-fonts] [vinext:google-fonts] /app/layout.tsx: .... Using this.error({ message, id }) from inside the transform hook would let Vite format the location and avoid the double prefix. Fine to leave for a follow-up.
|
Review posted at #893 (review). Summary: The latest commit ( |
What this changes
Wires the metadata-driven Google Fonts pipeline from #892 into the two consumers that previously hardcoded
:wght@100..900. Fixes issue #885 and related Google Fonts URL bugs.shims/font-google-base.tsbuildGoogleFontsUrlon top of the pure URL builder. No metadata in the shim, so the Worker bundle stays small. Empty options andweight: 'variable'no longer emit invalid fallback axes.plugins/fonts.tsvalidateGoogleFontOptions+getFontAxes+buildGoogleFontsUrl. Surface validation errors and HTTP 4xx/5xx as build errors, with bounded Google response bodies.build/google-fonts/validate.tsaxesvalues before font capability checks so object-form migration errors are clearer.examples/app-router-nitrosubsetsoption to itsGeist_Monocall so the example still builds under the stricter validator.tests/*weight: 'variable'fallback behavior, plugin-levelaxes, validation errors, HTTP error truncation, and network-error fallback.Why
Issue #885: vinext hardcoded
:wght@100..900in two URL builders, so fonts whosewghtaxis is narrower (Sen 400..800, Anton 400) returned HTTP 400 from Google and rendered assans-seriffallback. Investigation found related cases broken by the same code path: italic-only style requests dropped italic in dev fallback, the documentedaxesoption was ignored by the plugin path,weight: 'variable'could produce a rejected dev fallback URL, and unknown families silently produced URLs that Google rejected at request time.Approach
weight: 'variable'is dropped in the dev fallback because the shim intentionally has no metadata.axes: ['opsz']through the plugin pipeline, and clearer object-form axes errors.Validation
vp test run tests/font-google.test.ts tests/google-fonts/build-url.test.ts tests/google-fonts/validate.test.ts: 120 tests passvp check examples/app-router-nitro/app/layout.tsx packages/vinext/src/plugins/fonts.ts packages/vinext/src/shims/font-google-base.ts packages/vinext/src/build/google-fonts/validate.ts tests/font-google.test.ts tests/google-fonts/validate.test.ts: cleanvp run --filter vinext-app-router-nitro build: passRisks / release notes
fontCacheis keyed by URL. Existing on-disk caches under.vinext/fonts/<hash>/are stale for affected fonts; one refetch per font on next build.subsets: [...]orpreload: false; in-tree examples now satisfy this.ital=1instead of accidentally also shipping the regular face. This matches Next.js and the intent ofstyle: ['italic'], but it is user-visible for code that relied on the old side effect.weight: 'variable', so dev mode under dynamic options returns Google's default static face instead of a full variable range. Production uses the metadata-aware path and is unaffected.Playwrite AR Guides(empty subsets in current upstream metadata). If a future canary refresh adds a subset to that family the fixture needs to change.Closes #885.