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

Open
Divkix wants to merge 5 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 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
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
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>
);
}
6 changes: 6 additions & 0 deletions tests/fixtures/font-google-multiple/vite.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import { defineConfig } from "vite";
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.

This config file is unused — the build test passes configFile: false and constructs the vinext plugin manually. Either:

  1. Remove this file (the test doesn't use it), or
  2. Use it in the test (pass configFile: path.join(APP_FIXTURE_DIR, 'vite.config.ts') instead of configFile: false)

Also, the fixture is missing a package.json. Every other fixture in tests/fixtures/ has one.

import vinext from "vinext";

export default defineConfig({
plugins: [vinext({ appDir: import.meta.dirname })],
});
92 changes: 92 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,92 @@
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";
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.

The vite import should be vite-plus per the repo conventions (see AGENTS.md: "all modules should be imported from the project's vite-plus dependency"):

Suggested change
import { createBuilder } from "vite";
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");

/**
* Build an App Router fixture's RSC/SSR/client bundles using the actual Vite
* build pipeline (createBuilder + buildApp). This exercises the full build
* pipeline for font-google transforms.
*/
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");

// Mock fetch before building to avoid network calls
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 {
// Symlink node_modules before building so imports work
const projectNodeModules = path.resolve(import.meta.dirname, "../node_modules");
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.mjs");
} finally {
globalThis.fetch = originalFetch;
// Cleanup symlink
await fs.unlink(nodeModulesLink).catch(() => {});
}
}

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

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

it("should build successfully with multiple Google fonts (Geist + Geist_Mono)", async () => {
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.

The comment claims this reproduces issue #751, but it doesn't — this test passes on main without the one-line fix. Issue #751's root cause (double-comma injection with trailing commas) was already fixed by #719. The remaining report in #751 (DenyVeyten's comment about missing named exports) is a separate issue.

Please update or remove this comment to avoid misleading future readers.

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.

The comment on lines 81-83 implies this test validates the fix, but it passes without the one-line change. Please either:

// This test validates that the build pipeline can handle multiple
// Google font imports without errors. It exercises the font transform
// plugin during the full createBuilder + buildApp() flow.
buildOutputPath = await buildFontGoogleMultipleFixture();
outDir = path.dirname(path.dirname(buildOutputPath));

// Verify the build produced output
expect(buildOutputPath).toBeTruthy();
const stats = await fs.stat(buildOutputPath);
expect(stats.isFile()).toBe(true);
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 assertion only checks that the output file exists. After running a full buildApp() (which takes up to 2 minutes), the test should verify something meaningful about the build output — for example, that both fonts were transformed correctly:

Suggested change
expect(stats.isFile()).toBe(true);
const content = await fs.readFile(buildOutputPath, 'utf-8');
expect(content).toContain('Geist');

As-is, fs.stat().isFile() would pass even if the font transform was completely broken and the fonts were silently dropped.

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.

This assertion is too weak to justify a 2-minute full build. fs.stat().isFile() passes even if the font transform is completely broken. At minimum, read the output file and verify both fonts were transformed:

Suggested change
expect(stats.isFile()).toBe(true);
const content = await fs.readFile(buildOutputPath, 'utf-8');
expect(content).toContain('Geist');
expect(content).toContain('_selfHostedCSS');

}, 120000); // 2 minute timeout for full build
});
Loading