fix(transforms): make leaked node: named imports fail at the call site (depends on #2999)

[mattboon]

Summary

Found while fixing bug 2 — not in the reproducer's catalogue, but the same reported failure signature.

The browser noop polyfill for Node built-ins documents its own contract:

If a server-only Node built-in leaks into a browser-served module, this prevents the import from crashing. Any actual usage of the imported API will fail at call-site, which surfaces the problem clearly instead of a cryptic module resolution error.

That contract cannot hold for a named import. ESM resolves named imports at link time, so import { createHash } from "node:crypto" in browser-bound code took down the entire module graph before a line of it ran — and the error named the polyfill rather than the offending import:

SyntaxError: The requested module '/_vf_modules/_veryfront/platform/polyfills/node-noop.js'
does not provide an export named 'createHash'

Named imports of node: builtins are now rewritten to a namespace import plus a destructure, so linking succeeds and the failure lands where the API is actually called.

This also resolves a real source/binary divergence: src/platform/polyfills/node-noop.ts exports { nodeNoop, default } while the compiled-binary copy in module-server.ts exports only default, so identical source could link or fail depending on how the framework was served. Via a namespace import, both behave identically.

Reproduction

• Routes: g-transitive-ts-ext.tsx, app/use-server/page.tsx — components that call node:crypto at render time, rather than inside a data hook
• Test: src/transforms/pipeline/stages/browser-node-builtin-imports.test.ts

Test evidence

ok | 23 passed (204 steps) | 0 failed      # src/transforms/pipeline/

Covers aliases, multiple bindings, a default binding alongside named ones, and the untouched cases (default-only, namespace, side-effect-only, non-node).

Wider suite: deno task test:unit → 2525 passed | 0 failed.

Client evidence

Before — one leaked builtin kills the whole page, blaming the polyfill:

FAIL /test/g-transitive-ts-ext
   pageerror: The requested module '…/node-noop.js' does not provide an export named 'createHash'
FAIL /use-server
FAIL /test-app/d-server-static

After — the module links, and the error names the real API at its real call site:

FAIL /test/g-transitive-ts-ext
   pageerror: createHash is not a function
FAIL /use-server
   pageerror: createHash is not a function
FAIL /test-app/d-server-static
   pageerror: createHash is not a function

These three routes should still fail: they genuinely invoke node:crypto during client render, which no polyfill can make work. The point is that the framework now says so precisely, exactly as the polyfill's documentation promises, instead of emitting a link error against a file the author never wrote.

Related

• Discovered while fixing bug 2 (fix(transforms): strip server-only data hooks from the client artifact (depends on #2999) #3002); shares the reported signature of bugs 1/2/8.

---

Chain: this PR is part of a 13-PR chain fixing the bugs catalogued in
veryfront-router-testing.
Its base is the previous PR in the chain, so the diff shows only this fix.
The root of the chain is #2999 (fix/ssr-lazy-import-graceful-degrade) — merge #2999 first, then
rebase the chain onto main.

Regression gate: deno task test:unit → 2525 passed | 0 failed; deno task lint,
deno task fmt:check and deno task typecheck all clean. The reproducer's full 56-route
matrix (ROUTES.txt + sweep.sh) was re-run after every fix: 7 routes improved, 0 regressed.
A 46-route Chromium hydration sweep (client-sweep.mjs) backs the client-side claims.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0e15e161a0

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Match minified import syntax before skipping

In production browser transforms, compilePlugin runs esbuild with minify: !ctx.dev, and esbuild emits imports without spaces such as import{createHash as h}from"node:crypto";. This exact " from " lookup returns -1 for that valid static named import, so the new stage skips it as if it were side-effect-only; resolveImportsPlugin then still points the named import at node-noop.js, preserving the link-time does not provide an export named failure for production builds even though dev builds are fixed.

Useful? React with 👍 / 👎.

[kwakayama]

Score: 78/100

Requesting changes before this lands. The approach is reasonable for moving node-builtin failures from link time to call site, but the transform needs one more hardening pass.

Blocking concern:

• src/transforms/pipeline/stages/browser-node-builtin-imports.ts generates names like __vf_node_builtin_0 without checking existing user bindings. A source file that already declares/imports that name can be transformed into duplicate or conflicting bindings.

Please generate a non-conflicting namespace identifier by scanning existing identifiers/import bindings, and add a regression where __vf_node_builtin_0 already exists in the module.

Also note this PR is stacked on lower open PRs and currently only shows CLA in the status rollup, so it is not merge-ready yet regardless.

[mattboon]

Addressed. The namespace identifier is now collision-free.

pickNamespacePrefix(code) starts from __vf_node_builtin_ and appends _ while the prefix occurs anywhere in the module source, then every generated name is built from the result. Since all generated names share the prefix, a prefix that appears nowhere in the source cannot produce a name that collides with any existing identifier, import binding or alias. It over-matches slightly (a mention inside a string or comment also forces a step), which is safe in the only direction that matters, and it needs no binding analysis of post-esbuild output. Behaviour is unchanged when there is no collision.

Regressions added:

• module already declaring const __vf_node_builtin_0
• module already importing a binding named __vf_node_builtin_0
• module aliasing into that name (import { x as __vf_node_builtin_0 })
• a chained collision that forces two steps
• the collision case in minified source
• unique numbering across two rewritten imports after renaming

Each asserts the destructure is still correct and that the user's own name is bound exactly once.

Separately, this commit also fixes a review finding that made the original transform dev-only: the import clause is now read from the lexer's offsets instead of searching for " from ", which does not exist in import{createHash as h}from"node:crypto", i.e. in any minified build.

Verification: 24 steps passing, deno check clean on the test file, deno fmt/deno lint clean.

[kwakayama]

Follow-up after fixes: approving. Namespace generation now avoids collisions, including minified inputs. Local verification: \running 1 test from ./src/transforms/pipeline/stages/browser-node-builtin-imports.test.ts
browser-node-builtin-imports ...
converts a named import into a namespace import plus destructure ... ok (2ms)
preserves aliases ... ok (0ms)
handles multiple bindings ... ok (0ms)
keeps a default binding alongside the namespace ... ok (0ms)
leaves a default-only import alone ... ok (0ms)
leaves a namespace import alone ... ok (0ms)
leaves a side-effect-only import alone ... ok (0ms)
leaves non-node imports alone ... ok (0ms)
numbers each rewritten import uniquely ... ok (0ms)
minified input ...
rewrites a minified named import ... ok (0ms)
rewrites a minified import with a default binding ... ok (0ms)
leaves a minified side-effect-only import alone ... ok (0ms)
leaves a minified namespace import alone ... ok (0ms)
rewrites both imports in a minified module ... ok (0ms)
handles a single-quoted specifier ... ok (0ms)
minified input ... ok (1ms)
name collisions ...
avoids a name the module already declares ... ok (0ms)
avoids a name the module already imports ... ok (0ms)
avoids a name aliased in by another import ... ok (0ms)
keeps stepping away until the name is free ... ok (0ms)
avoids a collision in minified source ... ok (0ms)
keeps the numbering unique across imports once renamed ... ok (0ms)
name collisions ... ok (1ms)
only runs for the browser target ... ok (0ms)
browser-node-builtin-imports ... ok (6ms)

ok | 1 passed (24 steps) | 0 failed (9ms). Score: 93/100. Next step: merge after base stack and checks are green.

[kwakayama]

Clean follow-up after the approval above:

Score: 93/100.

Verification:

• deno test --allow-all src/transforms/pipeline/stages/browser-node-builtin-imports.test.ts

Next step: merge after the base stack and refreshed checks are green.

The base branch was changed.

[kwakayama]

Follow-up approval after #3003 merged and #3004 was rebased onto main.

Score: 92/100.

Rationale: the transform keeps browser node-builtin named imports from failing at module-link time, pushing failure to the call site while preserving existing coverage after the rebase.

Verification:

• deno task audit
• combined targeted regression suite: 21 tests, 334 steps, 0 failed

Next step: wait for refreshed GitHub checks and required reviewer gate, then merge when green.