feat(commonjs): add kill-switch for mixed es-cjs modules (rollup#358)

BREAKING CHANGE: Reverts default behavior of mixed es and cjs modules. Marked as a "breaking change", but the PR that raised this need was actually a breaking change for people who did not utilize include/exclude correctly and have variety of dependencies with mixed UMD mechanisms. * Implemented kill-switch for mixed commonjs in mixed es-cjs (Closes rollup#348, rollup#342) `transformMixedEsModules = false` * Update packages/commonjs/README.md Co-authored-by: Andrew Powell <shellscape@users.noreply.github.com> * Update index.d.ts Co-authored-by: Andrew Powell <shellscape@users.noreply.github.com>
LarsDenBakker · Sep 12, 2020 · b3acf74 · b3acf74
1 parent f6e3825
commit b3acf74
Show file tree

Hide file tree

Showing 10 changed files with 41 additions and 5 deletions.
diff --git a/packages/commonjs/README.md b/packages/commonjs/README.md
@@ -106,6 +106,13 @@ Default: `true`
 
 If false, skips source map generation for CommonJS modules.
 
+### `transformMixedEsModules`
+
+Type: `Boolean`<br>
+Default: `false`
+
+Instructs the plugin whether or not to enable mixed module transformations. This is useful in scenarios with mixed ES and CommonJS modules. Set to `true` if it's known that `require` calls should be transformed, or `false` if the code contains env detection and the `require` should survive a transformation.
+
 ### `ignore`
 
 Type: `Array[...String | (String) => Boolean]`<br>

diff --git a/packages/commonjs/src/index.js b/packages/commonjs/src/index.js
@@ -66,10 +66,14 @@ export default function commonjs(options = {}) {
 
     const isDynamicRequireModule = dynamicRequireModuleSet.has(normalizePathSlashes(id));
 
-    if (isEsModule && !isDynamicRequireModule) {
+    if (isEsModule && (!isDynamicRequireModule || !options.transformMixedEsModules)) {
       (hasDefaultExport ? esModulesWithDefaultExport : esModulesWithoutDefaultExport).add(id);
+      if (!options.transformMixedEsModules) {
+        setIsCjsPromise(id, false);
+        return null;
+      }
     }
-    // it is not an ES module but it does not have CJS-specific elements.
+    // it is not an ES module AND it does not have CJS-specific elements.
     else if (!hasCjsKeywords(code, ignoreGlobal)) {
       esModulesWithoutDefaultExport.add(id);
       setIsCjsPromise(id, false);

diff --git a/packages/commonjs/test/fixtures/form/unambiguous-with-default-export/_config.js b/packages/commonjs/test/fixtures/form/unambiguous-with-default-export/_config.js
@@ -0,0 +1,5 @@
+module.exports = {
+  options: {
+    transformMixedEsModules: true
+  }
+};
diff --git a/packages/commonjs/test/fixtures/form/unambiguous-with-import/_config.js b/packages/commonjs/test/fixtures/form/unambiguous-with-import/_config.js
@@ -0,0 +1,5 @@
+module.exports = {
+  options: {
+    transformMixedEsModules: true
+  }
+};
diff --git a/packages/commonjs/test/fixtures/form/unambiguous-with-named-export/_config.js b/packages/commonjs/test/fixtures/form/unambiguous-with-named-export/_config.js
@@ -0,0 +1,5 @@
+module.exports = {
+  options: {
+    transformMixedEsModules: true
+  }
+};
diff --git a/packages/commonjs/test/fixtures/function/dynamic-require-code-splitting/_config.js b/packages/commonjs/test/fixtures/function/dynamic-require-code-splitting/_config.js
@@ -7,6 +7,7 @@ module.exports = {
     ]
   },
   pluginOptions: {
-    dynamicRequireTargets: ['fixtures/function/dynamic-require-code-splitting/target?.js']
+    dynamicRequireTargets: ['fixtures/function/dynamic-require-code-splitting/target?.js'],
+    transformMixedEsModules: true
   }
 };
diff --git a/packages/commonjs/test/fixtures/function/dynamic-require-es-entry/_config.js b/packages/commonjs/test/fixtures/function/dynamic-require-es-entry/_config.js
@@ -1,6 +1,7 @@
 module.exports = {
   description: 'works when the entry point is an es module',
   pluginOptions: {
-    dynamicRequireTargets: ['fixtures/function/dynamic-require-es-entry/submodule.js']
+    dynamicRequireTargets: ['fixtures/function/dynamic-require-es-entry/submodule.js'],
+    transformMixedEsModules: true
   }
 };
diff --git a/packages/commonjs/test/fixtures/function/es6-export-with-global-sniffing/_config.js b/packages/commonjs/test/fixtures/function/es6-export-with-global-sniffing/_config.js
@@ -4,5 +4,7 @@ module.exports = {
   options: {
     plugins: [nodeResolve()]
   },
-  pluginOptions: {}
+  pluginOptions: {
+    transformMixedEsModules: true
+  }
 };
diff --git a/packages/commonjs/test/types.ts b/packages/commonjs/test/types.ts
@@ -13,6 +13,7 @@ const config: import('rollup').RollupOptions = {
       extensions: ['.js', '.coffee'],
       ignoreGlobal: false,
       sourceMap: false,
+      transformMixedEsModules: false,
       ignore: ['conditional-runtime-dependency'],
       dynamicRequireTargets: ['node_modules/logform/*.js']
     })

diff --git a/packages/commonjs/types/index.d.ts b/packages/commonjs/types/index.d.ts
@@ -30,6 +30,11 @@ interface RollupCommonJSOptions {
    * @default true
    */
   sourceMap?: boolean;
+  /**
+   * Instructs the plugin whether or not to enable mixed module transformations. This is useful in scenarios with mixed ES and CommonJS modules. Set to `true` if it's known that `require` calls should be transformed, or `false` if the code contains env detection and the `require` should survive a transformation.
+   * @default false
+   */
+  transformMixedEsModules?: boolean;
   /**
    * explicitly specify unresolvable named exports
    * ([see below for more details](https://github.com/rollup/plugins/tree/master/packages/commonjs#named-exports))