-
Notifications
You must be signed in to change notification settings - Fork 318
fix(next/font/google): use real axis range, validate options at build time (#885) #893
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
86dfebd
8d20bbe
5f68e02
50e0b79
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,26 @@ import { parseAst } from "vite"; | |
| import path from "node:path"; | ||
| import fs from "node:fs"; | ||
| import MagicString from "magic-string"; | ||
| import { validateGoogleFontOptions } from "../build/google-fonts/validate.js"; | ||
| import { getFontAxes } from "../build/google-fonts/get-axes.js"; | ||
| import { buildGoogleFontsUrl } from "../build/google-fonts/build-url.js"; | ||
|
|
||
| /** | ||
| * Thrown when Google Fonts returns a non-2xx response. Distinct from a raw | ||
| * `fetch` rejection (network error, DNS failure, AbortError) so the call | ||
| * site can decide whether to surface as a build error or fall through to | ||
| * the runtime CDN path. | ||
| */ | ||
| class GoogleFontsHttpError extends Error { | ||
| constructor( | ||
| public readonly url: string, | ||
| public readonly status: number, | ||
| public readonly responseBody: string, | ||
| ) { | ||
| super(`Google Fonts returned HTTP ${status} for ${url}`); | ||
| this.name = "GoogleFontsHttpError"; | ||
| } | ||
| } | ||
|
|
||
| // ── Virtual module IDs ──────────────────────────────────────────────────────── | ||
|
|
||
|
|
@@ -63,6 +83,15 @@ const GOOGLE_FONT_UTILITY_EXPORTS = new Set([ | |
| * user-provided public files. | ||
| */ | ||
| const VINEXT_FONT_URL_NAMESPACE = "_vinext_fonts"; | ||
| const MAX_GOOGLE_FONTS_ERROR_BODY_LENGTH = 500; | ||
|
|
||
| function formatGoogleFontsErrorBody(body: string): string { | ||
| const trimmed = body.trim(); | ||
| if (!trimmed) return "(empty response body)"; | ||
| if (trimmed.length <= MAX_GOOGLE_FONTS_ERROR_BODY_LENGTH) return trimmed; | ||
| const omitted = trimmed.length - MAX_GOOGLE_FONTS_ERROR_BODY_LENGTH; | ||
| return `${trimmed.slice(0, MAX_GOOGLE_FONTS_ERROR_BODY_LENGTH)}\n... (truncated ${omitted} characters)`; | ||
| } | ||
|
|
||
| /** | ||
| * Rewrite absolute filesystem paths in cached Google Fonts CSS so the | ||
|
|
@@ -409,7 +438,11 @@ async function fetchAndCacheFont( | |
| }, | ||
| }); | ||
| if (!cssResponse.ok) { | ||
| throw new Error(`Failed to fetch Google Fonts CSS: ${cssResponse.status}`); | ||
| // Include the response body when Google rejected the request so the | ||
| // caller can see why (the body usually contains a one-line CSS comment | ||
| // identifying the bad axis or family). | ||
| const body = await cssResponse.text().catch(() => ""); | ||
| throw new GoogleFontsHttpError(cssUrl, cssResponse.status, body); | ||
| } | ||
| let css = await cssResponse.text(); | ||
|
|
||
|
|
@@ -753,50 +786,51 @@ export function createGoogleFontsPlugin(fontGoogleShimPath: string, shimsDir: st | |
| return; // Can't parse options statically, skip | ||
| } | ||
|
|
||
| // Build the Google Fonts CSS URL | ||
| const weights = options.weight | ||
| ? Array.isArray(options.weight) | ||
| ? options.weight | ||
| : [options.weight] | ||
| : []; | ||
| const styles = options.style | ||
| ? Array.isArray(options.style) | ||
| ? options.style | ||
| : [options.style] | ||
| : []; | ||
| const display = options.display ?? "swap"; | ||
|
|
||
| let spec = family.replace(/\s+/g, "+"); | ||
| if (weights.length > 0) { | ||
| const hasItalic = styles.includes("italic"); | ||
| if (hasItalic) { | ||
| const pairs: string[] = []; | ||
| for (const w of weights) { | ||
| pairs.push(`0,${w}`); | ||
| pairs.push(`1,${w}`); | ||
| } | ||
| spec += `:ital,wght@${pairs.join(";")}`; | ||
| } else { | ||
| spec += `:wght@${weights.join(";")}`; | ||
| } | ||
| } else if (styles.length === 0) { | ||
| // Request full variable weight range when no weight specified. | ||
| // Without this, Google Fonts returns only weight 400. | ||
| spec += `:wght@100..900`; | ||
| // Validate the call against the bundled Google Fonts metadata | ||
| // and resolve the actual axis values. This replaces an earlier | ||
| // inline URL builder that hardcoded `:wght@100..900` regardless | ||
| // of the font's real `wght` axis range, which produced HTTP 400 | ||
| // for fonts whose axis is narrower (Sen 400..800, Anton 400). | ||
| // See issue #885. | ||
| let validated; | ||
| try { | ||
| validated = validateGoogleFontOptions(family, options); | ||
| } catch (err) { | ||
| // Validation errors are programmer errors (unknown family, | ||
| // missing required weight on a static font, etc.). Re-throw | ||
| // 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}`); | ||
| } | ||
| const params = new URLSearchParams(); | ||
| params.set("family", spec); | ||
| params.set("display", display); | ||
| const cssUrl = `https://fonts.googleapis.com/css2?${params.toString()}`; | ||
| const axes = getFontAxes( | ||
| family, | ||
| validated.weights, | ||
| validated.styles, | ||
| validated.selectedVariableAxes, | ||
| ); | ||
| const cssUrl = buildGoogleFontsUrl(family, axes, validated.display); | ||
|
|
||
| // Check cache | ||
| let localCSS = fontCache.get(cssUrl); | ||
| if (!localCSS) { | ||
| try { | ||
| localCSS = await fetchAndCacheFont(cssUrl, family, cacheDir); | ||
| fontCache.set(cssUrl, localCSS); | ||
| } catch { | ||
| // Fetch failed (offline?) — fall back to CDN mode | ||
| } catch (err) { | ||
| if (err instanceof GoogleFontsHttpError) { | ||
| // HTTP 4xx/5xx from Google means the URL is malformed or | ||
| // the family/axis combination is invalid. Surface as a | ||
| // build error so the user sees the failing URL plus | ||
| // Google's response body, rather than silently falling | ||
| // 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)}`, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking nit, carry-over from the prior review: the manual |
||
| ); | ||
| } | ||
| // Network errors (offline, DNS, AbortError) are recoverable; | ||
| // skip self-hosting and let the runtime CDN path handle it. | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| import { buildGoogleFontsUrl as buildUrlFromAxes } from "../build/google-fonts/build-url.js"; | ||
|
|
||
| /** | ||
| * next/font/google shim | ||
| * | ||
|
|
@@ -109,15 +111,24 @@ function toVarName(family: string): string { | |
|
|
||
| /** | ||
| * Build a Google Fonts CSS URL. | ||
| * | ||
| * In production this code path is dead. The build plugin | ||
| * (`vinext:google-fonts` in `src/plugins/fonts.ts`) statically resolves | ||
| * each font call's axis values against the bundled metadata, fetches the | ||
| * Google Fonts CSS, and injects the resulting CSS as `_selfHostedCSS` so | ||
| * the runtime never queries Google. The shim only reaches this builder | ||
| * when the plugin's static parser bails (dynamic options, eval-only | ||
| * shapes), which is dev-only. | ||
| * | ||
| * The dev fallback intentionally has no metadata: shipping the 388 KB | ||
| * `font-data.json` to the Worker bundle would dwarf the rest of the shim, | ||
| * and the production path already has the metadata-aware variant. The | ||
| * tradeoff is that the dev fallback cannot resolve a variable font's | ||
| * actual `wght` axis range. It emits no axis segment when no `weight` is | ||
| * given, which makes Google return the default static face (200) instead | ||
| * of the broken `:wght@100..900` URL that issue #885 reports. | ||
| */ | ||
| export function buildGoogleFontsUrl(family: string, options: FontOptions): string { | ||
| const params = new URLSearchParams(); | ||
| // Don't pre-replace spaces with "+". URLSearchParams handles encoding: | ||
| // spaces become "+" in application/x-www-form-urlencoded format. | ||
| // Pre-replacing would cause double-encoding: "+" -> "%2B" (400 error). | ||
| let spec = family; | ||
|
|
||
| // Build weight/style specs | ||
| const weights = options.weight | ||
| ? Array.isArray(options.weight) | ||
| ? options.weight | ||
|
|
@@ -129,33 +140,26 @@ export function buildGoogleFontsUrl(family: string, options: FontOptions): strin | |
| : [options.style] | ||
| : []; | ||
|
|
||
| if (weights.length > 0 || styles.length > 0) { | ||
| const hasItalic = styles.includes("italic"); | ||
| if (weights.length > 0) { | ||
| if (hasItalic) { | ||
| // Use ital axis: ital,wght@0,400;0,700;1,400;1,700 | ||
| const pairs: string[] = []; | ||
| for (const w of weights) { | ||
| pairs.push(`0,${w}`); | ||
| pairs.push(`1,${w}`); | ||
| } | ||
| spec += `:ital,wght@${pairs.join(";")}`; | ||
| } else { | ||
| spec += `:wght@${weights.join(";")}`; | ||
| } | ||
| } | ||
| } else { | ||
| // When no weight is specified, request the full variable weight range. | ||
| // Without this, Google Fonts returns only weight 400 (the default). | ||
| // Next.js loads the full variable font by default, so we match that | ||
| // behavior to ensure all font weights render correctly. | ||
| spec += `:wght@100..900`; | ||
| } | ||
|
|
||
| params.set("family", spec); | ||
| params.set("display", options.display ?? "swap"); | ||
|
|
||
| return `https://fonts.googleapis.com/css2?${params.toString()}`; | ||
| const hasItalic = styles.includes("italic"); | ||
| const hasNormal = styles.includes("normal"); | ||
| // Google treats omitted ital as ital=0, so italic-only requests emit | ||
| // ['1']; mixed requests emit ['0','1']; normal-only stays undefined so | ||
| // the URL has no ital axis at all. | ||
| const ital = hasItalic ? [...(hasNormal ? ["0"] : []), "1"] : undefined; | ||
|
|
||
| // The dev fallback has no metadata, so the variable sentinel cannot be | ||
| // resolved to the font's real axis range here. Drop it like empty options | ||
| // instead of emitting the invalid Google Fonts URL `:wght@variable`. | ||
| const normalizedWeights = weights.length === 1 && weights[0] === "variable" ? [] : weights; | ||
|
|
||
| // Italic-only with no explicit weight still needs a wght value or the | ||
| // ital axis has nowhere to attach in Google's URL grammar. Fall back to | ||
| // '400' because every Google Font has it and it is the visible default. | ||
| // The plugin's metadata-aware path covers the variable-font case in | ||
| // production. | ||
| const wght = normalizedWeights.length > 0 ? normalizedWeights : ital ? ["400"] : undefined; | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Edge case worth a comment or guard: if a caller in dev passes
The production path covers it correctly via |
||
| return buildUrlFromAxes(family, { wght, ital }, options.display ?? "swap"); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Behavioral note for the changelog: pre-port code emitted both |
||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.