fix(commonjs): do not transform "typeof exports" for mixed modules (#…

…1038)

resolves #1014

packages/commonjs/src/index.js

@@ -128,16 +128,19 @@ export default function commonjs(options = {}) {
       !isEsModule &&
       (dynamicRequireModules.has(normalizePathSlashes(id)) || strictRequiresFilter(id));
 
-    const checkDynamicRequire = () => {
+    const checkDynamicRequire = (position) => {
       if (id.indexOf(dynamicRequireRoot) !== 0) {
-        this.error({
-          code: 'DYNAMIC_REQUIRE_OUTSIDE_ROOT',
-          id,
-          dynamicRequireRoot,
-          message: `"${id}" contains dynamic require statements but it is not within the current dynamicRequireRoot "${dynamicRequireRoot}". You should set dynamicRequireRoot to "${dirname(
-            id
-          )}" or one of its parent directories.`
-        });
+        this.error(
+          {
+            code: 'DYNAMIC_REQUIRE_OUTSIDE_ROOT',
+            id,
+            dynamicRequireRoot,
+            message: `"${id}" contains dynamic require statements but it is not within the current dynamicRequireRoot "${dynamicRequireRoot}". You should set dynamicRequireRoot to "${dirname(
+              id
+            )}" or one of its parent directories.`
+          },
+          position
+        );
       }
     };
 

packages/commonjs/src/resolve-id.js

@@ -50,6 +50,14 @@ export function resolveExtensions(importee, importer, extensions) {
 
 export default function getResolveId(extensions) {
   return async function resolveId(importee, importer, resolveOptions) {
+    // We assume that all requires are pre-resolved
+    if (
+      resolveOptions.custom &&
+      resolveOptions.custom['node-resolve'] &&
+      resolveOptions.custom['node-resolve'].isRequire
+    ) {
+      return null;
+    }
     if (isWrappedId(importee, WRAPPED_SUFFIX)) {
       return unwrapId(importee, WRAPPED_SUFFIX);
     }
@@ -69,7 +77,7 @@ export default function getResolveId(extensions) {
     if (importer) {
       if (
         importer === DYNAMIC_MODULES_ID ||
-        // Except for exports, proxies are only importing resolved ids, no need to resolve again
+        // Proxies are only importing resolved ids, no need to resolve again
         isWrappedId(importer, PROXY_SUFFIX) ||
         isWrappedId(importer, ES_IMPORT_SUFFIX)
       ) {

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

@@ -11,9 +11,39 @@ export function getResolveRequireSourcesAndGetMeta(extensions, detectCyclesAndCo
   const knownCjsModuleTypes = Object.create(null);
   const requiredIds = Object.create(null);
   const unconditionallyRequiredIds = Object.create(null);
-  const dependentModules = Object.create(null);
-  const getDependentModules = (id) =>
-    dependentModules[id] || (dependentModules[id] = Object.create(null));
+  const dependencies = Object.create(null);
+  const getDependencies = (id) => dependencies[id] || (dependencies[id] = new Set());
+
+  const isCyclic = (id) => {
+    const dependenciesToCheck = new Set(getDependencies(id));
+    for (const dependency of dependenciesToCheck) {
+      if (dependency === id) {
+        return true;
+      }
+      for (const childDependency of getDependencies(dependency)) {
+        dependenciesToCheck.add(childDependency);
+      }
+    }
+    return false;
+  };
+
+  const fullyAnalyzedModules = Object.create(null);
+
+  const getTypeForFullyAnalyzedModule = (id) => {
+    const knownType = knownCjsModuleTypes[id];
+    if (
+      knownType === IS_WRAPPED_COMMONJS ||
+      !detectCyclesAndConditional ||
+      fullyAnalyzedModules[id]
+    ) {
+      return knownType;
+    }
+    fullyAnalyzedModules[id] = true;
+    if (isCyclic(id)) {
+      return (knownCjsModuleTypes[id] = IS_WRAPPED_COMMONJS);
+    }
+    return knownType;
+  };
 
   return {
     getWrappedIds: () =>
@@ -26,8 +56,9 @@ export function getResolveRequireSourcesAndGetMeta(extensions, detectCyclesAndCo
       isParentCommonJS,
       sources
     ) => {
-      knownCjsModuleTypes[parentId] = knownCjsModuleTypes[parentId] || isParentCommonJS;
+      knownCjsModuleTypes[parentId] = isParentCommonJS;
       if (
+        detectCyclesAndConditional &&
         knownCjsModuleTypes[parentId] &&
         requiredIds[parentId] &&
         !unconditionallyRequiredIds[parentId]
@@ -42,9 +73,7 @@ export function getResolveRequireSourcesAndGetMeta(extensions, detectCyclesAndCo
           }
           const resolved =
             (await rollupContext.resolve(source, parentId, {
-              custom: {
-                'node-resolve': { isRequire: true }
-              }
+              custom: { 'node-resolve': { isRequire: true } }
             })) || resolveExtensions(source, parentId, extensions);
           if (!resolved) {
             return { id: wrapId(source, EXTERNAL_SUFFIX), allowProxy: false };
@@ -54,44 +83,25 @@ export function getResolveRequireSourcesAndGetMeta(extensions, detectCyclesAndCo
             return { id: wrapId(childId, EXTERNAL_SUFFIX), allowProxy: false };
           }
           requiredIds[childId] = true;
-          if (
-            !(
-              detectCyclesAndConditional &&
-              (isConditional || knownCjsModuleTypes[parentId] === IS_WRAPPED_COMMONJS)
-            )
-          ) {
+          if (!(isConditional || knownCjsModuleTypes[parentId] === IS_WRAPPED_COMMONJS)) {
             unconditionallyRequiredIds[childId] = true;
           }
-          const parentDependentModules = getDependentModules(parentId);
-          const childDependentModules = getDependentModules(childId);
-          childDependentModules[parentId] = true;
-          for (const dependentId of Object.keys(parentDependentModules)) {
-            childDependentModules[dependentId] = true;
-          }
-          if (parentDependentModules[childId]) {
-            // If we depend on one of our dependencies, we have a cycle. Then all modules that
-            // we depend on that also depend on the same module are part of a cycle as well.
-            if (detectCyclesAndConditional && isParentCommonJS) {
-              knownCjsModuleTypes[parentId] = IS_WRAPPED_COMMONJS;
-              knownCjsModuleTypes[childId] = IS_WRAPPED_COMMONJS;
-              for (const dependentId of Object.keys(parentDependentModules)) {
-                if (getDependentModules(dependentId)[childId]) {
-                  knownCjsModuleTypes[dependentId] = IS_WRAPPED_COMMONJS;
-                }
-              }
-            }
-          } else {
+
+          getDependencies(parentId).add(childId);
+          if (!isCyclic(childId)) {
             // This makes sure the current transform handler waits for all direct dependencies to be
             // loaded and transformed and therefore for all transitive CommonJS dependencies to be
             // loaded as well so that all cycles have been found and knownCjsModuleTypes is reliable.
             await rollupContext.load(resolved);
+          } else if (detectCyclesAndConditional && knownCjsModuleTypes[parentId]) {
+            knownCjsModuleTypes[parentId] = IS_WRAPPED_COMMONJS;
           }
           return { id: childId, allowProxy: true };
         })
       );
       return {
         requireTargets: requireTargets.map(({ id: dependencyId, allowProxy }, index) => {
-          const isCommonJS = knownCjsModuleTypes[dependencyId];
+          const isCommonJS = getTypeForFullyAnalyzedModule(dependencyId);
           return {
             source: sources[index].source,
             id: allowProxy
@@ -102,7 +112,7 @@ export function getResolveRequireSourcesAndGetMeta(extensions, detectCyclesAndCo
             isCommonJS
           };
         }),
-        usesRequireWrapper: knownCjsModuleTypes[parentId] === IS_WRAPPED_COMMONJS
+        usesRequireWrapper: getTypeForFullyAnalyzedModule(parentId) === IS_WRAPPED_COMMONJS
       };
     }
   };

packages/commonjs/src/transform-commonjs.js

@@ -198,7 +198,7 @@ export default async function transformCommonjs(
             isRequire(node.callee.object, scope) &&
             node.callee.property.name === 'resolve'
           ) {
-            checkDynamicRequire();
+            checkDynamicRequire(node.start);
             uses.require = true;
             const requireNode = node.callee.object;
             magicString.appendLeft(
@@ -220,6 +220,7 @@ export default async function transformCommonjs(
 
           if (hasDynamicArguments(node)) {
             if (isDynamicRequireModulesEnabled) {
+              checkDynamicRequire(node.start);
               magicString.appendLeft(
                 node.end - 1,
                 `, ${JSON.stringify(
@@ -228,7 +229,6 @@ export default async function transformCommonjs(
               );
             }
             if (!ignoreDynamicRequires) {
-              checkDynamicRequire();
               replacedDynamicRequires.push(node.callee);
             }
             return;
@@ -286,7 +286,6 @@ export default async function transformCommonjs(
                 return;
               }
               if (!ignoreDynamicRequires) {
-                checkDynamicRequire();
                 if (isShorthandProperty(parent)) {
                   magicString.prependRight(node.start, 'require: ');
                 }
@@ -370,9 +369,10 @@ export default async function transformCommonjs(
             if (scope.contains(flattened.name)) return;
 
             if (
-              flattened.keypath === 'module.exports' ||
-              flattened.keypath === 'module' ||
-              flattened.keypath === 'exports'
+              !isEsModule &&
+              (flattened.keypath === 'module.exports' ||
+                flattened.keypath === 'module' ||
+                flattened.keypath === 'exports')
             ) {
               magicString.overwrite(node.start, node.end, `'object'`, {
                 storeName: false

packages/commonjs/test/fixtures/function/load-cycle-parallel/_config.js

@@ -0,0 +1,3 @@
+module.exports = {
+  description: 'handles loading all modules of a cycle in parallel'
+};

packages/commonjs/test/fixtures/function/load-cycle-parallel/a.js

@@ -0,0 +1 @@
+require('./b.js');

packages/commonjs/test/fixtures/function/load-cycle-parallel/b.js

@@ -0,0 +1 @@
+require('./c.js');

packages/commonjs/test/fixtures/function/load-cycle-parallel/c.js

@@ -0,0 +1 @@
+require('./a.js');

packages/commonjs/test/fixtures/function/load-cycle-parallel/main.js

@@ -0,0 +1,3 @@
+require('./a.js');
+require('./b.js');
+require('./c.js');

packages/commonjs/test/fixtures/samples/mixed-module-typeof-exports/_config.js

@@ -0,0 +1,4 @@
+module.exports = {
+  description: 'replaces "typeof exports" with "undefined" in mixed modules',
+  pluginOptions: { transformMixedEsModules: true }
+};

packages/commonjs/test/fixtures/samples/mixed-module-typeof-exports/foo.js

@@ -0,0 +1 @@
+module.exports = 21;

packages/commonjs/test/fixtures/samples/mixed-module-typeof-exports/main.js

@@ -0,0 +1,7 @@
+const foo = require('./foo');
+
+if (typeof exports !== 'undefined') {
+  throw new Error('There should be no global exports in an ES module');
+}
+
+export { foo as default };

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

@@ -3050,8 +3050,10 @@ Generated by [AVA](https://avajs.dev).
         root = window;␊
       } else if (typeof global !== 'undefined') {␊
         root = global;␊
-      } else {␊
+      } else if (typeof module !== 'undefined') {␊
         root = module;␊
+      } else {␊
+        root = Function('return this')(); // eslint-disable-line no-new-func␊
       }␊
       ␊
       root.pollution = 'foo';␊
@@ -4725,6 +4727,19 @@ Generated by [AVA](https://avajs.dev).
       `,
     }
 
+## load-cycle-parallel
+
+> Snapshot 1
+
+    {
+      'main.js': `'use strict';␊
+      ␊
+      var main = {};␊
+      ␊
+      module.exports = main;␊
+      `,
+    }
+
 ## module-meta-properties
 
 > Snapshot 1

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

@@ -100,3 +100,18 @@ Generated by [AVA](https://avajs.dev).
     ␊
     module.exports = main;␊
     `
+
+## does not transform typeof exports for mixed modules
+
+> Snapshot 1
+
+    `var foo$1 = 21;␊
+    ␊
+    const foo = foo$1;␊
+    ␊
+    if (typeof exports !== 'undefined') {␊
+      throw new Error('There should be no global exports in an ES module');␊
+    }␊
+    ␊
+    export { foo as default };␊
+    `

packages/commonjs/test/test.js

@@ -331,8 +331,8 @@ test('typeof transforms: sinon', async (t) => {
   } = await bundle.generate({ format: 'es' });
 
   t.is(code.indexOf('typeof require'), -1, code);
-  // t.not( code.indexOf( 'typeof module' ), -1, code ); // #151 breaks this test
-  // t.not( code.indexOf( 'typeof define' ), -1, code ); // #144 breaks this test
+  t.is(code.indexOf('typeof module'), -1, code);
+  t.is(code.indexOf('typeof define'), -1, code);
 });
 
 test('deconflicts helper name', async (t) => {
@@ -717,3 +717,17 @@ test('throws when there is a dynamic require from outside dynamicRequireRoot', a
     dynamicRequireRoot
   });
 });
+
+test('does not transform typeof exports for mixed modules', async (t) => {
+  const bundle = await rollup({
+    input: 'fixtures/samples/mixed-module-typeof-exports/main.js',
+    plugins: [commonjs({ transformMixedEsModules: true })]
+  });
+
+  const {
+    output: [{ code }]
+  } = await bundle.generate({ format: 'es' });
+
+  t.is(code.includes('typeof exports'), true, '"typeof exports" not found in the code');
+  t.snapshot(code);
+});