fix(commonjs): improved shouldWrap logic. fixes rollup#304 (rollup#355)

packages/commonjs/src/transform.js

@@ -141,7 +141,7 @@ export function transformCommonjs(
 
   // TODO handle transpiled modules
   let shouldWrap = /__esModule/.test(code);
-  let usesDynamicHelpers = false;
+  let usesCommonjsHelpers = false;
 
   function isRequireStatement(node) {
     if (!node) return false;
@@ -316,10 +316,12 @@ export function transformCommonjs(
       // rewrite `this` as `commonjsHelpers.commonjsGlobal`
       if (node.type === 'ThisExpression' && lexicalDepth === 0) {
         uses.global = true;
-        if (!ignoreGlobal)
+        if (!ignoreGlobal) {
           magicString.overwrite(node.start, node.end, `${HELPERS_NAME}.commonjsGlobal`, {
             storeName: true
           });
+          usesCommonjsHelpers = true;
+        }
         return;
       }
 
@@ -366,14 +368,15 @@ export function transformCommonjs(
               magicString.overwrite(node.start, node.end, `${HELPERS_NAME}.commonjsRequire`, {
                 storeName: true
               });
-              usesDynamicHelpers = true;
+              usesCommonjsHelpers = true;
             }
 
             uses[node.name] = true;
             if (node.name === 'global' && !ignoreGlobal) {
               magicString.overwrite(node.start, node.end, `${HELPERS_NAME}.commonjsGlobal`, {
                 storeName: true
               });
+              usesCommonjsHelpers = true;
             }
 
             // if module or exports are used outside the context of an assignment
@@ -474,7 +477,7 @@ export function transformCommonjs(
                 : getVirtualPathForDynamicRequirePath(normalizePathSlashes(dirname(id)), commonDir)
             )})`
           );
-          usesDynamicHelpers = true;
+          usesCommonjsHelpers = true;
         } else {
           magicString.overwrite(node.start, node.end, required.name);
         }
@@ -530,8 +533,13 @@ export function transformCommonjs(
     return null;
   }
 
-  const includeHelpers = usesDynamicHelpers || shouldWrap || uses.global || uses.require;
-  const importBlock = `${(includeHelpers
+  // If `isEsModule` is on, it means it has ES6 import/export statements,
+  //   which just can't be wrapped in a function.
+  if (isEsModule) shouldWrap = false;
+
+  usesCommonjsHelpers = usesCommonjsHelpers || shouldWrap;
+
+  const importBlock = `${(usesCommonjsHelpers
     ? [`import * as ${HELPERS_NAME} from '${HELPERS_ID}';`]
     : []
   )

packages/commonjs/test/fixtures/function/es6-export-with-global-sniffing/_config.js

@@ -0,0 +1,8 @@
+const nodeResolve = require('@rollup/plugin-node-resolve');
+
+module.exports = {
+  options: {
+    plugins: [nodeResolve()]
+  },
+  pluginOptions: {}
+};

packages/commonjs/test/fixtures/function/es6-export-with-global-sniffing/main.js

@@ -0,0 +1,7 @@
+// this test makes sure that "submodule" is not wrapped in commonjs
+//   helper due to its use of "typeof module", given that "submodule" has es6 exports.
+// any attempt to wrap it in a function will just fail as it's invalid syntax.
+
+import getGlobalPollution from './submodule.js';
+
+t.is(getGlobalPollution(), global.pollution);

packages/commonjs/test/fixtures/function/es6-export-with-global-sniffing/submodule.js

@@ -0,0 +1,19 @@
+let root;
+
+if (typeof self !== 'undefined') {
+  root = self;
+} else if (typeof window !== 'undefined') {
+  root = window;
+} else if (typeof global !== 'undefined') {
+  root = global;
+} else if (typeof module !== 'undefined') {
+  root = module;
+} else {
+  root = Function('return this')(); // eslint-disable-line no-new-func
+}
+
+root.pollution = 'foo';
+
+const getGlobalPollution = () => 'foo';
+
+export default getGlobalPollution;

packages/commonjs/test/snapshots/function.js.md

@@ -2500,6 +2500,35 @@ Generated by [AVA](https://ava.li).
       `,
     }
 
+## es6-export-with-global-sniffing
+
+> Snapshot 1
+
+    {
+      'main.js': `'use strict';␊
+      ␊
+      let root;␊
+      ␊
+      if (typeof self !== 'undefined') {␊
+        root = self;␊
+      } else if (typeof window !== 'undefined') {␊
+        root = window;␊
+      } else if (typeof global !== 'undefined') {␊
+        root = global;␊
+      } else {␊
+        root = module;␊
+      }␊
+      ␊
+      root.pollution = 'foo';␊
+      ␊
+      const getGlobalPollution = () => 'foo';␊
+      ␊
+      // this test makes sure that "submodule" is not wrapped in commonjs␊
+      ␊
+      t.is(getGlobalPollution(), global.pollution);␊
+      `,
+    }
+
 ## export-default-from
 
 > Snapshot 1