perf: async I/O + cache for og-inline-fetch-assets transform by Divkix · Pull Request #435 · cloudflare/vinext

Divkix · 2026-03-11T02:21:43Z

Closes #441

Summary

Converts vinext:og-inline-fetch-assets plugin's transform hook from synchronous fs.readFileSync() to async with fs.promises.readFile(), unblocking the Vite transform pipeline
Adds a per-build Map<string, string> cache so repeated reads of the same asset file (common with shared fonts across modules) hit memory instead of disk
Adds tests/og-inline.test.ts with 7 tests covering guard clause, both transform patterns, file-not-found handling, async assertion, and cache-hit verification

Test plan

pnpm test tests/og-inline.test.ts — 7/7 pass
pnpm test tests/deploy.test.ts tests/build-optimization.test.ts — 263/263 pass
pnpm run lint — 0 warnings, 0 errors
pnpm run typecheck — clean
CI full suite (Vitest + Playwright E2E)

…build cache The `vinext:og-inline-fetch-assets` plugin used synchronous `fs.readFileSync()` in Vite's transform hook, blocking the transform pipeline. This converts both Pattern 1 (fetch inlining) and Pattern 2 (readFileSync inlining) to use `await fs.promises.readFile()` and adds a per-build `Map<string, string>` cache so repeated reads of the same file (common with shared fonts) hit memory instead of disk.

pkg-pr-new · 2026-03-11T02:22:24Z

Open in StackBlitz

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

commit: 55b5278

Copilot

Pull request overview

This PR improves the vinext:og-inline-fetch-assets Vite plugin by making its asset inlining transform non-blocking (async I/O) and adding an in-memory cache to avoid repeated disk reads, with accompanying unit tests to validate behavior and caching.

Changes:

Convert the transform hook to async and use fs.promises.readFile() instead of fs.readFileSync().
Add a Map<string, string> cache for base64-encoded asset contents.
Add tests/og-inline.test.ts covering guard behavior, both transform patterns, missing-file skip, async hook behavior, and cache-hit behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
tests/og-inline.test.ts	Adds unit tests validating async transform, replacement behavior, missing-file handling, and caching.
packages/vinext/src/index.ts	Implements async file reads and introduces a shared cache for inlined OG assets.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-11T02:25:27Z

packages/vinext/src/index.ts

+            let fileBase64 = _ogInlineCache.get(absPath);
+            if (!fileBase64) {
+              try {
+                const buf = await fs.promises.readFile(absPath);
+                fileBase64 = buf.toString("base64");
+                _ogInlineCache.set(absPath, fileBase64);


The cache lookup treats an empty base64 string as a cache miss because it checks if (!fileBase64). If the asset file is empty, this will repeatedly re-read it from disk. Use an undefined check (or Map.has) instead so empty-string values are cached correctly.

Copilot · 2026-03-11T02:25:27Z

packages/vinext/src/index.ts

+            let fileBase64 = _ogInlineCache.get(absPath);
+            if (!fileBase64) {
+              try {
+                const buf = await fs.promises.readFile(absPath);
+                fileBase64 = buf.toString("base64");
+                _ogInlineCache.set(absPath, fileBase64);
+              } catch {


Same cache-miss issue here: if (!fileBase64) will re-read empty files since base64 for an empty file is "". Prefer checking for undefined (or using _ogInlineCache.has(absPath)) to distinguish a cached empty string from a true miss.

Copilot · 2026-03-11T02:25:27Z

packages/vinext/src/index.ts

  // Shim alias map — populated in config(), used by resolveId() for .js variants
  let nextShimMap: Record<string, string> = {};

+  // Per-build cache for og-inline-fetch-assets plugin to avoid repeated file reads


This is labeled as a "Per-build" cache, but it’s currently scoped to the vinext plugin instance and never cleared. In dev or build watch mode this can persist across rebuilds and potentially serve stale inlined asset contents. Consider clearing it on buildStart/configResolved (or adjust the comment/name to reflect the actual lifetime).

Suggested change

// Per-build cache for og-inline-fetch-assets plugin to avoid repeated file reads

// Plugin-instance cache for og-inline-fetch-assets plugin to avoid repeated file reads

Copilot · 2026-03-11T02:25:28Z

tests/og-inline.test.ts

+    const plugin = getOgInlinePlugin();
+    const transform = unwrapHook(plugin.transform);
+    const code = `const buf = fs.readFileSync(fileURLToPath(new URL("./noto-sans.ttf", import.meta.url)));`;
+    const moduleId = path.join(tmpDir, "og.tsx");
+
+    // First call — should read from disk
+    await transform.call(plugin, code, moduleId);
+
+    // Second call — should use cache
+    await transform.call(plugin, code, moduleId);
+
+    // fs.promises.readFile should have been called at most once for this path
+    const calls = readFileSpy.mock.calls.filter(
+      (call) => call[0] === path.join(tmpDir, "noto-sans.ttf"),
+    );
+    expect(calls.length).toBe(1);
+
+    readFileSpy.mockRestore();


The spy is restored at the end of the test, but if an assertion throws earlier the spy will stay active and can leak into later tests. Prefer restoring in a finally block or registering an afterEach(() => vi.restoreAllMocks())/afterEach(() => readFileSpy.mockRestore()) cleanup.

Suggested change

const plugin = getOgInlinePlugin();

const transform = unwrapHook(plugin.transform);

const code = `const buf = fs.readFileSync(fileURLToPath(new URL("./noto-sans.ttf", import.meta.url)));`;

const moduleId = path.join(tmpDir, "og.tsx");

// First call — should read from disk

await transform.call(plugin, code, moduleId);

// Second call — should use cache

await transform.call(plugin, code, moduleId);

// fs.promises.readFile should have been called at most once for this path

const calls = readFileSpy.mock.calls.filter(

(call) => call[0] === path.join(tmpDir, "noto-sans.ttf"),

);

expect(calls.length).toBe(1);

readFileSpy.mockRestore();

try {

const plugin = getOgInlinePlugin();

const transform = unwrapHook(plugin.transform);

const code = `const buf = fs.readFileSync(fileURLToPath(new URL("./noto-sans.ttf", import.meta.url)));`;

const moduleId = path.join(tmpDir, "og.tsx");

// First call — should read from disk

await transform.call(plugin, code, moduleId);

// Second call — should use cache

await transform.call(plugin, code, moduleId);

// fs.promises.readFile should have been called at most once for this path

const calls = readFileSpy.mock.calls.filter(

(call) => call[0] === path.join(tmpDir, "noto-sans.ttf"),

);

expect(calls.length).toBe(1);

} finally {

readFileSpy.mockRestore();

}

…via afterEach Replace falsy `if (!fileBase64)` with `if (fileBase64 === undefined)` in both og-inline patterns to correctly handle hypothetical empty-string cache values. Add `afterEach(() => vi.restoreAllMocks())` to prevent spy leaks on assertion failure, replacing the manual mockRestore() call.

james-elicx · 2026-03-11T07:58:42Z

/bigbonk review

ask-bonk

The async I/O conversion is a good improvement — making the transform non-blocking is the right call. The cache idea is also sound. However, there are a few issues that should be addressed before merging.

Issues

Cache is never invalidated — The _ogInlineCache is scoped to the plugin factory closure and never cleared. In dev mode (where the Vite server runs indefinitely), if a user replaces a font file on disk, they'll get the old cached base64 until they restart the dev server. The comment says "Per-build" but the lifetime is actually per-plugin-instance. This should either be cleared on buildStart (so it's truly per-build), or the comment should be corrected and the trade-off documented. For production builds this is fine, but dev mode staleness is a real concern for OG image development workflows.
Cache test is fragile due to cross-test cache pollution — Each test calls getOgInlinePlugin() which creates a new vinext() instance (and thus a fresh cache). This works today, but the earlier tests (Pattern 1, Pattern 2) also read the same noto-sans.ttf file from their own plugin instances. If this test structure ever changes to share a plugin instance, the cache test would silently pass for the wrong reason. Consider explicitly asserting the spy call count is exactly 1 (not "at most"), and adding a comment explaining why a fresh plugin instance is required.
Unused variable fontPath — It's declared and assigned but never referenced in any assertion. Minor cleanup.

ask-bonk · 2026-03-11T08:00:23Z

packages/vinext/src/index.ts

  let nextShimMap: Record<string, string> = {};

+  // Per-build cache for og-inline-fetch-assets plugin to avoid repeated file reads
+  const _ogInlineCache = new Map<string, string>();


This cache lives for the entire lifetime of the plugin instance, which in dev mode means the entire dev server session. If a developer changes a font file on disk while the dev server is running, the stale cached base64 will be served until restart.

Two options:

Clear the cache on buildStart (making it truly per-build as the comment says)

Skip caching when command === 'serve' (dev mode), and only cache during builds

Option 1 is simpler and still captures the main win (fonts are read once per build, not once per module):

Suggested change

const _ogInlineCache = new Map<string, string>();

// Cache for og-inline-fetch-assets plugin to avoid repeated file reads within a build.

// Cleared on each buildStart so dev-server HMR picks up changed assets.

const _ogInlineCache = new Map<string, string>();

Then add a buildStart hook to the vinext:og-inline-fetch-assets plugin object:

buildStart() { _ogInlineCache.clear(); },

ask-bonk · 2026-03-11T08:00:23Z

tests/og-inline.test.ts

+// ── Test fixture setup ────────────────────────────────────────
+
+let tmpDir: string;
+let fontPath: string;


fontPath is declared and assigned but never referenced in any test assertion — it's only used to write the fixture file in beforeAll. You can inline the path:

Suggested change

let fontPath: string;

let tmpDir: string;

And in beforeAll:

await fsp.writeFile(path.join(tmpDir, "noto-sans.ttf"), fontContent);

ask-bonk · 2026-03-11T08:00:23Z

tests/og-inline.test.ts

+    const calls = readFileSpy.mock.calls.filter(
+      (call) => call[0] === path.join(tmpDir, "noto-sans.ttf"),
+    );
+    expect(calls.length).toBe(1);


The comment says "at most once" but the assertion is toBe(1), which is correct — it should be exactly 1 (first call is a cache miss that reads from disk, second call hits the cache). The comment should match:

Suggested change

expect(calls.length).toBe(1);

expect(calls.length).toBe(1); // Exactly once: first call reads from disk, second hits cache

Also worth noting: this test works because getOgInlinePlugin() creates a fresh vinext() instance (with an empty cache). If the test ever shares a plugin instance with earlier tests, the first call here might also be a cache hit. Consider adding a comment to getOgInlinePlugin() documenting that each call creates an independent plugin instance with its own cache.

ask-bonk · 2026-03-11T08:00:23Z

tests/og-inline.test.ts

+
+  // ── Async assertion ──────────────────────────────────────
+
+  it("returns a Promise (hook is async)", () => {


This test doesn't await the result, so it's an unhandled promise. Vitest won't flag this as a failure today, but the dangling promise can cause flaky behavior (e.g., it might resolve after the test ends and interfere with afterEach cleanup). Since this test just asserts the return type is a Promise, that's fine, but you should still clean it up:

Suggested change

it("returns a Promise (hook is async)", () => {

it("returns a Promise (hook is async)", () => {

const plugin = getOgInlinePlugin();

const transform = unwrapHook(plugin.transform);

const code = `const data = fetch(new URL("./noto-sans.ttf", import.meta.url)).then((res) => res.arrayBuffer());`;

const moduleId = path.join(tmpDir, "og.tsx");

const result = transform.call(plugin, code, moduleId);

expect(result).toBeInstanceOf(Promise);

return result; // Return the promise so Vitest awaits it

ask-bonk · 2026-03-11T08:00:31Z

Review submitted. Here's a summary of the feedback:

Requested changes on PR #435:

Cache invalidation (main issue) — The _ogInlineCache is never cleared, so in dev mode, changed font files on disk won't be picked up until the dev server restarts. Suggested adding a buildStart hook to clear the cache, making it truly per-build as the comment claims.
Dangling promise in async assertion test — The "returns a Promise" test doesn't await or return the promise, which can cause flaky behavior. Suggested returning the promise so Vitest awaits it.
Unused fontPath variable — Declared and assigned but never referenced in assertions. Can be inlined.
Cache test comment accuracy — Comment says "at most once" but assertion is toBe(1) (exactly once). Suggested aligning the comment and documenting the fresh-plugin-instance dependency.

github run

Copilot AI review requested due to automatic review settings March 11, 2026 02:21

Copilot started reviewing on behalf of Divkix March 11, 2026 02:22 View session

Copilot AI reviewed Mar 11, 2026

View reviewed changes

Divkix mentioned this pull request Mar 11, 2026

perf: og-inline-fetch-assets blocks Vite transform pipeline with sync I/O #441

Open

Divkix mentioned this pull request Mar 11, 2026

refactor: consolidate 5 nested ALS scopes into unified request context #450

Draft

6 tasks

ask-bonk bot requested changes Mar 11, 2026

View reviewed changes

	// Per-build cache for og-inline-fetch-assets plugin to avoid repeated file reads
	// Plugin-instance cache for og-inline-fetch-assets plugin to avoid repeated file reads

	expect(calls.length).toBe(1);
	expect(calls.length).toBe(1); // Exactly once: first call reads from disk, second hits cache


		// ── Async assertion ──────────────────────────────────────

		it("returns a Promise (hook is async)", () => {

Conversation

Divkix commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

pkg-pr-new bot commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

james-elicx commented Mar 11, 2026

Uh oh!

ask-bonk bot left a comment

Choose a reason for hiding this comment

Issues

Uh oh!

ask-bonk bot Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk bot Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk bot Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk bot Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk bot commented Mar 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Divkix commented Mar 11, 2026 •

edited

Loading

pkg-pr-new bot commented Mar 11, 2026 •

edited

Loading