fix(google-fonts): use closure variables instead of this for plugin sstate

[MehediH]

Vite does not bind this to the plugin object when calling hooks — it extracts the hook function and calls it directly. This means this._isBuild etc. in configResolved and transform.handler refers to undefined in strict mode, crashing the dev server with:

TypeError: Cannot set properties of undefined (setting '_isBuild')

Fix: wrap the plugin in an IIFE and hold state in closure variables instead of as properties on the plugin object.

[southpolesteve]

/bigbonk review

[ask-bonk]

Review: fix(google-fonts): use closure variables instead of this for plugin state

The fix is correct. Vite extracts hook handler functions from plugin objects and calls them with a Rollup-compatible context object as this — not the plugin object itself. Using (this as any)._isBuild etc. would indeed reference undefined in strict mode. Wrapping the plugin in an IIFE and using closure variables is the standard and idiomatic pattern for Vite plugin state.

Two things to address:

1. Inconsistent indentation — the returned object body is indented at 6 spaces (matching the old top-level position), but being inside the IIFE it should be at 8 spaces. See inline comment.

2. The vinext:image-imports plugin has the same bug — it uses (this as any)._dimCache inside its load hook at line 2592. The load hook also receives a Rollup LoadPluginContext as this, not the plugin object. This means (this as any)._dimCache is always undefined, the cache.get() call at line 2593 always returns undefined, and the cache.set() at line 2600 writes to a throwaway object. Every image dimension is re-read from disk on every load call — the cache never works.

Since it's the identical pattern, please fix vinext:image-imports in the same PR. Specifically, the plugin at lines 2570-2668 should get the same IIFE + closure-variable treatment:

(() => {
  const dimCache = new Map<string, { width: number; height: number }>();
  return {
    name: "vinext:image-imports",
    // ... use dimCache directly instead of (this as any)._dimCache
  } satisfies Plugin;
})()

[ask-bonk]

Review posted successfully. Here's a summary of the review:

The fix is correct — Vite does not bind this to the plugin object when calling hooks. The IIFE + closure variable pattern is the idiomatic solution.

Two items flagged:

1. Minor indentation issue — the returned object inside the IIFE has properties at 6-space indent (carried over from the old structure) instead of the expected 8-space indent. Posted an inline suggestion.

2. Same bug exists in vinext:image-imports — the plugin at lines 2570-2668 uses (this as any)._dimCache inside its load hook, which has the same this binding issue. The dimension cache never actually works because this._dimCache resolves to undefined on the Rollup context object. Recommended fixing it in the same PR using the same IIFE pattern.

github run

[southpolesteve]

Review: fix(google-fonts): use closure variables instead of this for plugin state

The fix itself is correct

The IIFE + closure variable pattern is the right approach. Vite extracts hook functions from plugin objects and calls them with a Rollup-compatible context as this, not the plugin object. The closure approach is idiomatic and avoids the issue entirely. The satisfies Plugin type assertion is a nice improvement over the old as Plugin & { ... } cast.

Blocking: Tests are broken

The test file tests/font-google.test.ts was not updated, and multiple tests will now fail or silently test the wrong thing.

The getGoogleFontsPlugin() helper (line 15) returns the plugin with a type assertion claiming _isBuild, _fontCache, and _cacheDir exist. After this PR, those properties are gone (they are closure variables). The as any cast hides this at compile time, but at runtime:

1. Tests that set plugin._isBuild = true silently test nothing. Setting a property on the plugin object has no effect because the transform hook reads the closure variable isBuild, which stays false. The transform returns null on the if (!isBuild) return null check, and the test passes for the wrong reason. This affects:

   • "returns null for files without next/font/google imports" (line 287)
   • "returns null for node_modules files" (line 297)
   • "returns null for virtual modules" (line 306)
   • "returns null for non-script files" (line 315)
   • "returns null when import exists but no font constructor call" (line 324)

2. Tests that access plugin._fontCache will throw TypeError. plugin._fontCache is undefined, so .clear() and .set() crash. This affects:

   • "transforms font call to include _selfHostedCSS during build" (line 334)
   • "uses cached fonts on second call" (line 368)
   • "handles multiple font imports in one file" (line 396)
   • "skips font calls not from the import" (line 429)
   • "fetches Inter font CSS and downloads woff2 files" (line 468)

The tests need to be rewritten to work with the closure-based approach. The standard pattern is to call configResolved with a mock config to set isBuild, and for the font cache, either:

• Export the IIFE factory so tests can access the closure variables (not great)
• Call configResolved({ command: "build", root: "/tmp/test" }) on the plugin to set the closure state, then rely on the transform's own caching (the fontCache Map persists across calls within the same plugin instance)
• Pre-populate the filesystem cache instead of the in-memory cache

The simplest fix: call plugin.configResolved({ command: "build", root: testDir } as any) to set isBuild and cacheDir via the hook, and for fontCache, accept that you can't pre-populate it from outside. Tests that need a pre-populated cache should either make a real fetch (like the existing network test) or mock fetchAndCacheFont.

Correction on previous review: vinext:image-imports does NOT have this bug

The previous review flagged vinext:image-imports as having the same this binding issue. That's incorrect. While the plugin declares _dimCache: imageImportDimCache on the plugin object (line 2760), the load hook on line 2777 uses const cache = imageImportDimCache (the module-level variable), not this._dimCache. The _dimCache property is dead code but harmless. The cache works correctly because it references the module-level variable directly.

That said, the dead _dimCache property and the as Plugin & { _dimCache: ... } cast on line 2853 should be cleaned up in a separate PR.

Summary

• Blocking: Tests must be updated. Multiple tests will either silently pass for the wrong reason or throw TypeErrors.
• Non-blocking (pre-existing): The _dimCache property on vinext:image-imports is dead code. Not introduced by this PR, should be cleaned up separately.

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/vinext@53

commit: 41ddc05

[james-elicx]

/bigbonk review

[ask-bonk]

Review: fix(google-fonts): use closure variables instead of this for plugin state

The fix is correct and all CI checks are green. The IIFE + closure variable pattern is the idiomatic way to hold state in Vite plugins, and the satisfies Plugin type assertion is a clean improvement over the old as Plugin & { ... } cast.

The test updates are well done — the new initPlugin() helper correctly drives plugin state through the configResolved hook (the same path Vite uses), and the fetch mocking with try/finally cleanup is robust. The cache-hit test now properly verifies the in-memory fontCache Map works by checking fetch call count across two transform calls.

Two minor nits below, neither blocking.

[ask-bonk]

Nit: This comment is stale. The test doesn't pre-populate the on-disk cache — it mocks globalThis.fetch instead. The fakeCSS variable is only used as the mock response body, not written to disk.

Suggested change

	const fakeCSS = "@font-face { font-family: 'Inter'; src: url(/fake.woff2); }";
	const fakeCSS = "@font-face { font-family: 'Inter'; src: url(/fake.woff2); }";

(i.e., just remove lines 400-405, or replace with a comment like // Mock fetch to return controlled CSS and track call count)

[ask-bonk]

Nit (alternative to above): replace the stale block comment with something accurate:

Suggested change

	// Pre-populate the on-disk cache so fetchAndCacheFont finds it
	const fakeCSS = "@font-face { font-family: 'Inter'; src: url(/fake.woff2); }";
	plugin._fontCache.set(
	"https://fonts.googleapis.com/css2?family=Inter%3Awght%40400&display=swap",
	fakeCSS,
	);
	const transform = unwrapHook(plugin.transform);
	const code = [
	`import { Inter } from 'next/font/google';`,
	`const inter = Inter({ weight: '400' });`,
	].join("\n");
	const result = await transform.call(plugin, code, "/app/layout.tsx");
	expect(result).not.toBeNull();
	expect(result.code).toContain("virtual:vinext-google-fonts?");
	expect(result.code).toContain("_selfHostedCSS");
	// lgtm[js/incomplete-sanitization] — escaping quotes for test assertion, not sanitization
	expect(result.code).toContain(fakeCSS.replace(/"/g, '\\"'));
	plugin._fontCache.clear();
	// The plugin hashes the URL to create the dir name. Instead, call
	// transform twice: first with a real fetch to populate the in-memory
	// cache, then again to verify the cache is used (no second fetch).
	// Simpler approach: mock fetch to return controlled CSS.
	// Mock fetch to return controlled CSS and track call count.
	// First transform populates the in-memory fontCache; second transform
	// should hit the cache without calling fetch again.
	const fakeCSS = "@font-face { font-family: 'Inter'; src: url(/fake.woff2); }";
	// The plugin hashes the URL to create the dir name. Instead, call
	// transform twice: first with a real fetch to populate the in-memory
	// cache, then again to verify the cache is used (no second fetch).
	// Simpler approach: mock fetch to return controlled CSS.

[ask-bonk]

Review posted successfully. Here's a summary:

Approved. The fix is correct — Vite does not bind this to the plugin object when calling hooks, and the IIFE + closure variable pattern is the idiomatic solution. All CI checks pass.

The test rewrite is solid:

• initPlugin() helper drives state through configResolved (same code path Vite uses)
• fetch mocking with try/finally cleanup is robust
• The cache-hit test properly verifies the in-memory fontCache by tracking fetch call count across two transform calls

One minor nit flagged: a stale comment in the "uses cached fonts on second call" test that says "Pre-populate the on-disk cache" when the test actually mocks globalThis.fetch instead. Non-blocking.

github run

stale