port Next.js Google Fonts metadata + URL pipeline (PR 1 of 2 for #885)#892
Conversation
Adds packages/vinext/src/build/google-fonts/font-data.json (1911 entries, 388 KB pretty-printed) plus a typed re-export wrapper. The metadata enumerates each Google Font's available weights, styles, variable axes, and preloadable subsets, and is the source of truth a faithful next/font/google port needs to encode the right URL for variable fonts and reject invalid options at build time. Today vinext hardcodes :wght@100..900 in both the shim runtime URL builder and the build-time plugin, which produces HTTP 400 from Google for fonts whose wght axis is narrower (Sen 400..800, Anton single 400). Without metadata there is no way to emit the correct axis range, validate weights against the font's real options, or auto-disable preload for fonts with no preloadable subsets. The JSON itself is consumed only at build time and during dev. Production Workers run the self-hosted CSS injected by the build plugin and never read this file. The pretty-printed shape matches the upstream layout so future refreshes from canary remain reviewable. This commit only vendors the data and types. URL pipeline and validator land in the next two commits. Refs cloudflare#885.
Adds three pure helpers under packages/vinext/src/build/google-fonts:
sortFontsVariantValues (numeric variant comparator), buildGoogleFontsUrl
(URL assembler), and getFontAxes (metadata-driven axis resolver). Each is a
direct port of the matching module in vercel/next.js packages/font/src and
has no defaults of its own. Defaults live in the validator (next commit).
This is the layer that translates a validated options object into the URL
shape Google Fonts CDN actually accepts. Sorting matters because Google
rejects out-of-order variant lists with HTTP 400. The metadata-driven axis
resolver is what lets a Sen({}) call produce :wght@400..800 instead of the
broken :wght@100..900 default.
No consumer wired up yet. The shim and plugin still call the old hardcoded
URL builders; the integration lands in PR 2.
Tests: 21 new tests across the three modules. Each helper is exercised in
isolation against the vendored metadata, including the regression case
from cloudflare#885 (variable Sen resolves to wght 400..800) and the italic-only
branch that vinext currently drops.
Refs cloudflare#885.
Adds validateGoogleFontOptions, the parse-once boundary for next/font/google calls. Mirrors Next's validate-google-font-function-call.ts, dropping the SWC-coupled (functionName, fontFunctionArgument) signature in favour of (fontFamily, options). Returns ValidatedGoogleFontOptions, a type whose existence proves the input was rejected if Google would have rejected it. Behavior matches Next.js: throws on unknown family, unknown weight, mixing 'variable' with explicit weights, axes on non-variable fonts, axes without weight 'variable', invalid display, unknown subsets, preload with no subsets. Defaults missing weight to ['variable'] for variable fonts, and auto-disables preload for fonts with no preloadable subsets. This closes the loop on the URL pipeline added in the previous commit. Together with getFontAxes and buildGoogleFontsUrl, vinext now has a metadata-driven path from caller options to a valid Google Fonts URL. The shim and plugin still build URLs the old way; PR 2 wires this through. Tests: 16 new tests covering every throw branch and every default branch, plus an index.ts barrel that the future plugin import will use. The "auto-disable preload" test pins on Playwrite AR Guides, which has empty subsets in upstream metadata; if a future canary refresh adds a subset to that family the test will need a different fixture. Refs cloudflare#885.
commit: |
Drop NOTICE.md and the index.ts barrel, demote four type aliases from exports to module-internal. Knip flagged the index file as unused (no consumer until PR 2) and the type aliases as unused exports for the same reason; YAGNI says wait for the consumer rather than ship dead public API. Vercel's MIT copyright on font-data.json is preserved in the font-metadata.ts header instead of in a sibling NOTICE.md, matching the in-tree pattern Next.js uses for vendored third-party files. Tests: vp run knip exits 0, 37 google-fonts tests still pass.
|
/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 scaffolding PR. The four ports faithfully match the Next.js originals (verified line-by-line against vercel/next.js@canary), the 37 new tests cover the throw branches and the #885 regression case, all 84 existing font-google.test.ts tests still pass, vp check is clean, and vp run vinext#build produces a working bundle that inlines the JSON. The PR description is unusually clear about scope and the cleanup commit rationale.
A few observations (none blocking):
-
subsets: []+preload: truediverges from Next.js. Next.js'svalidateGoogleFontFunctionCallchecksif (!subsets)(truthy check), and[]is truthy, so explicitly passingsubsets: []withpreload: truesilently passes through. vinext defaultssubsets = []and then checkssubsets.length === 0, so it throws on bothundefinedand[]. In practice this only affects callers passingsubsets: []literally, which is a corner case, and arguably vinext's behavior is more correct — but it is a quiet behavioral divergence worth either matching or documenting. -
Refresh ergonomics for
font-data.json. The 16K-line vendor commit is fine for a one-off, but a smallscripts/refresh-google-fonts-metadata.{sh,ts}that fetches/copies and re-runsoxfmtwould make periodic refreshes safer than "re-copy from upstream". Not for this PR; mentioning so it doesn't get lost. -
Playwrite AR Guidesfixture fragility. The PR description already calls this out. If you want belt-and-braces, a tiny helper test likeexpect(Object.values(googleFontsMetadata).some((f) => f.subsets.length === 0)).toBe(true)would still cover the empty-subsets branch even if the specific family changes upstream, and thePlaywrite AR Guides-specific test could become a soft skip if the metadata changes. Fine to defer. -
with { type: "json" }import attribute. Works (rolldown rewrites to a regular import in the bundled output), but the project's TS config usesmoduleResolution: bundlerwithresolveJsonModule: true, so the attribute is decorative. Harmless either way — just calling it out in case you ever build under a stricter Node ESM loader where the attribute starts mattering. -
Error name parity. Next.js's
nextFontErrorsetserr.name = 'NextFontError'so its WellKnownErrorsPlugin can format them. vinext throws plainError. That's correct for vinext (you don't have that plugin), but if PR 2 ever wants to surface these as build errors with a distinct prefix, aclass GoogleFontError extends Errorwould make filtering easier later. Optional.
Overall this looks like a clean, low-risk scaffolding PR. LGTM pending any of the parity decisions above.
| // No preloadable subsets means preload is meaningless. Silently disable | ||
| // it rather than forcing the caller to opt out. | ||
| preload = false; | ||
| } else if (preload) { |
There was a problem hiding this comment.
Subtle parity gap with Next.js: upstream uses if (!subsets) (truthy check), so an explicit subsets: [] with preload: true passes silently and subsets.forEach is a no-op. Here subsets defaults to [] and then subsets.length === 0 throws, so vinext rejects subsets: [] where Next.js would accept it.
Probably benign — nobody passes [] deliberately — and vinext's behavior is arguably more correct, but it is a quiet behavioral divergence. If you want exact parity:
| } else if (preload) { | |
| } else if (preload) { | |
| if (!options.subsets) { | |
| throw new Error( | |
| `Preload is enabled but no subsets were specified for font \`${fontFamily}\`. Please specify subsets or disable preloading if your intended subset can't be preloaded.\nAvailable subsets: ${formatAvailableValues(availableSubsets)}`, | |
| ); | |
| } | |
| for (const subset of subsets) { |
Otherwise consider dropping a comment here noting the deliberate divergence so a future reader doesn't "fix" it back to upstream behavior.
| // and vinext are MIT licensed, so the file is redistributed under the same | ||
| // terms. | ||
|
|
||
| import rawFontData from "./font-data.json" with { type: "json" }; |
There was a problem hiding this comment.
Minor: with moduleResolution: bundler + resolveJsonModule: true (this repo's tsconfig) the with { type: "json" } attribute is decorative — tsdown/rolldown rewrites it to a plain import in dist/build/google-fonts/font-data.js. It is harmless and forward-compatible with stricter Node ESM JSON loading, so I'd keep it as-is, but worth knowing in case the import ever fails to resolve in some downstream tool that doesn't understand the attribute syntax.
| // need a different fixture. | ||
| const opts = validateGoogleFontOptions("Playwrite AR Guides", { weight: "400" }); | ||
| expect(opts.preload).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Belt-and-braces idea (optional): the Playwrite AR Guides pin is fragile because if upstream Google ever adds a subset to that family, this test silently starts asserting the wrong branch. A complementary metadata-driven test would be more robust:
it("has at least one font family with no preloadable subsets (sanity check for the auto-disable branch)", () => {
const families = Object.entries(googleFontsMetadata).filter(
([, meta]) => meta.subsets.length === 0,
);
expect(families.length).toBeGreaterThan(0);
});Then the Playwrite AR Guides test can stay, but the branch coverage doesn't depend on that one family.
| @@ -0,0 +1,16071 @@ | |||
| { | |||
There was a problem hiding this comment.
Follow-up suggestion (not blocking): a small scripts/refresh-google-fonts-metadata.{sh,ts} that fetches vercel/next.js@canary:packages/font/src/google/font-data.json and runs vp fmt would make periodic refreshes mechanical. Manually re-copying a 16K-line JSON every few months is a recipe for someone accidentally hand-editing it. Worth filing as a separate issue.
|
Review posted successfully: #892 (review) SummaryReviewed PR #892 — a scaffolding PR that vendors Verification performed:
Findings (all non-blocking):
Overall LGTM. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else if (axes.variableAxes) { | ||
| // Variable font with no requested wght: emit only the other variable axes. | ||
| variants.push([...axes.variableAxes]); | ||
| } | ||
|
|
||
| // Google requires axis tags within a variant to be ordered with lowercase | ||
| // tags first, then alphabetically. Every variant must agree on the same | ||
| // key order, since the URL takes the key list from variants[0] and | ||
| // applies it to every variant's value list. | ||
| if (axes.variableAxes) { | ||
| for (const variant of variants) { |
There was a problem hiding this comment.
axes.variableAxes is treated as truthy even when it's an empty array. In that case this branch pushes an empty variant, and later the function will build an invalid axis segment (...family=Foo:@...) because the key/value lists are empty. Consider gating on axes.variableAxes?.length > 0 here (and in the later if (axes.variableAxes) block) so empty arrays behave like undefined.
| const bIsLowercase = b.charCodeAt(0) > 96; | ||
| if (aIsLowercase && !bIsLowercase) return -1; | ||
| if (bIsLowercase && !aIsLowercase) return 1; | ||
| return a > b ? 1 : -1; |
There was a problem hiding this comment.
The axis-tag sort comparator never returns 0 when tags are equal (return a > b ? 1 : -1). This violates the comparator contract and can produce inconsistent ordering if duplicate keys ever appear in a variant (e.g. via malformed variableAxes). Return 0 for equality (or use localeCompare) to make the sort well-defined.
| return a > b ? 1 : -1; | |
| return a.localeCompare(b); |
…e test `validateGoogleFontOptions` rejects `subsets: []` with `preload: true` where Next.js silently accepts it (Next uses a truthy check on `subsets`, vinext defaults to `[]` and checks `length`). The behavior is intentional (an empty subsets array with preload enabled is always a caller mistake), but undocumented divergence from a faithful port invites a future "fix back to upstream" PR. Annotate the divergence at the throw site. Auto-disable-preload test pins on `Playwrite AR Guides` having an empty subsets array in the bundled metadata. If a metadata refresh ever adds a subset to that family, the family-specific test silently flips into a false positive. Add an invariant test asserting the bundled metadata still contains at least one family with no preloadable subsets, so loss of branch coverage fails CI loud.
|
Reminder for Bonk: I'm not adding verbose comments spelling out everything. |
|
/bigbonk review |
There was a problem hiding this comment.
Re-reviewed at c1abf20. The two follow-ups from the prior review are addressed: validate.ts:71-73 now has an explicit comment documenting the deliberate subsets: [] divergence from Next.js, and validate.test.ts:121-126 adds the metadata-driven sanity test so the empty-subsets branch coverage doesn't depend on Playwrite AR Guides staying empty upstream.
Re-verified parity against current vercel/next.js@canary:
build-url.ts— line-for-line port ofget-google-fonts-url.ts, including the comparator and theif (axes.variableAxes)guardsort-variants.ts— matchessort-fonts-variant-values.tsget-axes.ts— matchesget-font-axes.tsvalidate.ts— matchesvalidate-google-font-function-call.tsmodulo the documentedsubsets: []divergence and the SWC calling-convention drop noted in the file header
All required CI checks are green (Check, Vitest unit + 3 integration shards, all 9 E2E projects).
Addressing the new Copilot comments (both non-blocking, both already match upstream exactly):
-
Empty
variableAxesarray treated as truthy — Next.js uses the sameif (axes.variableAxes)truthiness guard and the sameaxes.variableAxes ?? []spread. In practicevariableAxesis constructed byget-axes.ts, which only assigns it viaif (!variableAxes) variableAxes = []; variableAxes.push(...), so the empty-array shape is unreachable from the ported pipeline. Tightening the guard would be a deliberate divergence from upstream for an unreachable case. -
Sort comparator never returns 0 — Next.js's
get-google-fonts-url.tsuses the exact samereturn a > b ? 1 : -1form. Axis tags within a variant are unique by construction (one entry per axis tag), so the equal-tags case doesn't arise in practice. A port that diverges from upstream comparator behavior is more risk than reward here.
LGTM. The follow-up work belongs in PR 2 as already scoped in the description.
|
Approval review posted: #892 (review) SummaryRe-reviewed PR #892 at commit Verification performed:
Addressed the new Copilot review comments (both non-blocking, both already match upstream exactly):
PR is good to merge; the deferred items (silent fetch catch, Sen fixture, |
What this changes
Vendors
font-data.jsonfromvercel/next.jsand ports four pure helpers that consume it. Build-time scaffolding only; no runtime wired up.font-data.jsonnext.js/packages/font/src/google/font-data.jsonfont-metadata.tssort-variants.tsnext.js/packages/font/src/google/sort-fonts-variant-values.tsbuild-url.tsnext.js/packages/font/src/google/get-google-fonts-url.tsget-axes.tsnext.js/packages/font/src/google/get-font-axes.tsvalidate.tsnext.js/packages/font/src/google/validate-google-font-function-call.tsWhy
Issue #885:
:wght@100..900is hardcoded in the shim and plugin URL builders, so fonts with narrower axes (Sen 400..800, Anton 400) get HTTP 400 from Google. Investigation also found four sibling bugs in the same code: italic-only requests drop italic, theaxesoption is ignored,weight: 'variable'400s, unknown families build URLs silently.The minimal fix (drop the hardcoded range) regresses Inter from 9 weights to 1. Next.js avoids both regressions by validating + resolving axes against bundled metadata. This PR vendors that metadata and ports the helpers; PR 2 swaps the consumers and adds a Sen fixture.
Approach
Three commits: vendor metadata, port URL pipeline, port validator. A fourth cleanup commit drops
NOTICE.mdand an unused barrel aftervp run knipflagged them; Vercel's MIT attribution lives in thefont-metadata.tsheader instead.Validation
vp test run tests/google-fonts/: 37 new tests pass (regression case fornext/font/googleshim hardcodes:wght@100..900when no weight is specified, causing HTTP 400 from Google Fonts for fonts with a narrower weight range #885, italic-only branch, axes on non-variable, unknown family, unknown subsets, preload auto-disable, every throw branch)vp test run tests/font-google.test.ts: 84 existing tests still passvp check packages/vinext/src/build/google-fonts/ tests/google-fonts/: cleanvp run knip: cleanRisks / follow-ups
next.js/packages/font/src/google/font-data.json.Playwrite AR Guides. If upstream adds a subset to that family the fixture needs to change.PR 2 (next)
shims/font-google-base.tsandplugins/fonts.tsto use the new pipelinecatch { return; }around the build-time fetch so 400s surface as build errorstests/fixtures/font-narrow-axis/(Sen) for CI regression coveragetests/font-google.test.tsfor the new throw pathsRefs #885.