fix(config): guard against duplicate __dirname/__filename injection by james-elicx · Pull Request #1772 · cloudflare/vinext

james-elicx · 2026-06-05T20:54:01Z

Extracted from #1749 (by @Divkix) as a standalone, focused fix. Closes #1345.

Problem

cjsGlobalsInjectorPlugin (packages/vinext/src/config/next-config.ts) unconditionally prepended const __dirname = ... and const __filename = ... without checking if the user had already declared them. A config file using a common ESM polyfill pattern:

const __dirname = dirname(fileURLToPath(import.meta.url));

would receive a second const __dirname declaration, which Rolldown rejects with a duplicate-declaration parse error, crashing the build.

Fix

Before emitting each preamble line, check for an existing declaration:

const hasOwnDirname  = /\b(?:const|let|var)\s+__dirname\b/.test(code);
const hasOwnFilename = /\b(?:const|let|var)\s+__filename\b/.test(code);

Only inject const __dirname / const __filename when the user hasn't already declared them.

Tests

Two new regression tests in tests/next-config.test.ts:

Config file with its own const __dirname = dirname(fileURLToPath(import.meta.url)) — verifies no duplicate-declaration error.
Config file with its own const __filename = fileURLToPath(import.meta.url) — same.

All 169 next-config.test.ts tests pass; vp check is clean.

Credit

This change is extracted from #1749 by @Divkix, which bundled this guard (Bug A) together with a separate node_modules CJS-globals shim (Bug B). This PR isolates the config-file duplicate-declaration guard so it can be reviewed and merged independently. Full credit to @Divkix for the original fix.

@Divkix

cjsGlobalsInjectorPlugin unconditionally prepended const __dirname / const __filename without checking if the user had already declared them. A config file with its own ESM polyfill: const __dirname = dirname(fileURLToPath(import.meta.url)) would receive a duplicate declaration that Rolldown rejects with a parse error. Fix: check for existing const/let/var declarations of each identifier before emitting the preamble line. Tests: two new cases in next-config.test.ts covering the duplicate-declaration scenarios. Extracted from #1749 by @Divkix. Co-authored-by: Divanshu Chauhan <divkix@divkix.me>

pkg-pr-new · 2026-06-05T20:54:56Z

Open in StackBlitz

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

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

commit: e2483f8

james-elicx · 2026-06-05T20:54:58Z

/bigbonk review for issues

github-actions · 2026-06-05T20:55:19Z

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

james-elicx · 2026-06-05T21:09:59Z

/bigbonk review for issues

ask-bonk · 2026-06-05T21:10:05Z

@james-elicx Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

ask-bonk

Review: guard against duplicate `dirname`/`filename` injection

The core fix is correct, minimal, and well-tested. The two regression tests directly exercise the duplicate-declaration crash path and pass (169/169 in tests/next-config.test.ts). Approving the approach.

A few observations, none blocking:

1. `require` has the same latent collision, but is unguarded

The preamble also unconditionally injects const require = __vinextCreateRequire(...):

`const require = __vinextCreateRequire(${requireBaseLiteral});\n` +

A config that declares its own require (e.g. const require = createRequire(import.meta.url) — a very common ESM polyfill, even more common than the __dirname one) hits the exact same duplicate-const Rolldown parse error this PR fixes. The __dirname/__filename polyfill and the createRequire polyfill almost always appear together in real-world ESM configs, so a user who triggers one is likely to trigger the other.

Since this PR scopes itself to __dirname/__filename, it's fine to leave require for a follow-up, but it's worth a tracking note — the fix as-is is incomplete for the broader "user wrote their own ESM polyfills" case it's motivated by. Consider extending the same guard:

const hasOwnRequire = /\b(?:const|let|var)\s+require\b/.test(code);
// ...
(hasOwnRequire ? "" : `const require = __vinextCreateRequire(${requireBaseLiteral});\n`) +

2. Regex matches inside strings/comments (acceptable here)

/\b(?:const|let|var)\s+__dirname\b/ will false-positive on __dirname appearing inside a string or comment, e.g. export default { x: "const __dirname" }. In that case the guard would suppress injection even though the user never actually declared the binding, and a real __dirname reference elsewhere would then be undefined at runtime.

This is an extremely unlikely edge case and the existing referencesCjsGlobals/reassignsModuleExports heuristics in this file already accept the same class of string/comment false-positives (documented as "false positives are harmless"). So this matches the file's existing precision tradeoff — just flagging that the new guard is slightly less safe than those (a false positive here causes a missing binding, not just an unused one). Fine to keep given how contrived the trigger is.

3. Test for `__dirname` doesn't assert the value is correct

The __filename test correctly asserts .endsWith("next.config.ts"), proving the user's declaration won. The __dirname test only checks typeof === "string" and length > 0 — which would also pass if the injected shim value were used. Consider asserting dir matches tmpDir (or that it ends with the temp dir basename) to prove the user's declaration was actually preserved rather than shadowed. Minor — the __filename test already covers the "user declaration wins" semantics.

Verdict: LGTM as a focused fix. The require gap (#1) is the only thing I'd recommend addressing, ideally in this PR or an immediate follow-up, since it's the same bug class and the same real-world configs trigger it.

ask-bonk · 2026-06-05T21:12:09Z

Review posted to PR #1772.

Summary

The fix is correct, minimal, and well-tested — 169/169 tests pass. I posted a review with three observations:

require has the same unguarded collision (main finding) — The preamble also unconditionally injects const require = __vinextCreateRequire(...). A config declaring its own const require = createRequire(import.meta.url) (a polyfill that almost always appears alongside the __dirname one this PR fixes) hits the identical duplicate-const Rolldown crash. The fix is incomplete for the broader "user wrote their own ESM polyfills" case it's motivated by. Recommended extending the same guard pattern to require.
Regex matches inside strings/comments — Acceptable, since it matches the file's existing precision tradeoff for the sibling heuristics, but slightly less safe (a false positive causes a missing binding rather than an unused one). Fine given how contrived the trigger is.
__dirname test is weaker than the __filename test — It only asserts typeof === "string" rather than proving the user's declaration won. Minor; the __filename test covers the semantics.

Verdict: LGTM as a focused fix. The require gap is the only thing worth addressing in this PR or an immediate follow-up.

github run

Address /bigbonk review on #1772: - Extend the duplicate-declaration guard to `require`. The createRequire ESM polyfill (const require = createRequire(import.meta.url)) commonly appears alongside the __dirname/__filename polyfills and hits the same duplicate-const Rolldown parse error. Skip the createRequire import and the const require line when the user already declares require. - Strengthen the __dirname regression test to assert the resolved value matches the config directory (realpathSync on both sides for the macOS /var symlink), proving the user's declaration wins over the shim. - Add a require regression test covering the same duplicate-declaration scenario. Co-authored-by: Divanshu Chauhan <divkix@divkix.me>

james-elicx · 2026-06-05T21:14:47Z

Addressed the /bigbonk review in e2483f8:

require gap (main finding) — Fixed. Extended the same guard to require: when the user declares their own const require = createRequire(import.meta.url), we now skip both the createRequire import and the const require = ... preamble line, avoiding the identical duplicate-const Rolldown crash. Added a require regression test.
Regex matches inside strings/comments — Left as-is, per the review. It matches the existing precision tradeoff of the sibling referencesCjsGlobals / reassignsModuleExports heuristics in this file, and the trigger is contrived.
__dirname test was weaker than __filename — Strengthened. The test now asserts the resolved value matches the config directory (fs.realpathSync on both sides to normalize the macOS /var → /private/var symlink), proving the user's declaration wins over the injected shim.

All 170 next-config.test.ts and 82 build-optimization.test.ts tests pass; vp check clean. Original fix credit remains with @Divkix (#1749).

james-elicx · 2026-06-05T21:14:51Z

/bigbonk review for issues

ask-bonk · 2026-06-05T21:16:22Z

All checks pass. I've verified the /bigbonk review fixes in commit e2483f8 are correctly implemented.

Summary

The changes addressing the review are complete and verified:

1. require gap (main finding) — Fixed (next-config.ts:712, 730-735)

Added hasOwnRequire guard mirroring the __dirname/__filename guards.
When the user declares const require = createRequire(...), both the injected createRequire import (line 730-732) and the const require = ... preamble line (line 735) are skipped, avoiding the duplicate-const Rolldown crash.
New regression test added (tests/next-config.test.ts) asserting typeof require === "function".

2. Regex string/comment matches — Left as-is

Consistent with the file's existing precision tradeoff documented for referencesCjsGlobals (line 622) and reassignsModuleExports (line 643).

3. __dirname test strengthened

The test now asserts fs.realpathSync(dir) === fs.realpathSync(tmpDir), proving the user's declaration wins over the injected shim. realpathSync on both sides normalizes the macOS /var → /private/var symlink.

Verification:

tests/next-config.test.ts: 170/170 pass
tests/build-optimization.test.ts: 82/82 pass
vp check on both changed files: clean (formatting, lint, types)

The fix now fully covers the "user wrote their own ESM polyfills" case the PR is motivated by — __dirname, __filename, and require are all guarded identically. Original fix credit remains with @Divkix (#1749).

github run

james-elicx enabled auto-merge (squash) June 5, 2026 20:54

james-elicx disabled auto-merge June 5, 2026 20:54

ask-bonk Bot reviewed Jun 5, 2026

View reviewed changes

james-elicx closed this Jun 5, 2026

james-elicx deleted the fix/dirname-duplicate-declaration branch June 5, 2026 21:16

james-elicx mentioned this pull request Jun 5, 2026

fix(config): avoid duplicate CJS global shims #1771

Merged

Navigation Menu

Uh oh!

fix(config): guard against duplicate dirname/filename injection - #1772

fix(config): guard against duplicate dirname/filename injection#1772
james-elicx wants to merge 2 commits into
mainfrom
fix/dirname-duplicate-declaration

james-elicx commented Jun 5, 2026

Uh oh!

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

edited

Loading

Uh oh!

james-elicx commented Jun 5, 2026

Uh oh!

github-actions Bot commented Jun 5, 2026

Uh oh!

james-elicx commented Jun 5, 2026

Uh oh!

ask-bonk Bot commented Jun 5, 2026

Uh oh!

ask-bonk Bot left a comment

Uh oh!

ask-bonk Bot 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 commented Jun 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

james-elicx commented Jun 5, 2026

Problem

Fix

Tests

Credit

Uh oh!

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

Uh oh!

Uh oh!

james-elicx commented Jun 5, 2026

Uh oh!

github-actions Bot commented Jun 5, 2026

Uh oh!

james-elicx commented Jun 5, 2026

Uh oh!

ask-bonk Bot commented Jun 5, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Review: guard against duplicate __dirname/__filename injection

1. require has the same latent collision, but is unguarded

2. Regex matches inside strings/comments (acceptable here)

3. Test for __dirname doesn't assert the value is correct

Uh oh!

ask-bonk Bot commented Jun 5, 2026

Summary

Uh oh!

james-elicx commented Jun 5, 2026

Uh oh!

james-elicx commented Jun 5, 2026

Uh oh!

ask-bonk Bot commented Jun 5, 2026

Summary

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

edited

Loading

Review: guard against duplicate `dirname`/`filename` injection

1. `require` has the same latent collision, but is unguarded

3. Test for `__dirname` doesn't assert the value is correct