feat(commonjs): auto-detect conditional requires (#1038)

rollup · Apr 24, 2022 · 55e7e2d · 55e7e2d
1 parent 38a3aa4
commit 55e7e2d
Show file tree

Hide file tree

Showing 18 changed files with 364 additions and 65 deletions.
diff --git a/packages/commonjs/src/generate-imports.js b/packages/commonjs/src/generate-imports.js
@@ -104,6 +104,7 @@ export function getRequireHandlers() {
     scope,
     usesReturnValue,
     isInsideTryBlock,
+    isInsideConditional,
     toBeRemoved
   ) {
     requireExpressions.push({
@@ -112,6 +113,7 @@ export function getRequireHandlers() {
       scope,
       usesReturnValue,
       isInsideTryBlock,
+      isInsideConditional,
       toBeRemoved
     });
   }
@@ -135,7 +137,6 @@ export function getRequireHandlers() {
     const imports = [];
     imports.push(`import * as ${helpersName} from "${HELPERS_ID}";`);
     if (usesRequire) {
-      // TODO Lukas check where to import it from or change to usesDynamicRequire
       imports.push(
         `import { commonjsRequire as ${dynamicRequireName} } from "${DYNAMIC_MODULES_ID}";`
       );
@@ -155,7 +156,12 @@ export function getRequireHandlers() {
     const { requireTargets, usesRequireWrapper } = await resolveRequireSourcesAndGetMeta(
       id,
       needsRequireWrapper ? IS_WRAPPED_COMMONJS : !isEsModule,
-      Object.keys(requiresBySource)
+      Object.keys(requiresBySource).map((source) => {
+        return {
+          source,
+          isConditional: requiresBySource[source].every((require) => require.isInsideConditional)
+        };
+      })
     );
     processRequireExpressions(
       imports,

diff --git a/packages/commonjs/src/index.js b/packages/commonjs/src/index.js
@@ -38,7 +38,7 @@ export default function commonjs(options = {}) {
   } = options;
   const extensions = options.extensions || ['.js'];
   const filter = createFilter(options.include, options.exclude);
-  const { strictRequiresFilter, detectCycles } = getStrictRequiresFilter(options);
+  const { strictRequiresFilter, detectCyclesAndConditional } = getStrictRequiresFilter(options);
 
   const getRequireReturnsDefault =
     typeof requireReturnsDefaultOption === 'function'
@@ -63,10 +63,11 @@ export default function commonjs(options = {}) {
     resolveRequireSourcesAndGetMeta,
     getWrappedIds,
     isRequiredId
-  } = getResolveRequireSourcesAndGetMeta(extensions, detectCycles);
+  } = getResolveRequireSourcesAndGetMeta(extensions, detectCyclesAndConditional);
   const dynamicRequireModules = getDynamicRequireModules(options.dynamicRequireTargets);
   const isDynamicRequireModulesEnabled = dynamicRequireModules.size > 0;
-  // TODO Lukas do we need the CWD?
+  // TODO Lukas replace with new dynamicRequireRoot to replace CWD
+  // TODO Lukas throw if require from outside commondir
   const commonDir = isDynamicRequireModulesEnabled
     ? getCommonDir(null, Array.from(dynamicRequireModules.keys()).concat(process.cwd()))
     : null;

diff --git a/packages/commonjs/src/resolve-require-sources.js b/packages/commonjs/src/resolve-require-sources.js
@@ -7,9 +7,10 @@ import {
 } from './helpers';
 import { resolveExtensions } from './resolve-id';
 
-export function getResolveRequireSourcesAndGetMeta(extensions, detectCycles) {
+export function getResolveRequireSourcesAndGetMeta(extensions, detectCyclesAndConditional) {
   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));
@@ -20,20 +21,31 @@ export function getResolveRequireSourcesAndGetMeta(extensions, detectCycles) {
         (id) => knownCjsModuleTypes[id] === IS_WRAPPED_COMMONJS
       ),
     isRequiredId: (id) => requiredIds[id],
-    resolveRequireSourcesAndGetMeta: (rollupContext) => async (id, isParentCommonJS, sources) => {
-      knownCjsModuleTypes[id] = isParentCommonJS;
+    resolveRequireSourcesAndGetMeta: (rollupContext) => async (
+      parentId,
+      isParentCommonJS,
+      sources
+    ) => {
+      knownCjsModuleTypes[parentId] = knownCjsModuleTypes[parentId] || isParentCommonJS;
+      if (
+        knownCjsModuleTypes[parentId] &&
+        requiredIds[parentId] &&
+        !unconditionallyRequiredIds[parentId]
+      ) {
+        knownCjsModuleTypes[parentId] = IS_WRAPPED_COMMONJS;
+      }
       const requireTargets = await Promise.all(
-        sources.map(async (source) => {
+        sources.map(async ({ source, isConditional }) => {
           // Never analyze or proxy internal modules
           if (source.startsWith('\0')) {
             return { id: source, allowProxy: false };
           }
           const resolved =
-            (await rollupContext.resolve(source, id, {
+            (await rollupContext.resolve(source, parentId, {
               custom: {
                 'node-resolve': { isRequire: true }
               }
-            })) || resolveExtensions(source, id, extensions);
+            })) || resolveExtensions(source, parentId, extensions);
           if (!resolved) {
             return { id: wrapId(source, EXTERNAL_SUFFIX), allowProxy: false };
           }
@@ -42,17 +54,25 @@ export function getResolveRequireSourcesAndGetMeta(extensions, detectCycles) {
             return { id: wrapId(childId, EXTERNAL_SUFFIX), allowProxy: false };
           }
           requiredIds[childId] = true;
-          const parentDependentModules = getDependentModules(id);
+          if (
+            !(
+              detectCyclesAndConditional &&
+              (isConditional || knownCjsModuleTypes[parentId] === IS_WRAPPED_COMMONJS)
+            )
+          ) {
+            unconditionallyRequiredIds[childId] = true;
+          }
+          const parentDependentModules = getDependentModules(parentId);
           const childDependentModules = getDependentModules(childId);
-          childDependentModules[id] = true;
+          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 (detectCycles && isParentCommonJS) {
-              knownCjsModuleTypes[id] = IS_WRAPPED_COMMONJS;
+            if (detectCyclesAndConditional && isParentCommonJS) {
+              knownCjsModuleTypes[parentId] = IS_WRAPPED_COMMONJS;
               knownCjsModuleTypes[childId] = IS_WRAPPED_COMMONJS;
               for (const dependentId of Object.keys(parentDependentModules)) {
                 if (getDependentModules(dependentId)[childId]) {
@@ -73,7 +93,7 @@ export function getResolveRequireSourcesAndGetMeta(extensions, detectCycles) {
         requireTargets: requireTargets.map(({ id: dependencyId, allowProxy }, index) => {
           const isCommonJS = knownCjsModuleTypes[dependencyId];
           return {
-            source: sources[index],
+            source: sources[index].source,
             id: allowProxy
               ? isCommonJS === IS_WRAPPED_COMMONJS
                 ? wrapId(dependencyId, WRAPPED_SUFFIX)
@@ -82,7 +102,7 @@ export function getResolveRequireSourcesAndGetMeta(extensions, detectCycles) {
             isCommonJS
           };
         }),
-        usesRequireWrapper: knownCjsModuleTypes[id] === IS_WRAPPED_COMMONJS
+        usesRequireWrapper: knownCjsModuleTypes[parentId] === IS_WRAPPED_COMMONJS
       };
     }
   };

diff --git a/packages/commonjs/src/transform-commonjs.js b/packages/commonjs/src/transform-commonjs.js
@@ -71,7 +71,13 @@ export default async function transformCommonjs(
   let shouldWrap = false;
 
   const globals = new Set();
+  // A conditionalNode is a node for which execution is not guaranteed. If such a node is a require
+  // or contains nested requires, those should be handled as function calls unless there is an
+  // unconditional require elsewhere.
+  let currentConditionalNodeEnd = null;
+  const conditionalNodes = new Set();
 
+  // TODO Lukas fix this at last, we are close
   // TODO technically wrong since globals isn't populated yet, but ¯\_(ツ)_/¯
   const helpersName = deconflict([scope], globals, 'commonjsHelpers');
   const dynamicRequireName = deconflict([scope], globals, 'commonjsRequire');
@@ -102,6 +108,12 @@ export default async function transformCommonjs(
       if (currentTryBlockEnd !== null && node.start > currentTryBlockEnd) {
         currentTryBlockEnd = null;
       }
+      if (currentConditionalNodeEnd !== null && node.start > currentConditionalNodeEnd) {
+        currentConditionalNodeEnd = null;
+      }
+      if (currentConditionalNodeEnd === null && conditionalNodes.has(node)) {
+        currentConditionalNodeEnd = node.end;
+      }
 
       programDepth += 1;
       if (node.scope) ({ scope } = node);
@@ -113,11 +125,6 @@ export default async function transformCommonjs(
 
       // eslint-disable-next-line default-case
       switch (node.type) {
-        case 'TryStatement':
-          if (currentTryBlockEnd === null) {
-            currentTryBlockEnd = node.block.end;
-          }
-          return;
         case 'AssignmentExpression':
           if (node.left.type === 'MemberExpression') {
             const flattened = getKeypath(node.left);
@@ -227,6 +234,7 @@ export default async function transformCommonjs(
               scope,
               usesReturnValue,
               currentTryBlockEnd !== null,
+              currentConditionalNodeEnd !== null,
               parent.type === 'ExpressionStatement' ? parent : node
             );
           }
@@ -237,8 +245,26 @@ export default async function transformCommonjs(
           // skip dead branches
           if (isFalsy(node.test)) {
             skippedNodes.add(node.consequent);
-          } else if (node.alternate && isTruthy(node.test)) {
-            skippedNodes.add(node.alternate);
+          } else if (isTruthy(node.test)) {
+            if (node.alternate) {
+              skippedNodes.add(node.alternate);
+            }
+          } else {
+            conditionalNodes.add(node.consequent);
+            if (node.alternate) {
+              conditionalNodes.add(node.alternate);
+            }
+          }
+          return;
+        case 'ArrowFunctionExpression':
+        case 'FunctionDeclaration':
+        case 'FunctionExpression':
+          // requires in functions should be conditional unless it is an IIFE
+          if (
+            currentConditionalNodeEnd === null &&
+            !(parent.type === 'CallExpression' && parent.callee === node)
+          ) {
+            currentConditionalNodeEnd = node.end;
           }
           return;
         case 'Identifier': {
@@ -301,6 +327,22 @@ export default async function transformCommonjs(
               return;
           }
         }
+        case 'LogicalExpression':
+          // skip dead branches
+          if (node.operator === '&&') {
+            if (isFalsy(node.left)) {
+              skippedNodes.add(node.right);
+            } else if (!isTruthy(node.left)) {
+              conditionalNodes.add(node.right);
+            }
+          } else if (node.operator === '||') {
+            if (isTruthy(node.left)) {
+              skippedNodes.add(node.right);
+            } else if (!isFalsy(node.left)) {
+              conditionalNodes.add(node.right);
+            }
+          }
+          return;
         case 'MemberExpression':
           if (!isDynamicRequireModulesEnabled && isModuleRequire(node, scope)) {
             uses.require = true;
@@ -328,6 +370,11 @@ export default async function transformCommonjs(
             }
           }
           return;
+        case 'TryStatement':
+          if (currentTryBlockEnd === null) {
+            currentTryBlockEnd = node.block.end;
+          }
+          return;
         case 'UnaryExpression':
           // rewrite `typeof module`, `typeof module.exports` and `typeof exports` (https://github.com/rollup/rollup-plugin-commonjs/issues/151)
           if (node.operator === 'typeof') {

diff --git a/packages/commonjs/src/utils.js b/packages/commonjs/src/utils.js
@@ -44,20 +44,20 @@ export function capitalize(name) {
 export function getStrictRequiresFilter({ strictRequires }) {
   switch (strictRequires) {
     case true:
-      return { strictRequiresFilter: () => true, detectCycles: false };
+      return { strictRequiresFilter: () => true, detectCyclesAndConditional: false };
     // eslint-disable-next-line no-undefined
     case undefined:
     case 'auto':
     case 'debug':
     case null:
-      return { strictRequiresFilter: () => false, detectCycles: true };
+      return { strictRequiresFilter: () => false, detectCyclesAndConditional: true };
     case false:
-      return { strictRequiresFilter: () => false, detectCycles: false };
+      return { strictRequiresFilter: () => false, detectCyclesAndConditional: false };
     default:
       if (typeof strictRequires === 'string' || Array.isArray(strictRequires)) {
         return {
           strictRequiresFilter: createFilter(strictRequires),
-          detectCycles: false
+          detectCyclesAndConditional: false
         };
       }
       throw new Error('Unexpected value for "strictRequires" option.');

diff --git a/packages/commonjs/test/fixtures/function/conditional-require-chain/_config.js b/packages/commonjs/test/fixtures/function/conditional-require-chain/_config.js
@@ -0,0 +1,3 @@
+module.exports = {
+  description: 'makes requires in conditionally required modules conditional as well'
+};
diff --git a/packages/commonjs/test/fixtures/function/conditional-require-chain/dep.js b/packages/commonjs/test/fixtures/function/conditional-require-chain/dep.js
@@ -0,0 +1 @@
+require('./throws.js');
diff --git a/packages/commonjs/test/fixtures/function/conditional-require-chain/main.js b/packages/commonjs/test/fixtures/function/conditional-require-chain/main.js
@@ -0,0 +1,6 @@
+global.false = false;
+
+if (global.false) {
+  // eslint-disable-next-line global-require
+  require('./dep.js');
+}
diff --git a/packages/commonjs/test/fixtures/function/conditional-require-chain/throws.js b/packages/commonjs/test/fixtures/function/conditional-require-chain/throws.js
@@ -0,0 +1 @@
+throw new Error('This should not be executed');
diff --git a/packages/commonjs/test/fixtures/function/skips-dead-branches/main.js b/packages/commonjs/test/fixtures/function/skips-dead-branches/main.js
@@ -3,4 +3,9 @@ if ('development' === 'production') {
   require('./a.js');
 }
 
-module.exports = true ? require('./b.js') : require('./c.js');
+exports.conditionalTrue = true ? require('./b.js') : require('./c.js');
+exports.conditionalFalse = false ? require('./c.js') : require('./b.js');
+exports.logicalAnd1 = true && require('./b.js');
+exports.logicalAnd2 = false && require('./c.js');
+exports.logicalOr1 = true || require('./c.js');
+exports.logicalOr2 = false || require('./b.js');
diff --git a/packages/commonjs/test/fixtures/function/strict-requires-detect-conditionals/_config.js b/packages/commonjs/test/fixtures/function/strict-requires-detect-conditionals/_config.js
@@ -0,0 +1,3 @@
+module.exports = {
+  description: 'automatically detects requires nested in conditionals'
+};
diff --git a/packages/commonjs/test/fixtures/function/strict-requires-detect-conditionals/hoisted.js b/packages/commonjs/test/fixtures/function/strict-requires-detect-conditionals/hoisted.js
@@ -0,0 +1 @@
+module.exports = 'this should be top-level';
diff --git a/packages/commonjs/test/fixtures/function/strict-requires-detect-conditionals/main.js b/packages/commonjs/test/fixtures/function/strict-requires-detect-conditionals/main.js
@@ -0,0 +1,44 @@
+/* eslint-disable global-require */
+global.false = false;
+global.true = true;
+
+if (global.false) {
+  require('./throws.js');
+}
+
+if (global.true) {
+  /* do nothing */
+} else {
+  require('./throws.js');
+}
+
+const conditionalFalse = global.false ? require('./throws.js') : null;
+const conditionalTrue = global.true ? null : require('./throws.js');
+
+const logicalAnd = global.false && require('./throws.js');
+const logicalOr = global.true || require('./throws.js');
+
+function requireFunctionDeclaration() {
+  require('./throws.js');
+}
+
+const requireFunctionExpression = function () {
+  require('./throws.js');
+};
+
+const requireArrowFunction = () => require('./throws.js');
+
+if (global.false) {
+  requireFunctionDeclaration();
+  requireFunctionExpression();
+  requireArrowFunction();
+}
+
+// These should not cause wrapping
+t.is(
+  (function () {
+    return require('./hoisted.js');
+  })(),
+  'this should be top-level'
+);
+t.is((() => require('./hoisted.js'))(), 'this should be top-level');
diff --git a/packages/commonjs/test/fixtures/function/strict-requires-detect-conditionals/throws.js b/packages/commonjs/test/fixtures/function/strict-requires-detect-conditionals/throws.js
@@ -0,0 +1 @@
+throw new Error('This should never be executed or imported');
diff --git a/packages/commonjs/test/fixtures/function/strict-requires-file-without-module-type/main.js b/packages/commonjs/test/fixtures/function/strict-requires-file-without-module-type/main.js
@@ -1,2 +1,4 @@
+global.null = 0;
+
 // eslint-disable-next-line global-require
-t.is(0 && require('./error.js'), 0);
+t.is(global.null && require('./error.js'), 0);