fix(server): support Node production entry contracts

[james-elicx]

Summary

• constant-fold unbound String.fromCharCode(<literal code units>) inside CommonJS requests so statically encoded imports remain bundleable
• provide an entry-relative globalThis.require before evaluating Node production server entries
• pass process.env to Worker-style handlers run by the Node production server
• cover the encoded require through the real app-basic production fixture, plus focused entry-loader and Worker-handler regressions

Downstream patch: nodejs/nodejs.org@30ca201#diff-f89185437eff5248b9d539ae73a8d6dd9b0836c5d1786c7dd71bae46e02f1f16

Validation

• vp check packages/vinext/src/plugins/ignore-dynamic-requests.ts packages/vinext/src/server/prod-server.ts tests/dynamic-requests-build.test.ts tests/prod-server-entry-import.test.ts tests/app-router-worker-entry.test.ts tests/app-router-production-server.test.ts tests/fixtures/app-basic/app/char-code-require
• vp test run tests/dynamic-requests-build.test.ts tests/prod-server-entry-import.test.ts tests/app-router-worker-entry.test.ts
• vp test run tests/app-router-production-server.test.ts -t "bundles a static CommonJS request encoded"
• vp run vinext#build

[james-elicx]

/bigbonk review for issues

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2749

npm i https://pkg.pr.new/create-vinext-app@2749

npm i https://pkg.pr.new/@vinext/types@2749

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

commit: d265caa

[github-actions]

Example	Preview	Production	Original
app-router-cloudflare	preview	production
pages-router-cloudflare	preview	production
app-router-playground	preview	production	original
realworld-api-rest	preview	production
nextra-docs-template	preview	production
benchmarks	preview	production
hackernews	preview	production	original
workers-cache	preview	production
web	preview	production

[ask-bonk]

Review — fix(server): support Node production entry contracts

Reviewed the diff against dev/prod parity and the constant-folding logic. Everything is well-tested (helper unit tests, entry-loader regression, Worker-handler regression, and a real app-basic production fixture) and vp check is clean on the two source files. Verified the targeted tests + the char-code integration test pass locally.

Correctness — looks good

• The require(String.fromCharCode(<literal code units>)) fold correctly gates on: unbound String (hasAstBinding(scope, "String")), non-computed member access, fromCharCode property, and integer literal args in [0, 0xffff]. Spread args and shadowed String fall through to the existing dynamic-request handling — both covered by tests/dynamic-requests-build.test.ts.
• Overwriting only the argument range (not the whole CallExpression) plus the early return avoids a MagicString overlap with the nested String.fromCharCode node. Good.
• globalThis.require is set once before evaluating the entry, matching the existing escape-hatch pattern in otel-tracer-extension.ts / client-trace-metadata.ts. Within a single server run only one entry path (RSC or Pages) is loaded, so the createRequire base is consistent.
• Passing process.env (instead of undefined) to Worker-style fetch(request, env, ctx) is a sensible Node-side equivalent of the CF bindings object.

Minor observations (non-blocking)

1. No hasSignificantPathPart/empty-specifier guard in stringFromCharCodeValue. require(String.fromCharCode()) folds to require("") and require(String.fromCharCode(47)) folds to require("/"). The existing requestHasStaticPart path rejects a bare "/" (see line 565) and would emit the dynamic-request error instead. These are pathological inputs unlikely in real encoded imports, but for parity with the string path you may want to return null when the resolved request is "" or normalizes to "/", letting it fall through to the dynamic replacement rather than emitting an unresolvable require("").

2. globalThis.require is never restored after the process sets it. Fine for a long-running prod server, but worth a one-line comment noting it's intentionally process-lifetime (the loader test has to save/restore it precisely because it leaks). Not a functional problem.

Nothing here blocks merge — the fold is conservative (fails closed) and the parity changes are correct. The two items above are optional hardening.

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Review posted to PR #2749.

Summary

I reviewed PR #2749 (fix(server): support Node production entry contracts). The changes are solid, well-tested, and pass vp check and the targeted test suites locally (including the end-to-end app-basic char-code fixture).

Verified correct:

• The require(String.fromCharCode(...)) constant-fold gates properly on unbound String, non-computed member access, fromCharCode, and integer literal args in [0, 0xffff]. Spread/shadowed-String cases fall through to the existing dynamic handling (both tested).
• Overwriting only the argument range + early return avoids MagicString overlap with the nested fromCharCode node.
• globalThis.require follows the existing escape-hatch pattern; only one entry path loads per server run so the createRequire base is consistent.
• Passing process.env to Worker-style fetch(request, env, ctx) is a reasonable Node equivalent.

Two non-blocking observations flagged in the review:

1. stringFromCharCodeValue has no empty/root-specifier guard — String.fromCharCode() folds to require("") and fromCharCode(47) to require("/"), which the existing string path (line 565) would instead reject as dynamic. Pathological inputs, but a return null there would restore parity.
2. globalThis.require is never restored (fine for a long-lived prod server; worth a comment since the loader test has to save/restore it).

Neither blocks merge — the fold fails closed and the parity fixes are correct.

github run

[james-elicx]

/bigbonk review for issues

[james-elicx]

/bigbonk review for issues

[ask-bonk]

@james-elicx Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

[ask-bonk]

UnknownError: ProviderInitError

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Review — fix(server): support Node production entry contracts

Re-reviewed the current diff (note: it differs substantially from the earlier ask-bonk pass — the prod-server.ts change is now an AsyncLocalStorage-scoped globalThis.require dispatcher, not a plain assignment). Verified vp check is clean on both source files and the targeted suites pass locally (prod-server-entry-import, app-router-worker-entry, dynamic-requests-build → 55/55).

Correctness — looks good

• String.fromCharCode fold gates correctly on: CallExpression → non-computed MemberExpression → unbound String (!hasAstBinding(scope, "String")) → fromCharCode property → every arg an integer Literal in [0, 0xffff]. Spread args and shadowed String fall through to the existing dynamic path, both covered by tests. Overwriting only the argument range (not the whole CallExpression) plus the early return avoids MagicString overlap with the nested node.
• Empty/root parity is actually handled now. The !== "/" guard sends String.fromCharCode(47) back through requestHasStaticPart (→ dynamic replacement), and String.fromCharCode() folds to require("") — which matches the literal string path (line 565 treats "" as static too). The new dynamic-requests-build.test.ts case asserts exactly this (require("") + one MODULE_NOT_FOUND). So the earlier review's "parity concern #1" is resolved.
• ALS dispatcher design is sound. I independently verified that AsyncLocalStorage.run(store, () => import(url)) propagates the store into the imported ESM module's top-level evaluation and isolates concurrent imports — so the Promise.all isolation the tests rely on is a real guarantee, not luck. Per-request require calls inside handleRequest also stay in-context because ALS survives awaits, and waitUntil promises are created in-context.
• Only one entry (RSC or Pages) loads per startProdServer process, so the single process-global dispatcher + entry-scoped resolver is consistent.
• Passing process.env to Worker-style fetch(request, env, ctx) is the right Node analog of the CF bindings object.
• No dev/CF parity gap: the fold lives in the shared ignore-dynamic-requests plugin (dev + build), and on Workers the encoded require is resolved statically at build time, so no runtime globalThis.require provisioning is needed there. Scoping the provisioning to the Node prod server is deliberate and correct.

Minor observations (non-blocking)

1. globalThis.require becomes a throwing dispatcher for the process lifetime. In a pure-ESM prod server globalThis.require starts undefined, so inheritedGlobalRequire is undefined. After the first runWithServerEntryRequire, globalThis.require is permanently the dispatcher, which throws "require() was called outside a Node production server entry context" for any call made off an ALS context. That's a behavioral change from the prior TypeError: ... is not a function. Practically harmless (bundled require runs at load or in-request, both in-context), but a fully-detached callback (e.g. a bare setTimeout in user code) would now get the vinext error instead. The existing comment covers the process-lifetime intent; consider a one-liner noting the out-of-context throw is intentional.

2. runWithServerEntryRequire never restores globalThis.require — fine for a long-lived server (the tests save/restore precisely because it leaks into the shared Vitest process).

Nothing here blocks merge. The fold fails closed, the ALS propagation is verified, and the parity items from the previous review are addressed.

[ask-bonk]

Review posted to PR #2749.

Summary

Re-reviewed PR #2749 (fix(server): support Node production entry contracts). Notably, the current diff differs substantially from the earlier ask-bonk review — prod-server.ts now implements an AsyncLocalStorage-scoped globalThis.require dispatcher, not the plain globalThis.require = ... assignment the previous review described. So I reviewed it fresh.

Verified correct:

• The require(String.fromCharCode(...)) fold gates properly (unbound String, non-computed member, fromCharCode, integer literals in [0, 0xffff]); shadowed/spread cases fall through — all tested.
• The earlier review's "empty/root specifier parity" concern is now resolved: !== "/" guard + require("") matching the literal-string path, with a dedicated test asserting it.
• I independently verified (with a standalone Node script) that AsyncLocalStorage.run(store, () => import(url)) propagates into ESM top-level evaluation and isolates concurrent imports — so the Promise.all isolation the tests depend on is a real guarantee.
• Single-entry-per-process, process.env passthrough to Worker fetch, and no dev/CF parity gap (fold is in the shared plugin; CF resolves statically at build time).
• vp check clean; 55/55 targeted tests pass.

Two non-blocking observations flagged:

1. globalThis.require becomes a process-lifetime throwing dispatcher; off-ALS-context calls now throw the vinext error instead of a TypeError. Harmless in practice, worth a one-line comment.
2. globalThis.require is never restored (fine for a long-lived server; tests save/restore because it leaks into the shared Vitest process).

Nothing blocks merge — the fold fails closed and the ALS design is verified correct.

github run

[github-actions]

Performance benchmarks

Compared d265caa against base 25dc2f3 using alternating same-runner rounds. Next.js was unchanged and skipped.

0 improved · 0 regressed · 6 within ±1.5%

Scenario	Framework	Baseline	Current	Change
Client bundle size (gzip)	vinext	134.3 KB	134.3 KB	⚫ -0.0%
Client entry size (gzip)	vinext	121.9 KB	121.9 KB	⚫ -0.0%
Dev server cold start	vinext	2.80 s	2.82 s	⚫ +0.5%
Production build time	vinext	2.97 s	2.94 s	⚫ -1.0%
RSC entry closure size (gzip)	vinext	111.3 KB	111.2 KB	⚫ -0.0%
Server bundle size (gzip)	vinext	189.0 KB	188.9 KB	⚫ -0.0%

View detailed results and traces

_{🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head}