fix(config): avoid duplicate CJS global shims

[Divkix]

Description

Fixes bug A from #1345 by skipping vinext's injected next.config.ts __dirname / __filename shims when the config already declares those identifiers. This intentionally excludes the separate node_modules runtime shim work for bug B.

Related Issue

Part of #1345

Potential Risk & Impact

Low. Scope is limited to next.config CJS global injection and preserves existing shim behavior when the identifiers are not user-declared.

How Has This Been Tested?

• vp test run tests/next-config.test.ts -t "does not inject"
• vp test run tests/next-config.test.ts
• vp check
• vp test (7145 passed, 4 skipped)

[pkg-pr-new]

Open in StackBlitz

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

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

commit: bc4cd50

[james-elicx]

Pushed bc4cd50 to this branch: extended the same 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 identical duplicate-const Rolldown parse error — so when the user declares their own require, we now skip both the createRequire import and the const require = ... preamble line. Added a require regression test. All 170 next-config tests pass; vp check clean. (This was flagged by the /bigbonk review on #1772.)

[ask-bonk]

LGTM. The change extends the existing duplicate-declaration guard to require in addition to __dirname/__filename, which is exactly the right scope for bug A of #1345. The createRequire import and the const require = ... preamble line are gated on the same hasOwnRequire flag, so the alias-based import (__vinextCreateRequire) never collides with a user-supplied import { createRequire }. Verified all 170 tests in tests/next-config.test.ts pass locally.

A few non-blocking observations below.

[ask-bonk]

The three hasOwn* regexes use \b(?:const|let|var)\s+IDENT\b, which is consistent with the conservative-heuristic style already used by referencesCjsGlobals / reassignsModuleExports. Two known gaps (both acceptable here, since they don't trigger the duplicate-const crash this PR targets):

• Destructured declarations (const { createRequire } = ..., const [require] = ...) won't match.
• function require() {} / function __dirname() {} won't match.

These are the same edge cases the surrounding heuristics already accept, so no change requested — just noting it for the record.

[ask-bonk]

Each new test covers one identifier in isolation, but the real-world failure in #1345 is the combined polyfill block where createRequire, __filename, and __dirname are all declared together (the canonical ESM CJS-shim preamble). Consider adding one test that declares all three at once to lock in the exact reproduction from the issue:

it("does not inject any CJS shim when the user declares the full polyfill block", async () => {
  tmpDir = makeTempDir();
  fs.writeFileSync(
    path.join(tmpDir, "next.config.ts"),
    `import { createRequire } from "node:module";\n` +
      `import { dirname } from "node:path";\n` +
      `import { fileURLToPath } from "node:url";\n` +
      `const __filename = fileURLToPath(import.meta.url);\n` +
      `const __dirname = dirname(__filename);\n` +
      `const require = createRequire(import.meta.url);\n` +
      `export default { env: { DIR: __dirname, HAS_REQUIRE: typeof require } };\n`,
  );

  const config = await loadNextConfig(tmpDir);
  expect(fs.realpathSync(config?.env?.DIR as string)).toBe(fs.realpathSync(tmpDir));
  expect(config?.env?.HAS_REQUIRE).toBe("function");
});

[ask-bonk]

Review submitted successfully. Here's my assessment of PR #1771:

Summary

Approved. The change is well-scoped, correct, and consistent with the codebase's existing patterns.

What the PR does

Extends the duplicate-declaration guard in cjsGlobalsInjectorPlugin to cover require in addition to __dirname/__filename. When the user's next.config.ts already declares any of these identifiers, vinext skips injecting its own const shim, avoiding the duplicate-const Rolldown parse error (bug A of #1345).

Correctness verification

• The createRequire import (__vinextCreateRequire) and the const require = ... preamble line are both gated on the same hasOwnRequire flag — they're correctly paired, so there's no scenario where the const references an undefined import.
• The alias (__vinextCreateRequire) means vinext's import never collides with a user's own import { createRequire }.
• The three regexes (\b(?:const|let|var)\s+IDENT\b) match the canonical ESM CJS-polyfill pattern from the issue.
• All 170 tests in tests/next-config.test.ts pass locally.
• Scope correctly excludes bug B (node_modules runtime shims), as stated in the PR description.

Non-blocking feedback posted inline

1. The hasOwn* regexes don't catch destructured (const { createRequire } = ...) or function-form declarations — but these don't trigger the targeted crash and match the surrounding heuristics' accepted gaps. No change requested.
2. Suggested adding one combined test declaring createRequire + __filename + __dirname together (the actual real-world reproduction in Build failure: __dirname/__filename shim injection conflicts and runtime errors #1345), since the three new tests each only cover one identifier in isolation. Included a ready-to-paste test as a suggestion.

github run