fix: add next shim js variants by hyoban · Pull Request #482 · cloudflare/vinext

hyoban · 2026-03-12T02:11:14Z

It looks like resolveId does not work for optimizeDeps

pkg-pr-new · 2026-03-12T02:11:54Z

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

commit: dafae17

james-elicx · 2026-03-12T07:48:31Z

/bigbonk review

ask-bonk

Good diagnosis and solid fix. Moving .js variant handling from resolve.alias (in the shim map) instead of resolveId is the right approach — optimizeDeps pre-bundling uses alias resolution before plugins' resolveId hooks run, so the old resolveId-based .js stripping never kicked in for dep scanning. The removal of ssr.noExternal: ["nuqs"] from the fixture is a nice UX win — users shouldn't need that workaround.

A few notes below. The main concern is the fragility of the new test assertion.

Stale comment (not in diff): The resolveId filter comment at line ~1455-1457 still references "next/router.js" as an example of what the filter matches, but .js variant handling has been moved out of resolveId. That handler no longer does anything with .js suffixes. Worth updating the comment to avoid confusion (minor, non-blocking).

ask-bonk · 2026-03-12T07:51:46Z

+    const optimizedAdapter = readFileSync(
+      path.join(fixtureRoot, "node_modules", ".vite", "deps", "nuqs_adapters_next_app.js"),
+      "utf8",
+    );
+
+    expect(optimizedAdapter).toContain("packages/vinext/dist/shims/navigation.js");


This assertion reads a Vite-internal pre-bundled file and checks for a monorepo-specific path substring. Two fragility concerns:

Dep filename: nuqs_adapters_next_app.js is derived from Vite's dep flattening heuristic. If Vite changes the naming scheme (or nuqs restructures its exports), this path breaks.

Path substring: "packages/vinext/dist/shims/navigation.js" is specific to the monorepo workspace layout where vinext is a linked workspace package. If vinext is installed from npm (or the monorepo structure changes), the absolute path embedded in the pre-bundled file will differ.

The negative assertion on line 239 (not.toContain("node_modules/.pnpm/next@")) is actually the more important and more robust check — it proves real Next.js was excluded from the pre-bundle. Consider either:

Making the positive assertion use a regex like /shims\/navigation\.js/ (matches regardless of absolute path prefix)

Or dropping line 238 entirely and relying on the negative assertion, which is the core invariant

ask-bonk · 2026-03-12T07:51:46Z

          "next/headers": path.join(shimsDir, "headers"),
+          "next/headers.js": path.join(shimsDir, "headers"),
          "next/font/google": path.join(shimsDir, "font-google"),
+          "next/font/google.js": path.join(shimsDir, "font-google"),


Minor: next/font/google.js and next/font/local.js don't actually exist as files in the next package (font resolution uses package.json exports maps, not file-backed .js files). Including them is harmless and defensive, but the comment on line 956 says "file-backed in next/package.json" which doesn't apply to these two. Non-blocking — just noting the inaccuracy.

ask-bonk · 2026-03-12T07:51:46Z

+        // Build the shim alias map. Exact `.js` variants are included for the
+        // public Next entrypoints that are file-backed in `next/package.json`.
+        // Some libraries (for example `nuqs`) import `next/navigation.js`
+        // directly; aliasing the `.js` form ensures optimizeDeps pre-bundles
+        // vinext's shim instead of real Next.


Nice — the updated comment is much clearer about the why (optimizeDeps pre-bundling) rather than just the what.

ask-bonk · 2026-03-12T07:51:58Z

Review posted successfully. Here's a summary of the feedback I left on PR #482:

Overall: The fix is correct. Moving .js variant handling from resolveId into resolve.alias (the shim map) is the right approach because optimizeDeps pre-bundling uses alias resolution before resolveId hooks run. The old approach never kicked in for dep scanning, which is why libraries like nuqs that import next/navigation.js were getting the real Next.js module instead of vinext's shim.

Comments posted:

Test fragility (main concern): The new test reads a Vite-internal pre-bundled file with a hardcoded path and asserts on a monorepo-specific path substring (packages/vinext/dist/shims/navigation.js). Both the dep filename and the path substring are fragile. The negative assertion (not.toContain("node_modules/.pnpm/next@")) is the more robust and important check.
Stale resolveId comment: The filter comment still references "next/router.js" as an example match, but .js handling has been removed from that handler.
Minor comment inaccuracy: next/font/google.js and next/font/local.js don't actually exist as files in the next package (they use exports maps), so the "file-backed" description in the comment doesn't apply to them. Harmless but slightly inaccurate.
Positive note: The updated comment explaining the why (optimizeDeps pre-bundling) is much clearer than the old one.

github run

hyoban · 2026-03-12T07:58:08Z

Updated

james-elicx · 2026-03-12T08:15:35Z

/bigbonk re-review

ask-bonk

Good fix. Moving .js variant handling from resolveId into the alias map is the correct approach since optimizeDeps pre-bundling resolves aliases before plugin hooks run. The removal of the ssr.noExternal: ["nuqs"] workaround confirms the fix works at the right layer.

A few minor notes below — none are blocking.

Stale resolveId filter comment (not in diff, so can't inline): Lines 1453-1455 still reference "next/router.js" as an example of what the filter matches, but .js variant handling has been moved out of resolveId. The handler no longer processes .js suffixes. Consider updating to:

// Hook filter: only invoke JS for next/* imports and virtual:vinext-* modules.
// Matches "next/navigation", "virtual:vinext-rsc-entry",
// and \0-prefixed re-imports from @vitejs/plugin-rsc.

ask-bonk · 2026-03-12T08:17:20Z

+        // Build the shim alias map. Exact `.js` variants are included for the
+        // public Next entrypoints that are file-backed in `next/package.json`.


Nit: the comment says "file-backed in next/package.json" but that's not accurate for all entries. next/config.js and next/amp.js don't exist on disk (no top-level .js files). next/font/google and next/font/local resolve through a directory index.js, not google.js/local.js.

The .js aliases for entries that don't have a real .js file are harmless (defensive), but the comment could be simpler:

Suggested change

// Build the shim alias map. Exact `.js` variants are included for the

// public Next entrypoints that are file-backed in `next/package.json`.

// Build the shim alias map. Exact `.js` variants are included because

// some libraries (for example `nuqs`) import `next/navigation.js`

ask-bonk · 2026-03-12T08:17:20Z

+
+    const optimizedAdapter = readFileSync(path.join(depsDir, optimizedAdapterFile!), "utf8");
+
+    expect(optimizedAdapter).toMatch(/shims\/navigation\.js/);


Good use of regex instead of a full path substring — this is resilient to different install layouts (monorepo workspace link vs npm install). Together with the negative assertion on line 244 this covers the invariant well.

ask-bonk · 2026-03-12T08:17:28Z

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

Verdict: Approved

The fix is correct. optimizeDeps pre-bundling uses resolve.alias before resolveId hooks run, so the old resolveId-based .js stripping never worked for dep scanning. Moving the .js variants into the alias map is the right fix.

Comments posted:

Comment on shim map comment (line 956-957): The "file-backed in next/package.json" description isn't accurate for all entries — next/config.js and next/amp.js don't exist on disk, and next/font/google/local resolve through directory index.js. Suggested simplifying the comment. (Non-blocking)
Stale resolveId filter comment (top-level note): Lines 1453-1455 still reference "next/router.js" as an example match, but .js handling has been removed from that hook. Suggested updating. (Non-blocking, couldn't inline since it's outside the diff)
Positive note on test: The regex-based assertion (/shims\/navigation\.js/) combined with the negative assertion is a good approach — resilient to different install layouts.

github run

james-elicx · 2026-03-12T08:18:23Z

Thanks, I was going to do this today so you've saved me some time!

fix: add next shim js variants

32bb45c

hyoban marked this pull request as draft March 12, 2026 02:36

hyoban added 3 commits March 12, 2026 10:37

Update

3f7439e

Update

a9dab6d

trigger ci

edace19

hyoban marked this pull request as ready for review March 12, 2026 02:51

hyoban added 2 commits March 12, 2026 10:54

trigger ci

926ae60

Update

6b2bdec

hyoban marked this pull request as draft March 12, 2026 03:03

hyoban added 2 commits March 12, 2026 11:11

Update

51954d3

Update

e73958e

hyoban marked this pull request as ready for review March 12, 2026 03:14

hyoban mentioned this pull request Mar 12, 2026

chore: update vinext langgenius/dify#33347

Merged

5 tasks

ask-bonk Bot reviewed Mar 12, 2026

View reviewed changes

Update

59b9a62

Update

eb5a506

ask-bonk Bot approved these changes Mar 12, 2026

View reviewed changes

james-elicx merged commit 3fc76e4 into cloudflare:main Mar 12, 2026
19 checks passed

		// Build the shim alias map. Exact `.js` variants are included for the
		// public Next entrypoints that are file-backed in `next/package.json`.


		const optimizedAdapter = readFileSync(path.join(depsDir, optimizedAdapterFile!), "utf8");

		expect(optimizedAdapter).toMatch(/shims\/navigation\.js/);

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

james-elicx commented Mar 12, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot commented Mar 12, 2026

Uh oh!

hyoban commented Mar 12, 2026

Uh oh!

james-elicx commented Mar 12, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot commented Mar 12, 2026

Uh oh!

james-elicx commented Mar 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hyoban commented Mar 12, 2026 •

edited

Loading

pkg-pr-new Bot commented Mar 12, 2026 •

edited

Loading