feat(commonjs): add externalBuiltinsRequire to stub external Node builtins when wrapped

[charliecreates]

Rollup Plugin Name: commonjs

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers:

resolves #1924

Description

Adds a new externalBuiltinsRequire option to @rollup/plugin-commonjs that controls how require('node:*') from wrapped CJS externals is handled.

• 'create-require' (default): current behavior; lazily resolves require via module.createRequire(import.meta.url) so Node built-ins continue to work as today.
• 'stub': emits a minimal proxy that exports a throwing __require() and does not import node:module, letting edge runtimes avoid bundling node:module when those paths are dead.

The option is threaded through src/index.js into getExternalBuiltinRequireProxy() with validation (invalid values warn and fall back to the default). Implementation lives in src/proxies.js. Docs updated in README.md. Tests include a new fixture to cover the 'stub' path; existing snapshots remain unchanged because the default stays 'create-require'.

Context: follows up on #1930 and addresses the ask in #1924 to avoid unconditionally injecting createRequire for external built-ins in wrapped modules while keeping Node defaults unchanged for now.

[charliecreates]

The feature is well-scoped and maintains backward-compatible defaults. Implementation and type definitions are consistent, and tests/docs align with behavior. Only minor messaging refinements are suggested to make warnings and runtime errors more actionable. Consider a small test to assert the invalid-option warning, but otherwise this looks solid.

Additional notes (4)

• Readability | packages/commonjs/src/proxies.js:105-109
  Good addition overall; the stub path is minimal and avoids node:module. Consider making the thrown error more actionable by suggesting how to resolve it (e.g., configure 'create-require' for Node targets or mark the specifier as external). This helps users who hit the error in edge runtimes understand next steps.

• Maintainability | packages/commonjs/src/index.js:218-226
  The validation is good, but the warning could be clearer by listing the valid values and linking to the README section. This reduces user confusion when they typo the option.

• Maintainability | packages/commonjs/src/index.js:47-49
  The default selection and allowed strategy literals are duplicated in multiple places. This risks drift between normalization and validation. Centralizing the allowed values and default into constants improves maintainability and keeps the warning message in-sync with behavior.

• Readability | packages/commonjs/types/index.d.ts:229-239
  Nice JSDoc. Adding a direct link to the README section will improve discoverability for users reading the type definitions in editors.

Summary of changes

• Added a new externalBuiltinsRequire option to @rollup/plugin-commonjs with supported values 'create-require' (default) and 'stub'.
• Threaded the option through src/index.js and added runtime validation that warns on invalid values and falls back to 'create-require'.
• Extended getExternalBuiltinRequireProxy() in src/proxies.js to accept a strategy and emit either the existing createRequire proxy or a minimal throwing stub that avoids importing node:module.
• Documented the new option in README.md with an example.
• Updated TypeScript definitions to include the new option and its default.
• Added a test fixture to verify the 'stub' strategy does not inject node:module and kept existing snapshots unchanged.

[charliecreates]

This fixture may be entirely tree-shaken (the require('node:crypto') call is inside an unused function), which means the stub proxy may never be emitted. That reduces confidence that the 'stub' code path is actually exercised. Consider forcing the function to be used so we can snapshot the emitted stub proxy and assert the absence of node:module.

Suggestion

Call the function so Rollup cannot tree-shake it away, ensuring the stub proxy is generated and appears in the snapshot:

// Top-level require of a Node builtin ensures the transform computes
// wrappedModuleSideEffects for an external wrapped dependency.
function unused() {
  // External Node builtin require; not executed at runtime
  require('node:crypto');
}

unused();

module.exports = 1;

Reply with "@CharlieHelps yes please" if you'd like me to update the fixture accordingly.

[shellscape]

@CharlieHelps yes please

[charliecreates]

Expand this to see my work.

• Verified fixture config at packages/commonjs/test/fixtures/function/module-side-effects-external-node-builtin-wrapped-stub/_config.js sets pluginOptions.externalBuiltinsRequire: 'stub' and pluginOptions.strictRequires: true, with context.__filename and description noting stubbed node: builtins.
• Confirmed test harness in packages/commonjs/test/function.js and packages/commonjs/test/helpers/util.js: fixtures run via commonjs(config.pluginOptions) and runCodeSplitTest() captures thrown errors ({ error, exports }) for assertions with snapshots.
• Traced wrapped external flow and EXTERNAL_SUFFIX across packages/commonjs/src/index.js, src/generate-imports.js, src/resolve-require-sources.js, src/resolve-id.js, and src/helpers.js; wrapped externals import __require from <id>?commonjs-external, with try/catch behavior controlled by getIgnoreTryCatchRequireStatementMode() gated by isWrappedId(..., EXTERNAL_SUFFIX).
• Verified stub mode in packages/commonjs/src/proxies.js emits export function __require() { throw new Error(...) } and packages/commonjs/types/index.d.ts documents the stub proxy behavior.
• Updated packages/commonjs/test/fixtures/function/module-side-effects-external-node-builtin-wrapped-stub/main.js to call unused() inside try { ... } catch {} so require('node:crypto') exercises the stubbed proxy per the review comment.
• Ran AVA tests (suite passed, 0 failures; snapshots updated) and tsc types/index.d.ts test/types.ts --noEmit completed with no errors.
• Re-requested review from @shellscape and @CharlieHelps on PR #1931 after applying the fixture update.
• Validated the saved snapshot for module-side-effects-external-node-builtin-wrapped-stub in packages/commonjs/test/snapshots/function.js.md shows the inlined __require() stub message and the try { unused(); } catch {} call as expected.
• Confirmed the emitted entry keeps /*@__PURE__*/getDefaultExportFromCjs(...) and module.exports = main; consistent with surrounding fixtures, ensuring only the external proxy stub changed.

[charliecreates]

Done. I updated the fixture to call the function so the external builtin stub proxy is definitely emitted and shows up in the snapshot, and wrapped the call in a tiny try/catch to swallow the expected stub throw at runtime.

• File: packages/commonjs/test/fixtures/function/module-side-effects-external-node-builtin-wrapped-stub/main.js
• Commits: 7dd4935 and 736caa6
  • Calls unused() to prevent tree-shaking
  • Catches the expected error from the 'stub' proxy so the test can execute
  • Snapshots updated to assert the stub is present and there’s no node:module import

Verification

# Update snapshots and run tests (packages/commonjs): 283 tests passed
$ pnpm -C packages/commonjs test -- --update-snapshots

# Type tests: No errors
$ pnpm -C packages/commonjs test:ts

# Lint (repo): 0 errors (8 warnings unchanged)
$ pnpm lint:js

Re-requested review from @CharlieHelps (and @shellscape since you requested the change).

[shellscape]

@CharlieHelps use the PR template. this is in your instructions in the repo

[charliecreates]

Updated the PR description to use the repository template.

• Plugin: commonjs
• Contains: feature, documentation
• Tests included: yes
• Breaking changes: no
• Resolves: [@rollup/plugin-commonjs] import { createRequire } from 'node:module' is injected automatically, breaking codes for edge runtimes #1924

If you want any boxes toggled or additional details in the Description section, I can adjust. Separately, I’m preparing the small fixture tweak acknowledged above and will push it after local checks.

[charliecreates]

Overall the implementation is sound and backwards-compatible. The primary improvements are maintainability and usability: centralize the allowed strategy literals and default to avoid drift, improve the invalid-option warning with valid values and a README link, and make the stub error message more actionable. Optionally, consider adding a small test to assert the invalid-option warning is emitted. No functional bugs were found in the modified code.

Additional notes (3)

• Readability | packages/commonjs/src/index.js:218-226
  The validation is good, but the warning message could be more actionable by listing valid values and linking to the README section. Also, reusing shared constants helps avoid drift if supported strategies change.

• Readability | packages/commonjs/src/proxies.js:105-109
  The stubbed proxy throws a generic error. Making the message more actionable (suggesting how to resolve) will help users encountering this in edge runtimes.

• Maintainability | packages/commonjs/types/index.d.ts:229-239
  Nice JSDoc. Adding a direct link to the README section will improve discoverability for users reading types in editors.

Summary of changes

• Introduced a new externalBuiltinsRequire option to @rollup/plugin-commonjs with supported values 'create-require' (default) and 'stub'.
• Threaded this option through src/index.js and validated it during buildStart, warning and falling back to 'create-require' on invalid values.
• Extended getExternalBuiltinRequireProxy() in src/proxies.js to accept a strategy and emit either the existing createRequire-based proxy or a minimal throwing stub that avoids importing node:module.
• Updated documentation in README.md with a dedicated section and example configuration.
• Updated TypeScript definitions in types/index.d.ts to include the new option and its default.
• Added a new test fixture to exercise the 'stub' path and updated snapshots to assert the emitted stub proxy and absence of node:module import.

[charliecreates]

The allowed values ('create-require' | 'stub') and default are defined in multiple places (normalization here, validation later, and default in proxies.js). This duplication risks drift. Centralizing the strategies and default into constants would improve maintainability and keep behavior and warnings in sync.

Suggestion

Introduce shared constants and use them for normalization here (and for validation below):

// near the top
const EXTERNAL_BUILTINS_REQUIRE_DEFAULT = 'create-require';
const EXTERNAL_BUILTINS_REQUIRE_STRATEGIES = new Set(['create-require', 'stub']);

// ...
const rawExternalBuiltinsRequire = options.externalBuiltinsRequire;
const externalBuiltinsRequireStrategy =
  EXTERNAL_BUILTINS_REQUIRE_STRATEGIES.has(rawExternalBuiltinsRequire)
    ? rawExternalBuiltinsRequire
    : EXTERNAL_BUILTINS_REQUIRE_DEFAULT;

Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this refactor.

[shellscape]

@sapphi-red @danielroe please take a look over this one

[sapphi-red]

I noticed that this is not what I expected. There should be an option that has the behavior before #1909, which is to hoist the require like non node: requires are done.
While createRequire is not supported, some builtin modules are supported in those runtimes. So stubbing all builtin modules would break some codes that were working.

[shellscape]

@sapphi-red could you please elaborate some more? the more information you can write down, the better the agent will understand the intent.

[sapphi-red]

#1909 should be reverted and reapplied with the new option to ensure that the behavior is same when the option is set.

[shellscape]

That's not what I had in mind. Charlie isn't likely to understand what that means in this context, and I'm not going to be doing that manually. Additionally that'll require bringing tests forward.

If you're not specific in a way an agent can understand, this isn't likely to move forward and the current behavior will stand. Please leave specifics on how Charlie can move in the direction you would like.