feat(commonjs)!: Add ignore-dynamic-requires option (#819)

lukastaegert · web-flow · commit 2d06c44874c1 · 2021-03-10T10:23:36.000-05:00
BREAKING CHANGES:
When dynamicRequireTargets is specified, "require" will be default no longer be used as fallback
diff --git a/packages/commonjs/README.md b/packages/commonjs/README.md
@@ -134,6 +134,25 @@ Due to the conversion of `require` to a static `import` - the call is hoisted to
 - `string[]`: Pass an array containing the IDs to left unconverted.
 - `((id: string) => boolean|'remove')`: Pass a function that control individual IDs.
 
+### `ignoreDynamicRequires`
+
+Type: `boolean`
+Default: false
+
+Some `require` calls cannot be resolved statically to be translated to imports, e.g.
+
+```js
+function wrappedRequire(target) {
+  return require(target);
+}
+wrappedRequire('foo');
+wrappedRequire('bar');
+```
+
+When this option is set to `false`, the generated code will either directly throw an error when such a call is encountered or, when `dynamicRequireTargets` is used, when such a call cannot be resolved with a configured dynamic require target.
+
+Setting this option to `true` will instead leave the `require` call in the code or use it as a fallback for `dynamicRequireTargets`.
+
 ### `esmExternals`
 
 Type: `boolean | string[] | ((id: string) => boolean)`
diff --git a/packages/commonjs/src/helpers.js b/packages/commonjs/src/helpers.js
@@ -48,18 +48,20 @@ export function getAugmentedNamespace(n) {
 }
 `;
 
+const FAILED_REQUIRE_ERROR = `throw new Error('Could not dynamically require "' + path + '". Please configure the dynamicRequireTargets or/and ignoreDynamicRequires option of @rollup/plugin-commonjs appropriately for this require call to work.');`;
+
 const HELPER_NON_DYNAMIC = `
 export function createCommonjsModule(fn) {
   var module = { exports: {} }
 	return fn(module, module.exports), module.exports;
 }
 
-export function commonjsRequire (target) {
-	throw new Error('Could not dynamically require "' + target + '". Please configure the dynamicRequireTargets option of @rollup/plugin-commonjs appropriately for this require call to behave properly.');
+export function commonjsRequire (path) {
+	${FAILED_REQUIRE_ERROR}
 }
 `;
 
-const HELPERS_DYNAMIC = `
+const getDynamicHelpers = (ignoreDynamicRequires) => `
 export function createCommonjsModule(fn, basedir, module) {
 	return module = {
 		path: basedir,
@@ -231,13 +233,15 @@ export function commonjsRequire (path, originalModuleDir) {
       return cachedModule.exports;
     };
 	}
-	return require(path);
+	${ignoreDynamicRequires ? 'return require(path);' : FAILED_REQUIRE_ERROR}
 }
 
 commonjsRequire.cache = DYNAMIC_REQUIRE_CACHE;
 commonjsRequire.resolve = commonjsResolve;
 `;
 
-export function getHelpersModule(isDynamicRequireModulesEnabled) {
-  return `${HELPERS}${isDynamicRequireModulesEnabled ? HELPERS_DYNAMIC : HELPER_NON_DYNAMIC}`;
+export function getHelpersModule(isDynamicRequireModulesEnabled, ignoreDynamicRequires) {
+  return `${HELPERS}${
+    isDynamicRequireModulesEnabled ? getDynamicHelpers(ignoreDynamicRequires) : HELPER_NON_DYNAMIC
+  }`;
 }
diff --git a/packages/commonjs/src/index.js b/packages/commonjs/src/index.js
@@ -44,6 +44,7 @@ export default function commonjs(options = {}) {
   const filter = createFilter(options.include, options.exclude);
   const {
     ignoreGlobal,
+    ignoreDynamicRequires,
     requireReturnsDefault: requireReturnsDefaultOption,
     esmExternals
   } = options;
@@ -97,6 +98,7 @@ export default function commonjs(options = {}) {
 
   function transformAndCheckExports(code, id) {
     if (isDynamicRequireModulesEnabled && this.getModuleInfo(id).isEntry) {
+      // eslint-disable-next-line no-param-reassign
       code =
         getDynamicPackagesEntryIntro(dynamicRequireModuleDirPaths, dynamicRequireModuleSet) + code;
     }
@@ -125,6 +127,7 @@ export default function commonjs(options = {}) {
     // avoid wrapping in createCommonjsModule, as this is a commonjsRegister call
     if (isModuleRegisterProxy(id)) {
       disableWrap = true;
+      // eslint-disable-next-line no-param-reassign
       id = unwrapModuleRegisterProxy(id);
     }
 
@@ -135,6 +138,7 @@ export default function commonjs(options = {}) {
       isEsModule,
       ignoreGlobal || isEsModule,
       ignoreRequire,
+      ignoreDynamicRequires && !isDynamicRequireModulesEnabled,
       getIgnoreTryCatchRequireStatementMode,
       sourceMap,
       isDynamicRequireModulesEnabled,
@@ -161,7 +165,7 @@ export default function commonjs(options = {}) {
 
     load(id) {
       if (id === HELPERS_ID) {
-        return getHelpersModule(isDynamicRequireModulesEnabled);
+        return getHelpersModule(isDynamicRequireModulesEnabled, ignoreDynamicRequires);
       }
 
       if (id.startsWith(HELPERS_ID)) {
@@ -232,6 +236,7 @@ export default function commonjs(options = {}) {
       }
     },
 
+    // eslint-disable-next-line no-shadow
     moduleParsed({ id, meta: { commonjs } }) {
       if (commonjs) {
         const isCjs = commonjs.isCommonJS;
diff --git a/packages/commonjs/src/transform-commonjs.js b/packages/commonjs/src/transform-commonjs.js
@@ -46,6 +46,7 @@ export default function transformCommonjs(
   isEsModule,
   ignoreGlobal,
   ignoreRequire,
+  ignoreDynamicRequires,
   getIgnoreTryCatchRequireStatementMode,
   sourceMap,
   isDynamicRequireModulesEnabled,
@@ -320,12 +321,14 @@ export default function transformCommonjs(
                   )}`
                 );
               }
-              if (isShorthandProperty(parent)) {
-                magicString.appendRight(node.end, `: ${HELPERS_NAME}.commonjsRequire`);
-              } else {
-                magicString.overwrite(node.start, node.end, `${HELPERS_NAME}.commonjsRequire`, {
-                  storeName: true
-                });
+              if (!ignoreDynamicRequires) {
+                if (isShorthandProperty(parent)) {
+                  magicString.appendRight(node.end, `: ${HELPERS_NAME}.commonjsRequire`);
+                } else {
+                  magicString.overwrite(node.start, node.end, `${HELPERS_NAME}.commonjsRequire`, {
+                    storeName: true
+                  });
+                }
               }
 
               uses.commonjsHelpers = true;
diff --git a/packages/commonjs/test/fixtures/function/dynamic-require-absolute-paths/_config.js b/packages/commonjs/test/fixtures/function/dynamic-require-absolute-paths/_config.js
diff --git a/packages/commonjs/test/fixtures/function/dynamic-require-absolute-paths/main.js b/packages/commonjs/test/fixtures/function/dynamic-require-absolute-paths/main.js
diff --git a/packages/commonjs/test/fixtures/function/dynamic-require-absolute-paths/submodule.js b/packages/commonjs/test/fixtures/function/dynamic-require-absolute-paths/submodule.js
diff --git a/packages/commonjs/test/fixtures/function/dynamic-require-fallback/_config.js b/packages/commonjs/test/fixtures/function/dynamic-require-fallback/_config.js
@@ -0,0 +1,6 @@
+module.exports = {
+  description: 'keeps a require call as fallback when configured',
+  pluginOptions: {
+    ignoreDynamicRequires: true
+  }
+};
diff --git a/packages/commonjs/test/fixtures/function/dynamic-require-fallback/dep.js b/packages/commonjs/test/fixtures/function/dynamic-require-fallback/dep.js
@@ -0,0 +1 @@
+module.exports = 'dep';
diff --git a/packages/commonjs/test/fixtures/function/dynamic-require-fallback/main.js b/packages/commonjs/test/fixtures/function/dynamic-require-fallback/main.js
@@ -0,0 +1,8 @@
+/* eslint-disable import/no-dynamic-require, global-require */
+
+function takeModule(withName) {
+  return require(withName);
+}
+
+// The bundled code will run from test/helpers/util.js
+t.is(takeModule('../fixtures/function/dynamic-require-fallback/dep.js'), 'dep');
diff --git a/packages/commonjs/test/fixtures/function/dynamic-require-globs/main.js b/packages/commonjs/test/fixtures/function/dynamic-require-globs/main.js
@@ -7,12 +7,7 @@ function takeModule(withName) {
 t.is(takeModule('submodule1.js'), 'submodule1');
 t.is(takeModule('submodule2.js'), 'submodule2');
 t.is(takeModule('extramodule1.js'), 'extramodule1');
-
-let hasThrown = false;
-try {
-  takeModule('extramodule2.js');
-} catch (error) {
-  t.truthy(/Cannot find module '\.\/extramodule2\.js'/.test(error.message));
-  hasThrown = true;
-}
-t.truthy(hasThrown);
+t.throws(() => takeModule('extramodule2.js'), {
+  message:
+    'Could not dynamically require "./extramodule2.js". Please configure the dynamicRequireTargets or/and ignoreDynamicRequires option of @rollup/plugin-commonjs appropriately for this require call to work.'
+});
diff --git a/packages/commonjs/test/fixtures/function/dynamic-require-no-fallback/_config.js b/packages/commonjs/test/fixtures/function/dynamic-require-no-fallback/_config.js
@@ -0,0 +1,3 @@
+module.exports = {
+  description: 'throws when there is no fallback for a dynamic require call'
+};
diff --git a/packages/commonjs/test/fixtures/function/dynamic-require-no-fallback/main.js b/packages/commonjs/test/fixtures/function/dynamic-require-no-fallback/main.js
@@ -0,0 +1,10 @@
+/* eslint-disable import/no-dynamic-require, global-require */
+
+function takeModule(withName) {
+  return require(withName);
+}
+
+t.throws(() => takeModule('./dep.js'), {
+  message:
+    'Could not dynamically require "./dep.js". Please configure the dynamicRequireTargets or/and ignoreDynamicRequires option of @rollup/plugin-commonjs appropriately for this require call to work.'
+});
diff --git a/packages/commonjs/test/fixtures/function/dynamic-require-targets-fallback/_config.js b/packages/commonjs/test/fixtures/function/dynamic-require-targets-fallback/_config.js
@@ -0,0 +1,7 @@
+module.exports = {
+  description: 'keeps a require call as fallback when configured for dynamicRequireTargets',
+  pluginOptions: {
+    ignoreDynamicRequires: true,
+    dynamicRequireTargets: ['fixtures/function/dynamic-require-targets-fallback/dep1.js']
+  }
+};
diff --git a/packages/commonjs/test/fixtures/function/dynamic-require-targets-fallback/dep1.js b/packages/commonjs/test/fixtures/function/dynamic-require-targets-fallback/dep1.js
@@ -0,0 +1 @@
+module.exports = 'dep';
diff --git a/packages/commonjs/test/fixtures/function/dynamic-require-targets-fallback/dep2.js b/packages/commonjs/test/fixtures/function/dynamic-require-targets-fallback/dep2.js
@@ -0,0 +1 @@
+module.exports = 'dep';
diff --git a/packages/commonjs/test/fixtures/function/dynamic-require-targets-fallback/main.js b/packages/commonjs/test/fixtures/function/dynamic-require-targets-fallback/main.js
@@ -0,0 +1,9 @@
+/* eslint-disable import/no-dynamic-require, global-require */
+
+function takeModule(withName) {
+  return require(withName);
+}
+
+t.is(takeModule('./dep1.js'), 'dep');
+// The bundled code will run from test/helpers/util.js
+t.is(takeModule('../fixtures/function/dynamic-require-targets-fallback/dep2.js'), 'dep');
diff --git a/packages/commonjs/test/fixtures/function/dynamic-require-targets-no-fallback/_config.js b/packages/commonjs/test/fixtures/function/dynamic-require-targets-no-fallback/_config.js
@@ -0,0 +1,7 @@
+module.exports = {
+  description:
+    'throws when there is no require call configured as fallback for dynamicRequireTargets',
+  pluginOptions: {
+    dynamicRequireTargets: ['fixtures/function/dynamic-require-targets-no-fallback/dep1.js']
+  }
+};
diff --git a/packages/commonjs/test/fixtures/function/dynamic-require-targets-no-fallback/dep1.js b/packages/commonjs/test/fixtures/function/dynamic-require-targets-no-fallback/dep1.js
@@ -0,0 +1 @@
+module.exports = 'dep';
diff --git a/packages/commonjs/test/fixtures/function/dynamic-require-targets-no-fallback/main.js b/packages/commonjs/test/fixtures/function/dynamic-require-targets-no-fallback/main.js
@@ -0,0 +1,11 @@
+/* eslint-disable import/no-dynamic-require, global-require */
+
+function takeModule(withName) {
+  return require(withName);
+}
+
+t.is(takeModule('./dep1.js'), 'dep');
+t.throws(() => takeModule('./dep2.js'), {
+  message:
+    'Could not dynamically require "./dep2.js". Please configure the dynamicRequireTargets or/and ignoreDynamicRequires option of @rollup/plugin-commonjs appropriately for this require call to work.'
+});
diff --git a/packages/commonjs/test/fixtures/function/global-not-overwritten/encode.js b/packages/commonjs/test/fixtures/function/global-not-overwritten/encode.js
@@ -1,4 +1,4 @@
-exports.encodeURIComponent = function() {
+exports.encodeURIComponent = function () {
   return encodeURIComponent(this.str);
 };
 
diff --git a/packages/commonjs/test/fixtures/function/inline/main.js b/packages/commonjs/test/fixtures/function/inline/main.js
@@ -1,4 +1,4 @@
 /* eslint-disable global-require */
-module.exports = function() {
+module.exports = function () {
   return require('./multiply')(2, require('./foo'));
 };
diff --git a/packages/commonjs/test/fixtures/function/inline/multiply.js b/packages/commonjs/test/fixtures/function/inline/multiply.js
@@ -1,3 +1,3 @@
-module.exports = function(a, b) {
+module.exports = function (a, b) {
   return a * b;
 };
diff --git a/packages/commonjs/test/fixtures/function/toplevel-return-complex/bar.js b/packages/commonjs/test/fixtures/function/toplevel-return-complex/bar.js
@@ -1,3 +1,3 @@
-module.exports = function() {
+module.exports = function () {
   return true;
 };
diff --git a/packages/commonjs/test/fixtures/function/unsupported-dynamic-require/_config.js b/packages/commonjs/test/fixtures/function/unsupported-dynamic-require/_config.js
diff --git a/packages/commonjs/test/fixtures/function/unsupported-dynamic-require/main.js b/packages/commonjs/test/fixtures/function/unsupported-dynamic-require/main.js
diff --git a/packages/commonjs/test/fixtures/samples/dynamic-require-different-loader/submodule2.js b/packages/commonjs/test/fixtures/samples/dynamic-require-different-loader/submodule2.js
@@ -1,3 +1,3 @@
-module.exports = function() {
+module.exports = function () {
   return 'Hello there';
 };
diff --git a/packages/commonjs/test/fixtures/samples/umd/correct-scoping.js b/packages/commonjs/test/fixtures/samples/umd/correct-scoping.js
@@ -1,5 +1,5 @@
 if (typeof require === 'function') {
-  module.exports = (function(require) {
+  module.exports = (function (require) {
     return typeof require;
   })({});
 }
diff --git a/packages/commonjs/test/form.js b/packages/commonjs/test/form.js
@@ -74,8 +74,15 @@ readdirSync('./fixtures/form').forEach((dir) => {
       // this will benefit issues like `form/try-catch-remove` where whitespace is left in the line,
       // and testing on windows (\r\n)
       t.is(
-        actual.split('\n').map(x => x.trimEnd()).join('\n'),
-        expected.split('\n').map(x => x.trimEnd()).join('\n'));
+        actual
+          .split('\n')
+          .map((x) => x.trimEnd())
+          .join('\n'),
+        expected
+          .split('\n')
+          .map((x) => x.trimEnd())
+          .join('\n')
+      );
     }
   });
 });
diff --git a/packages/commonjs/test/snapshots/function.js.md b/packages/commonjs/test/snapshots/function.js.md
diff --git a/packages/commonjs/test/snapshots/function.js.snap b/packages/commonjs/test/snapshots/function.js.snap
diff --git a/packages/commonjs/test/types.ts b/packages/commonjs/test/types.ts
diff --git a/packages/commonjs/types/index.d.ts b/packages/commonjs/types/index.d.ts
