fix(server): define CJS path globals in bundled modules by NathanDrake2406 · Pull Request #1740 · cloudflare/vinext

NathanDrake2406 · 2026-06-04T14:43:55Z

Overview

Item	Detail
Goal	Match Next.js server-module compatibility for proxy files and other server-side user modules that read `__filename` or `__dirname`.
Core change	Extend the existing source-identity transform to inject server-only top-level CJS path globals for eligible source modules.
Primary review files	`packages/vinext/src/plugins/import-meta-url.ts`, `tests/import-meta-url.test.ts`, `tests/app-router.test.ts`
Expected impact	Server-side user modules can read `__filename` / `__dirname` after ESM bundling without throwing, while preserving source-module identity semantics.

Why

Vinext already rewrites direct import.meta.url reads so module identity survives bundling. The same compatibility boundary did not define CommonJS path globals, so server-side user modules that read __filename or __dirname could throw in the bundled ESM server output before user code returned a response.

Next.js server artifacts run with CommonJS-compatible path globals available. This PR closes that compatibility gap for Vinext server modules without changing client output.

What changed

Area	Before	After
Server CJS globals	Free `__filename` / `__dirname` reads could throw after bundling.	Eligible server modules get injected top-level `var __filename` / `var __dirname` bindings pointing at canonical source paths.
`import.meta.url`	Existing source-identity rewrite handled direct reads.	Preserved; the transform now handles both `import.meta.url` and server CJS path globals in one source-identity pass.
Client output	No CJS global support.	Unchanged; CJS global injection is server-only.
Build artifacts / deps	Risk if applied to generated output or dependencies.	Reuses the existing source-identity eligibility boundary: project source only, excluding dependencies and known build output directories.

Implementation notes

This PR uses binding injection rather than free-identifier replacement. For eligible server modules, the transform inserts top-level var bindings for the missing CJS path globals and lets JavaScript scope rules handle params, nested locals, object shorthand, assignment/update behaviour, class expression names, and nested var hoisting naturally.

Injection is gated by a syntactic read check so we only inject when the module actually references the global as a value, skipping member names, object/class keys, and lookalikes.

Bindings are inserted after the directive prologue, so file-level directives such as "use server" and "use strict" remain directives.

Binding / syntax safeguards

The transform skips injection for a global name when:

There is no syntactic read reference to the name anywhere in the module.
The name is already bound at the top level of the module — value imports, top-level declarations, exported declarations, destructuring declarations, and top-level for (var ...) forms (including for-in and for-of).

The plugin runs enforce: "post" after Vite's TypeScript transforms have stripped type-only syntax, so the parsed AST is always plain JavaScript. TypeScript-only binding forms (import type, declare, enum, namespace, import =) are therefore not part of the binding-detection surface.

Regression coverage

Test area	Coverage
Focused transform tests	Injection, canonical source paths, local binding conflicts, destructuring conflicts, `for (var ...)` conflicts, object shorthand, pattern defaults, read-reference gating, directive-prologue preservation, build-output exclusion.
Production App Router build	Builds a minimal app with `proxy.ts` that reads `__filename`, redirects `/home`, and continues normally for `/`.
Upstream compatibility	The production regression is ported from Next.js' `proxy-nfc-traced` behaviour.

Validation

vp test run tests/import-meta-url.test.ts
vp test run tests/app-router.test.ts -t "builds proxy.ts that reads __filename before redirecting"
vp check
Next.js proxy-nfc-traced compatibility case against v16.2.6

Risk / compatibility

Public API: no new exported runtime API.
Client output: unchanged.
Server output: eligible source modules that reference __filename / __dirname now receive source-path bindings.
Scope: limited to transformable project source files inside the app root, excluding dependencies and known build output paths.

References

Reference	Why it matters
Next.js proxy-nfc-traced test	Upstream behaviour this regression ports.
Next.js proxy fixture	The proxy redirects `/home` and reads `__filename` before continuing.
Next.js proxy file convention docs	Documents the `proxy` file convention and runtime expectations.

Server-side user modules that referenced __filename or __dirname ran inside vinext's ESM bundle without CommonJS path globals. Proxy files that used __filename, including Next.js's proxy-nfc-traced fixture, threw before redirecting or continuing. The missing compatibility boundary was the source-identity transform: it preserved import.meta.url but did not provide Next-compatible CJS path globals for server modules. The transform now rewrites free server-side __filename and __dirname reads to canonical source paths while preserving local bindings, object keys, and client modules. Regression coverage ports the upstream proxy-nfc-traced scenario at the transform boundary and through a production App Router build.

pkg-pr-new · 2026-06-04T14:44:58Z

Open in StackBlitz

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

commit: 0e2cbb7

…r __filename/__dirname Replaces the custom free-identifier AST walker with a simpler top-level var injection approach. This fixes two review blockers: 1. Var hoisting bug: the old walker discovered nested var declarations only after entering their block, so reads before the block could be incorrectly rewritten even though they were shadowed by a hoisted local var. 2. TS type-space awareness: by injecting module-level bindings instead of rewriting individual identifiers, we no longer need to distinguish type-space from value-space usage. JavaScript scope rules naturally handle params, nested locals, object shorthand, assignment targets, etc. We only need a lightweight top-level binding scan to avoid duplicate declarations with let/const/class/function/value imports. All other shadowing is handled by JS semantics automatically. Per-review feedback on PR cloudflare#1740.

…n top-level binding scan Adds hasBindingInPattern() to recurse through ObjectPattern, ArrayPattern, RestElement, and AssignmentPattern so destructuring declarations like const { __filename } = source correctly block injection. Also handles top-level ForStatement/ForInStatement/ForOfStatement with var declarations, which create top-level bindings that must block injection. let/const in for-loop init are block-scoped and correctly ignored.

Changes output.prepend to output.appendLeft(findDirectivePrologueEnd(ast)) so that use server, use client, and use strict directives remain the first statement in the program body. Adds findDirectivePrologueEnd() helper that scans leading ExpressionStatement(Literal<string>) nodes and returns the end position of the last directive.

james-elicx · 2026-06-04T22:16:38Z

/bigbonk review

ask-bonk · 2026-06-04T22:46:58Z

@james-elicx Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

Keep PR cloudflare#1740's baked-literal value strategy (var __filename/__dirname computed in the plugin) but adopt the condensed binding-collision helpers. Drops ~31% of the added lines vs the original PR with byte-identical generated output and no new runtime surface.

… branches - rewriteServerCjsGlobals now delegates to rewriteCanonicalSourceIdentity (the same function the plugin runs), removing the parallel rewriteCanonicalServerCjsGlobals implementation so the unit tests exercise the production code path. - Drop the TSEnumDeclaration/TSModuleDeclaration/TSImportEqualsDeclaration branches and importKind/declare guards from declaresBinding: Vite's parseAst (rolldown/oxc) rejects TS syntax and the plugin runs after TS is stripped, so those nodes/fields are unreachable.

Replace the substring `code.includes(name)` injection gate with a syntactic read-reference check (hasReadReference): an Identifier in value position, excluding non-computed member properties (obj.__filename), non-computed object/class keys, and lookalikes (__filenameFoo). Object shorthand, computed keys/members, default values, and assignment targets still count as reads. Collision safety is unchanged (hasTopLevelBinding still blocks a `var` that would redeclare a top-level let/const/class/import). This fixes baking an unused `var __filename = "/abs/path"` into modules that only mention the name in a member access, key, comment, or a lookalike identifier. Adds 6 false-positive/read tests.

james-elicx · 2026-06-04T23:38:34Z

/bigbonk review

…format

Replaces the two name-parameterised predicates (hasReadReference and hasTopLevelBinding) with a single analyzeServerCjsGlobals() function that walks the AST once and returns { reads, topLevelBindings }. injectServerCjsGlobals() now reads from the analysis instead of asking the same questions in two separate functions. The injection rule stays the same: inject iff reads.has(name) and not topLevelBindings.has(name) Behaviour is unchanged: all 30 import-meta-url tests and the proxy build integration test pass.

james-elicx · 2026-06-05T11:26:40Z

/bigbonk review

findDirectivePrologueEnd returned 0 for a module with no directive prologue, so appendLeft(0, ...) inserted the var before a leading `#!` shebang (which oxc stores in ast.hashbang, outside ast.body), producing output like `\nvar __filename=...;#!/usr/bin/env node` that fails to parse. Start the injection floor after the hashbang.

…reads recordReads' generic walk visited ImportSpecifier.imported, ExportSpecifier.exported, and ExportAllDeclaration.exported identifiers, so `export * as __filename from`, `export { foo as __filename }`, and `import { __filename as foo }` each recorded __filename as a read with no module binding, injecting a spurious unused `var __filename = "<path>"`. Add explicit cases: imports read nothing; export-namespace aliases read nothing; named-export specifiers reference only `local` (and only without a re-export source).

… production gate The test-only entry re-implemented a subset of eligibility (within-root + build-output exclusion) but omitted the node_modules and script-extension guards that the plugin applies via transformableModuleCanonicalId, so tests could pass while the real plugin path behaved differently. Delegate to transformableModuleCanonicalId so tests exercise the same boundary.

rewriteCanonicalImportMetaUrl re-implemented the import.meta.url branch already in rewriteCanonicalSourceIdentity (parse, collect ranges, overwrite, generateMap) and was reached only by the test-only rewriteImportMetaUrl, so the test path could drift from the production path. Delegate rewriteImportMetaUrl to rewriteCanonicalSourceIdentity and drop the duplicate.

The CJS-global gate matches __filename/__dirname anywhere, admitting many modules with no import.meta.url. collectImportMetaUrlRanges walked the full AST for all of them; guard it behind the cheap mayContainImportMetaUrl substring check (a range can only exist when the substring is present).

The single-line delegation introduced in ad0000a was 104 chars, over the 100-col print width, failing `vp check`. Wrap the call.

james-elicx · 2026-06-05T11:57:09Z

/bigbonk review

james-elicx · 2026-06-05T12:18:28Z

/bigbonk review

ask-bonk

Review: CJS path globals in bundled server modules

Reviewed the transform, the binding/read analysis, and the test coverage. This is a well-designed change and I'm comfortable with the approach.

What's good:

Binding injection over free-identifier replacement is the right call. Injecting var __filename/__dirname at module top-level and letting JS scope rules handle params, shadowing, shorthand, and nested var hoisting is much more robust than a scope-modeling rewrite walker. The class-expression and pattern-default tests demonstrate this pays off.
Server-only gating is correct. this.environment?.name === "client" keeps client output unchanged, and the CJS injection only runs in the server branch of rewriteCanonicalSourceIdentity. Since this is a build-time Vite transform (not request-handling logic), it applies uniformly to dev ssrLoadModule and the prod bundle, so there's no dev/prod parity gap to worry about here.
Binding-detection completeness. I audited recordDirectTopLevelBindings + recordModuleScopedVarBindings against the top-level binding forms (import default/named/namespace, const/let/var, function/class decls, exported decls, export default class Named, destructuring, top-level for (var ...)/for-in/for-of). The dangerous failure mode would be a missed module binding causing an injected var to collide with an existing let/const/class/import (a SyntaxError) — I couldn't find a missed form. Catch-clause params are correctly treated as block-scoped (not module bindings).
enforce: "post" legitimately removes TS-only binding forms from the surface, so the comment about not needing to handle enum/namespace/import type holds.
canonicalizePath refactor to tryRealpathSync(value) ?? path.resolve(value) preserves the prior realpathSync.native → path.resolve fallback behavior.
Test coverage is excellent — 42 passing focused tests plus a production-build regression ported from Next.js proxy-nfc-traced. All pass locally.

Minor (non-blocking) observations below. None of these are correctness bugs; they're about the read walker over-reporting in harmless ways. No changes required to merge.

ask-bonk · 2026-06-05T12:21:23Z

+      case "Property":
+        if (value.computed) recordReads(value.key);
+        recordReads(value.value);
+        return;


Minor / non-blocking: the read walker can over-report reads for names that only appear as binding targets rather than value reads. Two cases:

Function/arrow params: function f(__filename) { ... } where nothing else reads the global — the param Identifier is visited via the default case and recorded as a read.

Function-scoped shorthand destructuring: function f() { const { __dirname } = x; } — the Property case recurses into value.value (the binding-target Identifier) and records it as a read.

In both cases moduleBindings is empty (params and function-locals aren't module bindings), so an unnecessary var __filename/var __dirname gets injected at module scope. This is harmless — the injected var is unused and the inner binding shadows it — but it does mean we emit dead module-scoped declarations for modules that never actually read the CJS global at module scope. Not worth complicating the walker to fix; flagging only so it's a known, intentional tradeoff. A short comment near the Property/default cases noting that param/local binding targets may be counted as reads (and why that's safe) would help future readers.

ask-bonk · 2026-06-05T12:21:23Z

+      }
+    } finally {
+      fs.rmSync(tmpDir, { recursive: true, force: true });
+    }


Nice regression test. One coverage gap worth a follow-up (not blocking): this only exercises the production build path. The transform also runs in dev via ssrLoadModule, and dev is where users hit the proxy/middleware first. Consider adding a dev-server case (or a pages-router proxy variant) that loads a proxy.ts reading __filename through the dev server, to lock in dev/prod parity for this behavior.

ask-bonk · 2026-06-05T12:21:32Z

Review posted successfully. Here's a summary of my review of PR #1740:

Summary

Verdict: Approve-quality change. The PR fixes a real Next.js compatibility gap (server modules reading __filename/__dirname after ESM bundling) with a sound design and excellent test coverage. My findings were all minor and non-blocking.

What I verified

Binding injection over free-identifier replacement is the correct architectural choice — it delegates shadowing/scoping to JS itself instead of a fragile scope-modeling walker.
Server-only gating (this.environment?.name === "client") correctly leaves client output unchanged.
No dev/prod parity gap — this is a build-time Vite transform, not request-handling logic, so it applies uniformly to dev ssrLoadModule and the prod bundle (the AGENTS.md parity concern doesn't apply here).
Binding-detection completeness — I audited the binding walkers against all top-level binding forms. The dangerous failure mode (a missed binding causing an injected var to collide with an existing let/const/class/import → SyntaxError) does not occur; I couldn't find a missed form. Catch params are correctly block-scoped.
canonicalizePath refactor to tryRealpathSync(...) ?? path.resolve(...) preserves prior behavior.
Tests pass — all 42 focused tests green locally.

Non-blocking observations posted inline

The read walker can over-report reads for function params and function-scoped destructuring binding targets, causing harmless dead var injection. Suggested a clarifying comment.
The regression test only covers the production build path; suggested a dev-server case for dev/prod parity follow-up.

github run

NathanDrake2406 added 3 commits June 5, 2026 01:40

NathanDrake2406 marked this pull request as ready for review June 4, 2026 16:23

james-elicx added 3 commits June 5, 2026 00:13

NathanDrake2406 added 3 commits June 5, 2026 14:22

fix(server): add typeof guards for switch-exhaustiveness-check; auto-…

76d96f9

…format

test(server): rename stale assignment/update test to match assertions

5f04254

NathanDrake2406 marked this pull request as draft June 5, 2026 09:24

fix(server): handle module-scoped var bindings

4c09fb3

NathanDrake2406 marked this pull request as ready for review June 5, 2026 09:35

NathanDrake2406 and others added 2 commits June 5, 2026 19:45

refactor(server): simplify module-scoped var binding scan

3935806

Merge branch 'main' into nathan/proxy-nfc-traced

a33f543

james-elicx mentioned this pull request Jun 5, 2026

fix(config): guard against duplicate __dirname/__filename injection and cover node_modules #1749

Closed

james-elicx added 6 commits June 5, 2026 12:47

style(server): wrap rewriteImportMetaUrl delegation to satisfy formatter

0e2cbb7

The single-line delegation introduced in ad0000a was 104 chars, over the 100-col print width, failing `vp check`. Wrap the call.

ask-bonk Bot reviewed Jun 5, 2026

View reviewed changes

james-elicx merged commit f3f120e into cloudflare:main Jun 5, 2026
30 checks passed

github-actions Bot mentioned this pull request Jun 5, 2026

chore: version packages #1759

Merged

NathanDrake2406 deleted the nathan/proxy-nfc-traced branch June 13, 2026 13:25

This was referenced Jul 11, 2026

fix(check): exclude test-runner files from app compatibility scans #2595

Closed

fix(check): exclude test-runner files from app compatibility scans #2596

Merged

Navigation Menu

Uh oh!

fix(server): define CJS path globals in bundled modules - #1740

fix(server): define CJS path globals in bundled modules#1740
james-elicx merged 19 commits into
cloudflare:mainfrom
NathanDrake2406:nathan/proxy-nfc-traced

NathanDrake2406 commented Jun 4, 2026 •

edited

Loading

Uh oh!

pkg-pr-new Bot commented Jun 4, 2026 •

edited

Loading

Uh oh!

james-elicx commented Jun 4, 2026

Uh oh!

ask-bonk Bot commented Jun 4, 2026 •

edited

Loading

Uh oh!

james-elicx commented Jun 4, 2026

Uh oh!

james-elicx commented Jun 5, 2026

Uh oh!

james-elicx commented Jun 5, 2026

Uh oh!

james-elicx commented Jun 5, 2026

Uh oh!

ask-bonk Bot left a comment

Uh oh!

ask-bonk Bot Jun 5, 2026

Uh oh!

ask-bonk Bot Jun 5, 2026

Uh oh!

ask-bonk Bot commented Jun 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

NathanDrake2406 commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Why

What changed

Implementation notes

Binding / syntax safeguards

Regression coverage

Validation

Risk / compatibility

References

Uh oh!

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

Uh oh!

Uh oh!

james-elicx commented Jun 4, 2026

Uh oh!

ask-bonk Bot commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

james-elicx commented Jun 4, 2026

Uh oh!

james-elicx commented Jun 5, 2026

Uh oh!

james-elicx commented Jun 5, 2026

Uh oh!

james-elicx commented Jun 5, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Review: CJS path globals in bundled server modules

Uh oh!

ask-bonk Bot Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot commented Jun 5, 2026

Summary

What I verified

Non-blocking observations posted inline

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

NathanDrake2406 commented Jun 4, 2026 •

edited

Loading

pkg-pr-new Bot commented Jun 4, 2026 •

edited

Loading

ask-bonk Bot commented Jun 4, 2026 •

edited

Loading