fix(build): avoid __dirname shim conflicts and cover node_modules#1385
fix(build): avoid __dirname shim conflicts and cover node_modules#1385james-elicx wants to merge 2 commits into
Conversation
Two related issues with CommonJS-global shim injection were causing
build failures and runtime errors on the Next.js deploy suite:
1. **next.config.ts redeclaration.** When a config file already declared
its own `const __dirname = fileURLToPath(import.meta.url)` polyfill,
the injector emitted a second `const __dirname = ...` shim and OXC
rejected the duplicate `const` with
`Identifier __dirname has already been declared`.
Fix: add `findUserDeclaredCjsGlobals` to scan masked source for
user-owned bindings of `__dirname` / `__filename` (plain, comma-
continued, or object-destructure forms; outside strings, template
literals, and comments) and skip injection for the names already
declared.
2. **node_modules ReferenceError.** Third-party packages bundled into
the ESM server entry (`type: module`) reference `__dirname` /
`__filename`, but the previous shim only ran for `next.config.ts`.
Fix: add a per-module `vinext:cjs-globals-shim` plugin that runs in
the `ssr` / `rsc` environments, targets `node_modules` `.js/.mjs/
.cjs` files containing bare references (skipping strings, templates,
and comments), and prepends a local
`const __filename = fileURLToPath(import.meta.url);
const __dirname = dirname(__filename);`
preamble. Helper imports are aliased so they do not collide with
user code already importing `dirname` or `fileURLToPath`.
The shim only prevents `ReferenceError`; libraries that load sibling
on-disk assets through `__dirname` still need to be externalised via
`serverExternalPackages`.
commit: |
|
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good PR. Two focused fixes with solid test coverage and thorough documentation. A few observations below — one potential correctness issue worth discussing, and some minor nits.
Overall assessment: The approach is sound. The findUserDeclaredCjsGlobals helper for the config injector and the new cjsGlobalsShimPlugin for node_modules are well-scoped and correctly layered. The limitation about import.meta.url pointing at the bundle rather than the original module is clearly documented, which is the right call.
| const masked = source.replace( | ||
| /\/\*[\s\S]*?\*\/|\/\/[^\n]*|`(?:[^`\\$]|\\.|\$(?!\{))*`|"(?:[^"\\]|\\.)*"|'(?:[^'\\]|\\.)*'/g, | ||
| (m) => " ".repeat(m.length), | ||
| ); |
There was a problem hiding this comment.
The masking regex here is duplicated verbatim from findUserDeclaredCjsGlobals in next-config.ts.
Consider extracting this into a shared helper (e.g., maskStringsAndComments(source: string): string) to avoid the two copies drifting apart. Not a blocker — the regex is stable and well-tested — but it would be nice for maintainability.
| // Strip any query suffix before the filter recheck. Vite passes ids | ||
| // with `?v=…` cache busters that the filter regex already allows | ||
| // through, but downstream logic only cares about the source text. | ||
| void id; |
There was a problem hiding this comment.
void id; is a clear "intentionally unused" signal, but on its own it looks a bit orphaned. The comment above it discusses query suffixes but doesn't explicitly say "id is only used by the Vite filter".
nit: consider void id; // consumed by the Vite filter; transform only uses code`` or similar.
| // keyword itself, a comma, an opening brace, or a colon (object-pattern | ||
| // alias). Anything preceded by `=` is the initializer side and ignored, | ||
| // so `const x = __dirname;` does NOT count as declaring `__dirname`. | ||
| const declStmt = /\b(const|let|var)\b([^;]*)/g; |
There was a problem hiding this comment.
Minor note on the declStmt regex. /\b(const|let|var)\b([^;]*)/g captures everything up to the next ;. This works for the common case, but relies on semicolons as statement boundaries. Two observations:
-
ASI-eligible code without semicolons — if the user writes
const __dirname = dirname(...)without a trailing;, the[^;]*consumes past it into subsequent lines. This is actually still correct (it just captures more text than one statement), so no bug. -
for-loop initializers like
for (let __dirname = 0; ...)— the[^;]*stops at the first;in the for-header, capturinglet __dirname = 0and reporting a false positive. In practice nobody writesfor (let __dirname ...)so this is purely academic.
No action needed — failure modes are safe or unrealistic. The docstring already covers the important invariant (false negatives surface the collision error, false positives skip the shim → ReferenceError). Just flagging for completeness.
| enforce: "pre", | ||
|
|
||
| transform: { | ||
| filter: { id: /\/node_modules\/.+\.(?:m?js|cjs)(?:\?.*)?$/ }, |
There was a problem hiding this comment.
Plugin ordering confirmation: This plugin runs with enforce: "pre", so it executes before the top-level commonjs() plugin (which runs in "normal" phase). That means for a node_modules file using both __dirname and require(), the shim's import statements are prepended first, then commonjs() converts the CJS patterns. The ordering is correct — the prepended ESM import statements won't confuse the CJS-to-ESM transform.
Also worth noting: commonjs() at the top level in index.ts doesn't have a node_modules filter exclusion (unlike the one used in loadNextConfig), so CJS code in node_modules still gets converted to ESM as expected.
| // overhead in the hot transform path. Stack traces still resolve to | ||
| // the original source via the bundler's combined sourcemap once the | ||
| // module is concatenated. | ||
| map: null, |
There was a problem hiding this comment.
Source map trade-off is reasonable but worth noting: when the preamble is multi-line (e.g., 4 lines for both __dirname + __filename), every line in the original source shifts by that offset. Stack traces during development will be off by 4 lines for any error originating from a shimmed node_modules file. Since this only applies to server-side node_modules code and the bundler's combined sourcemap corrects it in production, this is acceptable.
If this ever becomes a debugging pain point, MagicString with prepend() + generateMap() would fix it without changing the architecture.
| // (`{ filter, handler }`) on Vite 8+. Resolve either shape to the | ||
| // underlying function so the tests can invoke it directly with a fake | ||
| // plugin `this` context. | ||
| function getTransformHandler(): ( |
There was a problem hiding this comment.
Nice defensive handling of both the function-style and object-style ({ filter, handler }) transform hook shapes. This future-proofs the test against Vite API evolution.
| expect(typeof config?.env?.OWN_DIR).toBe("string"); | ||
| expect(ownFile?.endsWith("next.config.ts")).toBe(true); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Good regression test. One additional edge case worth considering for a follow-up: what about const { __dirname: localAlias } = something;? The bindingRegex in findUserDeclaredCjsGlobals uses [,{:] as preceding context, so { __dirname: localAlias } would match __dirname after { — which is a false positive (the binding is localAlias, not __dirname). But the failure mode is safe (skipping the shim when it's not needed, since the user has a local alias), so this is fine in practice.
|
Review posted successfully. Here's a summary of what I reviewed and commented on: PR #1385 Review Summary The PR fixes two related issues with CJS-global shim injection that surfaced in the Next.js v16 deploy suite:
Line comments posted (7 total):
|
Address Bonk review feedback on #1345: - Extract the duplicated strings/comments/template-literal masking regex from `findUserDeclaredCjsGlobals` and `detectCjsGlobalReferences` into a shared `maskStringsAndComments` helper in `utils/mask-source.ts`. - Avoid false positives in `findUserDeclaredCjsGlobals` for object-key uses of `__dirname` / `__filename`. The trailing `(?!\\s*:)` lookahead excludes property keys (`{ __dirname: ... }`, `{ __dirname: alias }`) from binding matches so the injector keeps the shim in place when no real top-level declaration exists. - Clarify the `void id` rationale in `cjsGlobalsShimPlugin.transform`.
|
/bigbonk review |
There was a problem hiding this comment.
Good PR. Two well-scoped fixes with solid test coverage. The shared mask-source.ts extraction addresses the previous review's duplication concern, and the (?!\s*:) lookahead correctly handles the const { __dirname: alias } destructuring edge case.
A few fresh observations not covered by the previous review:
Correctness — looks solid. The two-pass scan approach (quick reject via raw regex, then masked scan) is the right architecture. The findUserDeclaredCjsGlobals false-positive/false-negative analysis in the docstring is correct and well-reasoned. The cjsGlobalsShimPlugin scoping (node_modules only, server envs only) is correctly restrictive.
Dev/prod parity. The cjsGlobalsShimPlugin is registered as a top-level Vite plugin via index.ts, so it runs in both dev and prod for SSR/RSC environments. The next.config injector fix (findUserDeclaredCjsGlobals) runs through the cjsGlobalsInjectorPlugin which applies to .ts config files loaded via Vite's module runner — also runs in both dev and build. No parity gap here.
Performance. The quick-reject !/__dirname|__filename/.test(source) before maskStringsAndComments is critical — maskStringsAndComments allocates a new string via .replace() for every call, and this hot path sees every node_modules .js/.mjs/.cjs file. The vast majority of modules won't contain either identifier, so the fast path is almost always taken. This is well-designed.
One note for future consideration (not blocking): the MASK_REGEX uses [\s\S]*? for block comments which is correct but non-greedy — on pathological inputs with many /* openers and no closers it degrades gracefully (each unmatched /* just doesn't match). Template literal handling correctly stops at ${ to preserve interpolation visibility. The regex is sound.
Clean implementation, thorough tests, clear documentation. Ship it.
|
Review posted successfully. Here's a summary: PR #1385 Review — APPROVED Two well-scoped fixes with solid test coverage:
Key verification points:
|
1 participant
Summary
Two related fixes to vinext's CommonJS-global shim injection that surfaced on the Next.js v16 deploy suite:
const __dirname = fileURLToPath(import.meta.url)(a real pattern in Next.js v16-style configs), the injector still emitted its ownconst __dirname = ...and OXC rejected the duplicateconst. NewfindUserDeclaredCjsGlobalshelper masks strings/comments and detects existingconst/let/var __dirname/__filenamebindings (plain, comma-continued, or object-destructure); the injector skips those names.__dirname/__filename, but the server entry is emitted as ESM (type: module) where those CJS globals do not exist. Newvinext:cjs-globals-shimplugin runs in thessr/rscenvironments only, targetsnode_modules.js/.mjs/.cjsfiles containing bare references, and prepends a per-module preamble that declares both bindings viaimport.meta.url+fileURLToPath+dirname(helpers imported under__vinext_*aliases to avoid colliding with user imports).The shim only prevents the
ReferenceError. Libraries that load sibling on-disk assets through__dirname(native addons, embedded wasm) still need to be externalized viaserverExternalPackages—import.meta.urlafter bundling points at the bundle, not the original module location.Test plan
pnpm test tests/next-config.test.ts(125 tests pass, including new regressions for user-declared__dirname/__filenameand helper-level coverage offindUserDeclaredCjsGlobals)pnpm test tests/cjs-globals-shim.test.ts(18 new tests coveringdetectCjsGlobalReferences,buildCjsGlobalsShimPreamble, plugin filter, environment gating, and full transform output)pnpm test tests/build-optimization.test.ts tests/deploy.test.ts tests/standalone-build.test.ts(304 pass, no regressions)pnpm run checkclean (format + lint + types)Closes #1345