Skip to content

fix(plugins): track font call ranges during transform #785

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.

Sign up for GitHub

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

Merged
james-elicx merged 8 commits into from
Apr 9, 2026
Merged

fix(plugins): track font call ranges during transform #785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c92de34
fix(plugins): track font call ranges during transform
Divkix Apr 5, 2026
0e44a62
test(fonts): remove duplicate test and add E2E build test for #751
Divkix Apr 5, 2026
ed6295c
style(fonts): fix lint and formatting issues in #751 test
Divkix Apr 5, 2026
73858a1
style: fix formatting in font-google-multiple layout
Divkix Apr 5, 2026
0676075
refactor(tests): move font-google-multiple to standalone fixture and …
Divkix Apr 5, 2026
68883af
fix(tests): address PR review feedback on font transform build test
Divkix Apr 8, 2026
65bb0de
chore: update pnpm-lock.yaml for font-google-multiple fixture
Divkix Apr 8, 2026
c636958
fix(tests): handle pre-existing node_modules and correct build output…
Divkix Apr 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/vinext/src/plugins/fonts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -729,6 +729,7 @@ export function createGoogleFontsPlugin(fontGoogleShimPath: string, shimsDir: st

const replacement = `${calleeSource}(${optionsWithCSS})`;
s.overwrite(callStart, callEnd, replacement);
overwrittenRanges.push([callStart, callEnd]);
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is a reasonable defensive improvement — it ensures overwrittenRanges is consistent after injectSelfHostedCss runs, preventing theoretical double-overwrite of overlapping ranges.

However, it does not fix issue #751. I verified by reverting this line and running the new test — it still passes. The two font calls (Geist(...) and Geist_Mono(...)) occupy non-overlapping character ranges in the original source, so the overlap check at line 757 was never triggered.

The actual error from #751 (Unexpected token in app/layout.tsx at 234..235) likely originates from a different part of the build pipeline. To find the real cause, someone needs to reproduce the build failure with the reproduction repo and trace the actual error through the Vite build.

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a correct fix. Every other s.overwrite() call in the transform hook (lines 594-599, 610-614, 640-645) pushes to overwrittenRanges, and this was the only one that didn't. Without this, a pathological case where namedCallRe and memberCallRe both match overlapping ranges (unlikely but theoretically possible with unusual code patterns) could double-overwrite.

That said, for the specific scenario described in the PR (two non-overlapping Geist/Geist_Mono calls), this line has no effect — both calls are found independently by namedCallRe, and their ranges never overlap.

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct fix. This was the only s.overwrite() in the transform hook that didn't track its range in overwrittenRanges. It ensures the overlap guard at line 757 and 786 will correctly skip ranges already processed by injectSelfHostedCss.

Note: for the specific Geist/Geist_Mono scenario from #751, this has no effect (the two calls have non-overlapping ranges), but it prevents a theoretical double-overwrite if namedCallRe and memberCallRe ever matched overlapping spans.

hasChanges = true;
}

Expand Down
25 changes: 25 additions & 0 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 28 additions & 0 deletions tests/fixtures/font-google-multiple/app/layout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import type { Metadata } from "next";
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a nested layout with next/font/google imports inside app-basic means every test that builds the app-basic fixture will now also process this file's font imports. This could cause unexpected side effects (e.g., tests that mock fetch differently, or tests that assert on the total number of routes/layouts).

Consider either:

  1. Making this a standalone minimal fixture (e.g., tests/fixtures/font-google-multiple/) with just a root layout and page
  2. Converting this to a page without its own layout, so the font imports don't add a layout nesting level

import { Geist, Geist_Mono } from "next/font/google";

const geistSans = Geist({
variable: "--font-geist-sans",
subsets: ["latin"],
});

const geistMono = Geist_Mono({
variable: "--font-geist-mono",
subsets: ["latin"],
});

export const metadata: Metadata = {
title: "Font Google Multiple Test",
};

export default function RootLayout({
children,
}: Readonly<{
children: React.ReactNode;
}>) {
return (
<html lang="en" className={`${geistSans.variable} ${geistMono.variable} h-full antialiased`}>
<body className="min-h-full flex flex-col">{children}</body>
</html>
);
}
8 changes: 8 additions & 0 deletions tests/fixtures/font-google-multiple/app/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
export default function HomePage() {
return (
<main>
<h1>Font Google Multiple Test</h1>
<p>This page tests multiple Google Fonts (Geist + Geist_Mono).</p>
</main>
);
}
16 changes: 16 additions & 0 deletions tests/fixtures/font-google-multiple/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"name": "font-google-multiple-fixture",
"private": true,
"type": "module",
"dependencies": {
"@vitejs/plugin-rsc": "catalog:",
"react": "catalog:",
"react-dom": "catalog:",
"react-server-dom-webpack": "catalog:",
"vinext": "workspace:*",
"vite": "catalog:"
},
"devDependencies": {
"vite-plus": "catalog:"
}
}
80 changes: 80 additions & 0 deletions tests/font-google-build.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
import { describe, it, expect, afterAll } from "vite-plus/test";
import path from "node:path";
import fs from "node:fs/promises";
import os from "node:os";
import { createBuilder } from "vite-plus";
import vinext from "../packages/vinext/src/index.js";

const APP_FIXTURE_DIR = path.resolve(import.meta.dirname, "./fixtures/font-google-multiple");

async function buildFontGoogleMultipleFixture(): Promise<string> {
const outDir = await fs.mkdtemp(path.join(os.tmpdir(), "vinext-font-google-multiple-"));

const rscOutDir = path.join(outDir, "server");
const ssrOutDir = path.join(outDir, "server", "ssr");
const clientOutDir = path.join(outDir, "client");

const originalFetch = globalThis.fetch;
globalThis.fetch = async (input: any) => {
const url = String(input);
if (url.includes("Geist") && !url.includes("Mono")) {
return new Response("@font-face { font-family: 'Geist'; src: url(/geist.woff2); }", {
status: 200,
headers: { "content-type": "text/css" },
});
}
return new Response("@font-face { font-family: 'Geist Mono'; src: url(/geist-mono.woff2); }", {
status: 200,
headers: { "content-type": "text/css" },
});
};

const nodeModulesLink = path.join(APP_FIXTURE_DIR, "node_modules");

try {
const projectNodeModules = path.resolve(import.meta.dirname, "../node_modules");
await fs.rm(nodeModulesLink, { recursive: true, force: true });
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: since this fixture is registered in pnpm-lock.yaml, pnpm install will create a real node_modules here (with workspace links). The fs.rm + fs.symlink will blow that away and replace it with a flat symlink to the root node_modules. This works, but it means running the test locally after pnpm install will destroy the pnpm-managed node_modules for this fixture (the finally block only calls fs.unlink, which removes the symlink).

Consider either:

  1. Removing the fixture from pnpm-lock.yaml (don't register it as a workspace package) so pnpm doesn't create node_modules there, or
  2. Using a fully temp-dir approach like intercepting-routes-build.test.ts does, which avoids mutating the fixture directory entirely.

await fs.symlink(projectNodeModules, nodeModulesLink);

const builder = await createBuilder({
root: APP_FIXTURE_DIR,
configFile: false,
plugins: [
vinext({
appDir: APP_FIXTURE_DIR,
rscOutDir,
ssrOutDir,
clientOutDir,
}),
],
logLevel: "silent",
});

await builder.buildApp();

return path.join(outDir, "server", "index.js");
} finally {
globalThis.fetch = originalFetch;
await fs.unlink(nodeModulesLink).catch(() => {});
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: if the fixture already has a real node_modules (e.g., from pnpm install — the lockfile does register this fixture), fs.unlink on a directory symlink works on Linux but can behave differently on Windows. The intercepting-routes-build.test.ts avoids this by using an entirely temp directory. Not blocking since CI is Linux-only, but worth noting if Windows CI is ever added.

}
}

describe("font-google build integration", () => {
let buildOutputPath: string;
let outDir: string;

afterAll(async () => {
if (outDir) {
await fs.rm(outDir, { recursive: true, force: true });
}
});

it("should build and transform multiple Google fonts (Geist + Geist_Mono)", async () => {
buildOutputPath = await buildFontGoogleMultipleFixture();
outDir = path.dirname(path.dirname(buildOutputPath));

const content = await fs.readFile(buildOutputPath, "utf-8");
expect(content).toContain("Geist");
expect(content).toContain("_selfHostedCSS");
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: toContain("Geist") will match both Geist and Geist_Mono / Geist Mono, so it doesn't prove both fonts were independently transformed. Consider asserting on both CSS payloads to distinguish them:

Suggested change
expect(content).toContain("_selfHostedCSS");
expect(content).toContain("Geist");
expect(content).toContain("Geist Mono");
expect(content).toContain("_selfHostedCSS");

Alternatively, you could count _selfHostedCSS occurrences (like the unit test at font-google.test.ts:476 does) to prove both calls were transformed independently.

}, 120000);
});
Loading