fix(fonts): filter preloads by subsets and externalize @font-face CSS#1899
fix(fonts): filter preloads by subsets and externalize @font-face CSS#1899james-elicx wants to merge 1 commit into
Conversation
…#1897) next/font/google ignored the `subsets` option: every unicode subset's .woff2 was preloaded (13 extra preloads for two fonts in the report), and the full @font-face CSS was inlined into every HTML response as a <style data-vinext-fonts> block (~12.7 KB uncompressed). Match Next.js behavior: - Port findFontFilesInCss from Next.js: parse the `/* subset */` comments in the Google Fonts css2 response and only emit <link rel="preload"> (and Link header entries) for files in the requested `subsets`; emit none for `preload: false`. All @font-face rules stay in the CSS — each carries a unicode-range, so the browser only downloads files matching page content. - Write the served @font-face CSS as an external font.<contenthash>.css next to the cached font files and reference it via <link rel="stylesheet"> instead of inlining it. The build plugin now injects `_vinext.font.selfHostedCSSUrl` + `preloadFontFiles` instead of embedding the CSS string in the bundle; writeBundle copies the stylesheet into the client output (served immutable) and the dev middleware serves it from the cache dir. The tiny class/variable/ fallback-metric rules stay inline.
|
/bigbonk review for issues |
| }; | ||
| globalThis.fetch = async (input: unknown, init?: RequestInit) => { | ||
| const url = resolveFetchUrl(input); | ||
| if (url.includes("fonts.googleapis.com")) { |
| const css = isMono ? subsetCss("Geist Mono", "geistmono") : subsetCss("Geist", "geist"); | ||
| return new Response(css, { status: 200, headers: { "content-type": "text/css" } }); | ||
| } | ||
| if (url.includes("fonts.gstatic.com")) { |
There was a problem hiding this comment.
Review: fix(fonts): filter preloads by subsets and externalize @font-face CSS
Reviewed the full diff against Next.js's actual findFontFilesInCss behavior. The implementation is correct and matches Next.js parity well. All targeted suites pass locally (find-font-files-in-css, font-google, app-router-font-google-prod, app-router-font-google-subsets-prod) and vp check is clean on the changed source files.
Verified correct
findFontFilesInCssport is faithful. Compared line-by-line againstpackages/font/src/google/find-font-files-in-css.tson canary — same regexes (/\/\* (.+?) \*\//and/src: url\((.+?)\)/), same subset-tracking walk, same!!subsetsToPreload?.includes(...)flag, same dedup semantics. The only change (aSetfor dedup vsArray.some) is a behavior-preserving optimization.subsets/preload: falsefiltering flows correctly:validated.preload ? validated.subsets : undefinedintofindFontFilesInCss, sopreload: falseyields zero preloads while all subsets remain in the served CSS — confirmed by the subsets prod test (1 preload for latin-only Geist, 0 forpreload: falseGeist Mono, all three/* subset */comments present in the stylesheet).- External stylesheet caching.
font.<hash>.csslands under_next/static/_vinext_fonts/..., whichstatic-file-cache.tsflags as hashed (startsWith("_next/static/")) →immutable. Matches the test assertion. - Multi-call preload dedup. Preload collection runs outside the stylesheet
injectedFontsdedup, so two loader calls sharing a stylesheet (same axes, identical content hash) but different subsets both contribute, deduped at the href level viassrFontPreloadHrefs. The legacyselfHostedCSSfallback path remains intact and itssrc: local(...)fallback face correctly produces no spuriousurl()preloads. - Stale-CSS cleanup is race-safe. The
servedCssFileNameis a deterministic content hash, so concurrent transforms across RSC/SSR/client environments compute the same filename; the cleanup loop never deletes the in-use file and the write is idempotent (rmSync({ force: true })+if (!exists)).
Minor: stale code comment (non-blocking)
packages/vinext/src/plugins/fonts.ts:717-718 — the dev middleware comment is now inaccurate:
// CONTENT_TYPES is the same map prod-server uses, so fonts get
// identical MIME types in dev and prod. fetchAndCacheFont only
// ever writes .woff2/.woff/.ttf, all of which are covered.This middleware now also serves the font.<hash>.css stylesheet written by injectSelfHostedCss, not just the font files written by fetchAndCacheFont. It works correctly (.css is in CONTENT_TYPES → text/css, exercised by the updated e2e spec), but the comment should be updated to reflect that .css is served here too. Suggest something like: "fetchAndCacheFont writes .woff2/.woff/.ttf, and injectSelfHostedCss writes the served font.<hash>.css stylesheet — all covered by CONTENT_TYPES."
Conclusion
No correctness or parity issues found. The dev/prod/Link-header paths all derive from the same rewritten CSS, and the change is well-tested. The only nit is the stale comment above, which is safe to address in a follow-up or a quick amend.
|
Review posted to PR #1899. SummaryI reviewed PR #1899 thoroughly, verifying the implementation against Next.js's actual Findings:
One minor nit (non-blocking): the dev-middleware comment at |
Fixes #1897.
Problem
next/font/googlehad two intertwined parity gaps:subsetswas ignored for preloading. Google's css2 response contains one@font-faceblock per available unicode subset; vinext preloaded every subset's.woff2regardless of thesubsetsoption (13 unnecessary preloads for two fonts in the report).preload: falsewas also ignored.@font-faceCSS was inlined into every HTML response as a<style data-vinext-fonts>block (~12.7 KB uncompressed in the report), so neither the browser nor a CDN could cache it.What Next.js actually does (and this PR now matches)
The css2 API has no
subsetquery parameter — Next.js keeps all subsets'@font-facerules in the CSS (each carries aunicode-range, so browsers only download files matching page content) and filters only the preloads: it parses the/* subset */comments in Google's CSS and emits<link rel="preload">solely for files in the requestedsubsets(none forpreload: false). The font CSS lands in an external cacheable stylesheet, not the HTML.Changes
build/google-fonts/find-font-files-in-css.ts— direct port of Next.js'sfindFontFilesInCss: walks the CSS tracking/* subset */comments and flags which files should be preloaded.plugins/fonts.ts— the transform now computes a subset-filtered preload list and writes the served@font-faceCSS as an externalfont.<contenthash>.cssnext to the cached font files. It injects_vinext.font.selfHostedCSSUrl+preloadFontFilesinstead of embedding the whole CSS string in the bundle.writeBundlecopies the stylesheet into the client output alongside the.woff2files (served with immutable caching); the dev middleware serves it from.vinext/fonts/.shims/font-google-base.ts— emits a<link rel="stylesheet">for the external CSS and uses the explicit preload list (deduped at the href level so multiple loader calls sharing a stylesheet but requesting different subsets all contribute). The tiny class/variable/fallback-metric rules (nourl()) stay inline; the legacy inlineselfHostedCSSpath is kept for the dynamic-options fallback. App Router, Pages Router, and the HTTPLinkheader all pick this up via the sharedgetSSRFontLinks/getSSRFontPreloadscollectors.Tests
tests/app-router-font-google-subsets-prod.test.ts+tests/fixtures/font-google-subsets(multi-subset mock faithful to real css2 responses): exactly one preload for a latin-only font, zero forpreload: false, filteredLinkheader, external stylesheet servestext/css+immutablewith all subsets present, and nourl(left inline in HTML. Before the fix this showed 6 preloads and 0 external stylesheets.findFontFilesInCss./* latin */comment real Google responses always carry), and extended the dev-mode Playwright spec to verify the external stylesheet serves in dev. The unit test hitting real Google Fonts confirms real Inter CSS yields a single latin preload.All font-related suites (215 tests), the dev e2e spec, and
vp checkpass.