feat(commonjs)!: revert #1909 and add requireNodeBuiltins option (#1937)

* Initial plan

* Add requireNodeBuiltins option to control createRequire injection

Co-authored-by: sapphi-red <[email protected]>

* Address code review feedback - use explicit boolean checks

Co-authored-by: sapphi-red <[email protected]>

* Add requireNodeBuiltins: false variant tests for 3 fixtures

Co-authored-by: sapphi-red <[email protected]>

* Generate snapshots for requireNodeBuiltins: false variant tests

Co-authored-by: sapphi-red <[email protected]>

---------

Co-authored-by: copilot-swe-agent[bot] <[email protected]>

Author: sapphi-red

packages/commonjs/README.md

@@ -63,6 +63,34 @@ You can also provide a [picomatch pattern](https://github.com/micromatch/picomat
 
 `"debug"` works like `"auto"` but after bundling, it will display a warning containing a list of ids that have been wrapped which can be used as picomatch pattern for fine-tuning or to avoid the potential race conditions mentioned for `"auto"`.
 
+### `requireNodeBuiltins`
+
+Type: `boolean`<br>
+Default: `false`
+
+When enabled, external Node built-ins (e.g., `node:fs`, `node:path`) required from wrapped CommonJS modules will use `createRequire(import.meta.url)` instead of being hoisted as ESM imports. This prevents eager loading of Node built-ins at module initialization time and preserves the lazy execution semantics of `require()`.
+
+**Important:** Enabling this option adds a dependency on `node:module` in the output bundle, which may not be available in some environments like edge runtimes (Cloudflare Workers, Vercel Edge Runtime). Only enable this option if you are targeting Node.js environments and need the lazy loading behavior for Node built-ins.
+
+Example:
+
+```js
+commonjs({
+  strictRequires: true,
+  requireNodeBuiltins: true
+});
+```
+
+With `requireNodeBuiltins: true`, code like:
+
+```js
+if (condition) {
+  require('node:fs');
+}
+```
+
+will generate output using `createRequire` instead of hoisting the import to the top of the file.
+
 ### `dynamicRequireTargets`
 
 Type: `string | string[]`<br>

packages/commonjs/src/index.js

@@ -42,7 +42,8 @@ export default function commonjs(options = {}) {
     ignoreDynamicRequires,
     requireReturnsDefault: requireReturnsDefaultOption,
     defaultIsModuleExports: defaultIsModuleExportsOption,
-    esmExternals
+    esmExternals,
+    requireNodeBuiltins = false
   } = options;
   const extensions = options.extensions || ['.js'];
   const filter = createFilter(options.include, options.exclude);
@@ -215,7 +216,8 @@ export default function commonjs(options = {}) {
       requireResolver = getRequireResolver(
         extensions,
         detectCyclesAndConditional,
-        currentlyResolving
+        currentlyResolving,
+        requireNodeBuiltins
       );
     },
 
@@ -263,7 +265,7 @@ export default function commonjs(options = {}) {
 
       if (isWrappedId(id, EXTERNAL_SUFFIX)) {
         const actualId = unwrapId(id, EXTERNAL_SUFFIX);
-        if (actualId.startsWith('node:')) {
+        if (requireNodeBuiltins === true && actualId.startsWith('node:')) {
           return getExternalBuiltinRequireProxy(actualId);
         }
         return getUnknownRequireProxy(

packages/commonjs/src/resolve-require-sources.js

@@ -10,7 +10,12 @@ import {
 } from './helpers';
 import { resolveExtensions } from './resolve-id';
 
-export function getRequireResolver(extensions, detectCyclesAndConditional, currentlyResolving) {
+export function getRequireResolver(
+  extensions,
+  detectCyclesAndConditional,
+  currentlyResolving,
+  requireNodeBuiltins
+) {
   const knownCjsModuleTypes = Object.create(null);
   const requiredIds = Object.create(null);
   const unconditionallyRequiredIds = Object.create(null);
@@ -195,21 +200,24 @@ export function getRequireResolver(extensions, detectCyclesAndConditional, curre
             getTypeForFullyAnalyzedModule(dependencyId));
           // Special-case external Node built-ins to be handled via a lazy __require
           // helper instead of hoisted ESM imports when strict wrapping is used.
+          // Only apply this when requireNodeBuiltins option is enabled.
           const isExternalWrapped = isWrappedId(dependencyId, EXTERNAL_SUFFIX);
           let resolvedDependencyId = dependencyId;
-          if (parentMeta.isCommonJS === IS_WRAPPED_COMMONJS && !allowProxy && isExternalWrapped) {
-            const actualExternalId = unwrapId(dependencyId, EXTERNAL_SUFFIX);
-            if (actualExternalId.startsWith('node:')) {
-              isCommonJS = IS_WRAPPED_COMMONJS;
-              parentMeta.isRequiredCommonJS[dependencyId] = isCommonJS;
-            }
-          } else if (isExternalWrapped && !allowProxy) {
-            // If the parent is not wrapped but the dependency is a node: builtin external,
-            // unwrap the EXTERNAL_SUFFIX so it's treated as a normal external.
-            // This avoids trying to load the lazy __require proxy for non-wrapped contexts.
-            const actualExternalId = unwrapId(dependencyId, EXTERNAL_SUFFIX);
-            if (actualExternalId.startsWith('node:')) {
-              resolvedDependencyId = actualExternalId;
+          if (requireNodeBuiltins === true) {
+            if (parentMeta.isCommonJS === IS_WRAPPED_COMMONJS && !allowProxy && isExternalWrapped) {
+              const actualExternalId = unwrapId(dependencyId, EXTERNAL_SUFFIX);
+              if (actualExternalId.startsWith('node:')) {
+                isCommonJS = IS_WRAPPED_COMMONJS;
+                parentMeta.isRequiredCommonJS[dependencyId] = isCommonJS;
+              }
+            } else if (isExternalWrapped && !allowProxy) {
+              // If the parent is not wrapped but the dependency is a node: builtin external,
+              // unwrap the EXTERNAL_SUFFIX so it's treated as a normal external.
+              // This avoids trying to load the lazy __require proxy for non-wrapped contexts.
+              const actualExternalId = unwrapId(dependencyId, EXTERNAL_SUFFIX);
+              if (actualExternalId.startsWith('node:')) {
+                resolvedDependencyId = actualExternalId;
+              }
             }
           }
           const isWrappedCommonJS = isCommonJS === IS_WRAPPED_COMMONJS;

packages/commonjs/test/fixtures/function/module-side-effects-external-node-builtin-wrapped-default/_config.js

@@ -0,0 +1,11 @@
+module.exports = {
+  description:
+    'does not crash and does not mark external node: builtins as pure when strictRequires is true and requireNodeBuiltins is false (default)',
+  pluginOptions: {
+    strictRequires: true,
+    requireNodeBuiltins: false
+  },
+  context: {
+    __filename: __filename
+  }
+};

packages/commonjs/test/fixtures/function/module-side-effects-external-node-builtin-wrapped-default/main.js

@@ -0,0 +1,8 @@
+// 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');
+}
+
+module.exports = 1;

packages/commonjs/test/fixtures/function/module-side-effects-external-node-builtin-wrapped/_config.js

@@ -2,7 +2,8 @@ module.exports = {
   description:
     'does not crash and does not mark external node: builtins as pure when strictRequires is true',
   pluginOptions: {
-    strictRequires: true
+    strictRequires: true,
+    requireNodeBuiltins: true
   },
   context: {
     __filename: __filename

packages/commonjs/test/fixtures/function/strict-requires-auto-external-node-builtin-default/_config.js

@@ -0,0 +1,14 @@
+module.exports = {
+  description: 'handles node: builtins correctly with strictRequires: auto and requireNodeBuiltins: false (default)',
+  pluginOptions: {
+    strictRequires: 'auto',
+    requireNodeBuiltins: false
+  },
+  exports: (exports, t) => {
+    // Should be able to access properties of node:stream
+    t.truthy(exports.Readable);
+    t.is(typeof exports.Readable, 'function');
+    // Should be able to instantiate
+    t.truthy(exports.readable);
+  }
+};

packages/commonjs/test/fixtures/function/strict-requires-auto-external-node-builtin-default/main.js

@@ -0,0 +1,4 @@
+const stream = require('node:stream');
+const readable = new stream.Readable({});
+
+module.exports = { Readable: stream.Readable, readable };

packages/commonjs/test/fixtures/function/strict-requires-auto-external-node-builtin/_config.js

@@ -1,7 +1,8 @@
 module.exports = {
   description: 'handles node: builtins correctly with strictRequires: auto',
   pluginOptions: {
-    strictRequires: 'auto'
+    strictRequires: 'auto',
+    requireNodeBuiltins: true
   },
   exports: (exports, t) => {
     // Should be able to access properties of node:stream

packages/commonjs/test/fixtures/function/strict-requires-external-node-builtin-default/_config.js

@@ -0,0 +1,10 @@
+module.exports = {
+  description: "hoists external node built-in requires when requireNodeBuiltins is false (default)",
+  pluginOptions: {
+    strictRequires: true,
+    requireNodeBuiltins: false
+  },
+  exports: (exports, t) => {
+    t.is(exports, 42);
+  }
+};