fix(commonjs): add heuristic to deoptimize requires after calling imp…

…orted function (requires [email protected]) (#1038)

BREAKING CHANGES: Requires at least [email protected]

packages/commonjs/package.json

@@ -52,7 +52,7 @@
     "require"
   ],
   "peerDependencies": {
-    "rollup": "^2.67.0"
+    "rollup": "^2.68.0"
   },
   "dependencies": {
     "@rollup/pluginutils": "^3.1.0",
@@ -68,7 +68,7 @@
     "@rollup/plugin-node-resolve": "^13.1.0",
     "locate-character": "^2.0.5",
     "require-relative": "^0.8.7",
-    "rollup": "^2.67.3",
+    "rollup": "^2.68.0",
     "shx": "^0.3.2",
     "source-map": "^0.7.3",
     "source-map-support": "^0.5.19",

packages/commonjs/src/proxies.js

@@ -73,7 +73,6 @@ export function getEsImportProxy(id, defaultIsModuleExports) {
   }
   return {
     code,
-    syntheticNamedExports: '__moduleExports',
-    meta: { commonjs: { isCommonJS: false } }
+    syntheticNamedExports: '__moduleExports'
   };
 }

packages/commonjs/src/resolve-id.js

@@ -121,7 +121,7 @@ export default function getResolveId(extensions) {
       meta: { commonjs: commonjsMeta }
     } = moduleInfo;
     if (commonjsMeta && commonjsMeta.isCommonJS === IS_WRAPPED_COMMONJS) {
-      return wrapId(resolved.id, ES_IMPORT_SUFFIX);
+      return { id: wrapId(resolved.id, ES_IMPORT_SUFFIX), meta: { commonjs: { resolved } } };
     }
     return resolved;
   };

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

@@ -1,6 +1,8 @@
 import {
+  ES_IMPORT_SUFFIX,
   EXTERNAL_SUFFIX,
   IS_WRAPPED_COMMONJS,
+  isWrappedId,
   PROXY_SUFFIX,
   wrapId,
   WRAPPED_SUFFIX
@@ -27,28 +29,28 @@ export function getRequireResolver(extensions, detectCyclesAndConditional) {
     return false;
   };
 
+  // Once a module is listed here, its type (wrapped or not) is fixed and may
+  // not change for the rest of the current build, to not break already
+  // transformed modules.
   const fullyAnalyzedModules = Object.create(null);
 
   const getTypeForFullyAnalyzedModule = (id) => {
     const knownType = knownCjsModuleTypes[id];
     if (knownType !== true || !detectCyclesAndConditional || fullyAnalyzedModules[id]) {
       return knownType;
     }
-    fullyAnalyzedModules[id] = true;
     if (isCyclic(id)) {
       return (knownCjsModuleTypes[id] = IS_WRAPPED_COMMONJS);
     }
     return knownType;
   };
 
   const setInitialParentType = (id, initialCommonJSType) => {
-    // It is possible a transformed module is already fully analyzed when using
-    // the cache and one dependency introduces a new cycle. Then transform is
-    // run for a fully analzyed module again. Fully analyzed modules may never
-    // change their type as importers already trust their type.
-    knownCjsModuleTypes[id] = fullyAnalyzedModules[id]
-      ? knownCjsModuleTypes[id]
-      : initialCommonJSType;
+    // Fully analyzed modules may never change type
+    if (fullyAnalyzedModules[id]) {
+      return;
+    }
+    knownCjsModuleTypes[id] = initialCommonJSType;
     if (
       detectCyclesAndConditional &&
       knownCjsModuleTypes[id] === true &&
@@ -59,7 +61,7 @@ export function getRequireResolver(extensions, detectCyclesAndConditional) {
     }
   };
 
-  const setTypesForRequiredModules = async (parentId, resolved, isConditional, loadModule) => {
+  const analyzeRequiredModule = async (parentId, resolved, isConditional, loadModule) => {
     const childId = resolved.id;
     requiredIds[childId] = true;
     if (!(isConditional || knownCjsModuleTypes[parentId] === IS_WRAPPED_COMMONJS)) {
@@ -68,41 +70,85 @@ export function getRequireResolver(extensions, detectCyclesAndConditional) {
 
     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.
+      // 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 loadModule(resolved);
     }
   };
 
+  const getTypeForImportedModule = async (resolved, loadModule) => {
+    if (resolved.id in knownCjsModuleTypes) {
+      // This handles cyclic ES dependencies
+      return knownCjsModuleTypes[resolved.id];
+    }
+    const {
+      meta: { commonjs }
+    } = await loadModule(resolved);
+    return (commonjs && commonjs.isCommonJS) || false;
+  };
+
   return {
     getWrappedIds: () =>
       Object.keys(knownCjsModuleTypes).filter(
         (id) => knownCjsModuleTypes[id] === IS_WRAPPED_COMMONJS
       ),
     isRequiredId: (id) => requiredIds[id],
-    async shouldTransformCachedModule({ id: parentId, meta: { commonjs: parentMeta } }) {
-      // Ignore modules that did not pass through the original transformer in a previous build
-      if (!(parentMeta && parentMeta.requires)) {
-        return false;
-      }
-      setInitialParentType(parentId, parentMeta.initialCommonJSType);
-      await Promise.all(
-        parentMeta.requires.map(({ resolved, isConditional }) =>
-          setTypesForRequiredModules(parentId, resolved, isConditional, this.load)
-        )
-      );
-      if (getTypeForFullyAnalyzedModule(parentId) !== parentMeta.isCommonJS) {
-        return true;
-      }
-      for (const {
-        resolved: { id }
-      } of parentMeta.requires) {
-        if (getTypeForFullyAnalyzedModule(id) !== parentMeta.isRequiredCommonJS[id]) {
+    async shouldTransformCachedModule({
+      id: parentId,
+      resolvedSources,
+      meta: { commonjs: parentMeta }
+    }) {
+      // We explicitly track ES modules to handle ciruclar imports
+      if (!(parentMeta && parentMeta.isCommonJS)) knownCjsModuleTypes[parentId] = false;
+      if (isWrappedId(parentId, ES_IMPORT_SUFFIX)) return false;
+      const parentRequires = parentMeta && parentMeta.requires;
+      if (parentRequires) {
+        setInitialParentType(parentId, parentMeta.initialCommonJSType);
+        await Promise.all(
+          parentRequires.map(({ resolved, isConditional }) =>
+            analyzeRequiredModule(parentId, resolved, isConditional, this.load)
+          )
+        );
+        if (getTypeForFullyAnalyzedModule(parentId) !== parentMeta.isCommonJS) {
           return true;
         }
+        for (const {
+          resolved: { id }
+        } of parentRequires) {
+          if (getTypeForFullyAnalyzedModule(id) !== parentMeta.isRequiredCommonJS[id]) {
+            return true;
+          }
+        }
+        // Now that we decided to go with the cached copy, neither the parent
+        // module nor any of its children may change types anymore
+        fullyAnalyzedModules[parentId] = true;
+        for (const {
+          resolved: { id }
+        } of parentRequires) {
+          fullyAnalyzedModules[id] = true;
+        }
       }
-      return false;
+      const parentRequireSet = new Set((parentRequires || []).map(({ resolved: { id } }) => id));
+      return (
+        await Promise.all(
+          Object.keys(resolvedSources)
+            .map((source) => resolvedSources[source])
+            .filter(({ id }) => !parentRequireSet.has(id))
+            .map(async (resolved) => {
+              if (isWrappedId(resolved.id, ES_IMPORT_SUFFIX)) {
+                return (
+                  (await getTypeForImportedModule(
+                    (await this.load({ id: resolved.id })).meta.commonjs.resolved,
+                    this.load
+                  )) !== IS_WRAPPED_COMMONJS
+                );
+              }
+              return (await getTypeForImportedModule(resolved, this.load)) === IS_WRAPPED_COMMONJS;
+            })
+        )
+      ).some((shouldTransform) => shouldTransform);
     },
     /* eslint-disable no-param-reassign */
     resolveRequireSourcesAndUpdateMeta: (rollupContext) => async (
@@ -133,16 +179,18 @@ export function getRequireResolver(extensions, detectCyclesAndConditional) {
             return { id: wrapId(childId, EXTERNAL_SUFFIX), allowProxy: false };
           }
           parentMeta.requires.push({ resolved, isConditional });
-          await setTypesForRequiredModules(parentId, resolved, isConditional, rollupContext.load);
+          await analyzeRequiredModule(parentId, resolved, isConditional, rollupContext.load);
           return { id: childId, allowProxy: true };
         })
       );
       parentMeta.isCommonJS = getTypeForFullyAnalyzedModule(parentId);
+      fullyAnalyzedModules[parentId] = true;
       return requireTargets.map(({ id: dependencyId, allowProxy }, index) => {
         // eslint-disable-next-line no-multi-assign
         const isCommonJS = (parentMeta.isRequiredCommonJS[
           dependencyId
         ] = getTypeForFullyAnalyzedModule(dependencyId));
+        fullyAnalyzedModules[dependencyId] = true;
         return {
           source: sources[index].source,
           id: allowProxy

packages/commonjs/src/transform-commonjs.js

@@ -95,6 +95,7 @@ export default async function transformCommonjs(
   const topLevelDefineCompiledEsmExpressions = [];
   const replacedGlobal = [];
   const replacedDynamicRequires = [];
+  const importedVariables = new Set();
 
   walk(ast, {
     enter(node, parent) {
@@ -208,6 +209,11 @@ export default async function transformCommonjs(
           }
 
           if (!isRequireExpression(node, scope)) {
+            const keypath = getKeypath(node.callee);
+            if (keypath && importedVariables.has(keypath.name)) {
+              // Heuristic to deoptimize requires after a required function has been called
+              currentConditionalNodeEnd = Infinity;
+            }
             return;
           }
 
@@ -236,6 +242,11 @@ export default async function transformCommonjs(
               currentConditionalNodeEnd !== null,
               parent.type === 'ExpressionStatement' ? parent : node
             );
+            if (parent.type === 'VariableDeclarator' && parent.id.type === 'Identifier') {
+              for (const name of extractAssignedNames(parent.id)) {
+                importedVariables.add(name);
+              }
+            }
           }
           return;
         }
@@ -448,7 +459,9 @@ export default async function transformCommonjs(
     magicString.remove(0, commentEnd).trim();
   }
 
-  const exportMode = shouldWrap
+  const exportMode = isEsModule
+    ? 'none'
+    : shouldWrap
     ? uses.module
       ? 'module'
       : 'exports'

packages/commonjs/test/fixtures/form/unambiguous-with-default-export/output.js

@@ -1,5 +1,4 @@
 import * as commonjsHelpers from "_commonjsHelpers.js";
-import { __exports as input } from "\u0000fixtures/form/unambiguous-with-default-export/input.js?commonjs-exports"
 import "\u0000CWD/fixtures/form/unambiguous-with-default-export/foo.js?commonjs-proxy";
 
 export default {};

packages/commonjs/test/fixtures/form/unambiguous-with-import/output.js

@@ -1,5 +1,4 @@
 import * as commonjsHelpers from "_commonjsHelpers.js";
-import { __exports as input } from "\u0000fixtures/form/unambiguous-with-import/input.js?commonjs-exports"
 import "\u0000CWD/fixtures/form/unambiguous-with-import/foo.js?commonjs-proxy";
 
 import './bar.js';

packages/commonjs/test/fixtures/form/unambiguous-with-named-export/output.js

@@ -1,5 +1,4 @@
 import * as commonjsHelpers from "_commonjsHelpers.js";
-import { __exports as input } from "\u0000fixtures/form/unambiguous-with-named-export/input.js?commonjs-exports"
 import "\u0000CWD/fixtures/form/unambiguous-with-named-export/foo.js?commonjs-proxy";
 
 export {};

packages/commonjs/test/fixtures/function/call-non-local-function-semantics/_config.js

@@ -0,0 +1,4 @@
+module.exports = {
+  description:
+    'uses strict require semantics for all modules that are required after an imported function is called'
+};

packages/commonjs/test/fixtures/function/call-non-local-function-semantics/browser.js

@@ -0,0 +1 @@
+module.exports = 'browser';

packages/commonjs/test/fixtures/function/call-non-local-function-semantics/main.js

@@ -0,0 +1,7 @@
+// simplified from dd-trace
+const platform = require('./platform');
+const browser = require('./browser');
+
+platform.use(browser);
+
+require('./proxy');

packages/commonjs/test/fixtures/function/call-non-local-function-semantics/platform.js

@@ -0,0 +1 @@
+exports.use = (platform) => (exports.platform = platform);

packages/commonjs/test/fixtures/function/call-non-local-function-semantics/proxy.js

@@ -0,0 +1,3 @@
+const { platform } = require('./platform.js');
+
+t.is(platform, 'browser');

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

@@ -87,6 +87,46 @@ Generated by [AVA](https://avajs.dev).
       `,
     }
 
+## call-non-local-function-semantics
+
+> Snapshot 1
+
+    {
+      'main.js': `'use strict';␊
+      ␊
+      var main = {};␊
+      ␊
+      var platform$1 = {};␊
+      ␊
+      platform$1.use = (platform) => (platform$1.platform = platform);␊
+      ␊
+      var browser$1 = 'browser';␊
+      ␊
+      var proxy = {};␊
+      ␊
+      var hasRequiredProxy;␊
+      ␊
+      function requireProxy () {␊
+      	if (hasRequiredProxy) return proxy;␊
+      	hasRequiredProxy = 1;␊
+      	const { platform } = platform$1;␊
+      ␊
+      	t.is(platform, 'browser');␊
+      	return proxy;␊
+      }␊
+      ␊
+      // simplified from dd-trace␊
+      const platform = platform$1;␊
+      const browser = browser$1;␊
+      ␊
+      platform.use(browser);␊
+      ␊
+      requireProxy();␊
+      ␊
+      module.exports = main;␊
+      `,
+    }
+
 ## circular-dependencies
 
 > Snapshot 1