refactor(commonjs): deconflict helpers only once globals are known (#…

…1038)
rollup · Apr 24, 2022 · 335a013 · 335a013
1 parent c198d9d
commit 335a013
Show file tree

Hide file tree

Showing 16 changed files with 239 additions and 171 deletions.
diff --git a/packages/commonjs/src/generate-imports.js b/packages/commonjs/src/generate-imports.js
@@ -1,7 +1,3 @@
-import { dirname, resolve } from 'path';
-
-import { sync as nodeResolveSync } from 'resolve';
-
 import {
   DYNAMIC_MODULES_ID,
   EXPORTS_SUFFIX,
@@ -12,9 +8,8 @@ import {
   MODULE_SUFFIX,
   wrapId
 } from './helpers';
-import { normalizePathSlashes } from './utils';
 
-export function isRequireStatement(node, scope) {
+export function isRequireExpression(node, scope) {
   if (!node) return false;
   if (node.type !== 'CallExpression') return false;
 
@@ -24,7 +19,7 @@ export function isRequireStatement(node, scope) {
   return isRequire(node.callee, scope);
 }
 
-function isRequire(node, scope) {
+export function isRequire(node, scope) {
   return (
     (node.type === 'Identifier' && node.name === 'require' && !scope.contains('require')) ||
     (node.type === 'MemberExpression' && isModuleRequire(node, scope))
@@ -41,12 +36,7 @@ export function isModuleRequire({ object, property }, scope) {
   );
 }
 
-export function isStaticRequireStatement(node, scope) {
-  if (!isRequireStatement(node, scope)) return false;
-  return !hasDynamicArguments(node);
-}
-
-function hasDynamicArguments(node) {
+export function hasDynamicArguments(node) {
   return (
     node.arguments.length > 1 ||
     (node.arguments[0].type !== 'Literal' &&
@@ -60,41 +50,12 @@ export function isNodeRequirePropertyAccess(parent) {
   return parent && parent.property && reservedMethod[parent.property.name];
 }
 
-export function isIgnoredRequireStatement(requiredNode, ignoreRequire) {
-  return ignoreRequire(requiredNode.arguments[0].value);
-}
-
 export function getRequireStringArg(node) {
   return node.arguments[0].type === 'Literal'
     ? node.arguments[0].value
     : node.arguments[0].quasis[0].value.cooked;
 }
 
-export function hasDynamicModuleForPath(source, id, dynamicRequireModules) {
-  if (!/^(?:\.{0,2}[/\\]|[A-Za-z]:[/\\])/.test(source)) {
-    try {
-      const resolvedPath = normalizePathSlashes(nodeResolveSync(source, { basedir: dirname(id) }));
-      if (dynamicRequireModules.has(resolvedPath)) {
-        return true;
-      }
-    } catch (ex) {
-      // Probably a node.js internal module
-      return false;
-    }
-
-    return false;
-  }
-
-  for (const attemptExt of ['', '.js', '.json']) {
-    const resolvedPath = normalizePathSlashes(resolve(dirname(id), source + attemptExt));
-    if (dynamicRequireModules.has(resolvedPath)) {
-      return true;
-    }
-  }
-
-  return false;
-}
-
 export function getRequireHandlers() {
   const requireExpressions = [];
 

diff --git a/packages/commonjs/src/index.js b/packages/commonjs/src/index.js
@@ -67,7 +67,6 @@ export default function commonjs(options = {}) {
     typeof options.dynamicRequireRoot === 'string'
       ? resolve(options.dynamicRequireRoot)
       : process.cwd();
-  // TODO Lukas throw if require from outside commondir
   const { commonDir, dynamicRequireModules } = getDynamicRequireModules(
     options.dynamicRequireTargets,
     dynamicRequireRoot

diff --git a/packages/commonjs/src/transform-commonjs.js b/packages/commonjs/src/transform-commonjs.js
@@ -20,12 +20,11 @@ import { rewriteExportsAndGetExportsBlock, wrapCode } from './generate-exports';
 import {
   getRequireHandlers,
   getRequireStringArg,
-  hasDynamicModuleForPath,
-  isIgnoredRequireStatement,
+  hasDynamicArguments,
   isModuleRequire,
   isNodeRequirePropertyAccess,
-  isRequireStatement,
-  isStaticRequireStatement
+  isRequire,
+  isRequireExpression
 } from './generate-imports';
 import { IS_WRAPPED_COMMONJS } from './helpers';
 import { tryParse } from './parse';
@@ -77,12 +76,6 @@ export default async function transformCommonjs(
   // 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');
-
   const { addRequireStatement, rewriteRequireExpressionsAndGetImportBlock } = getRequireHandlers();
 
   // See which names are assigned to. This is necessary to prevent
@@ -98,6 +91,8 @@ export default async function transformCommonjs(
   const exportsAssignmentsByName = new Map();
   const topLevelAssignments = new Set();
   const topLevelDefineCompiledEsmExpressions = [];
+  const replacedGlobal = [];
+  const replacedDynamicRequires = [];
 
   walk(ast, {
     enter(node, parent) {
@@ -198,12 +193,12 @@ export default async function transformCommonjs(
 
           // Transform require.resolve
           if (
+            isDynamicRequireModulesEnabled &&
             node.callee.object &&
-            node.callee.object.name === 'require' &&
-            node.callee.property.name === 'resolve' &&
-            hasDynamicModuleForPath(id, '/', dynamicRequireModules)
+            isRequire(node.callee.object, scope) &&
+            node.callee.property.name === 'resolve'
           ) {
-            // TODO Lukas reimplement?
+            checkDynamicRequire();
             uses.require = true;
             const requireNode = node.callee.object;
             magicString.appendLeft(
@@ -212,25 +207,38 @@ export default async function transformCommonjs(
                 dirname(id) === '.' ? null /* default behavior */ : virtualDynamicRequirePath
               )}`
             );
-            magicString.overwrite(requireNode.start, requireNode.end, dynamicRequireName, {
-              storeName: true
-            });
+            replacedDynamicRequires.push(requireNode);
             return;
           }
 
-          // Ignore call expressions of dynamic requires, the callee will be transformed within Identifier
-          if (!isStaticRequireStatement(node, scope)) {
+          if (!isRequireExpression(node, scope)) {
             return;
           }
 
-          // Otherwise we do not want to replace "require" with the internal helper
           skippedNodes.add(node.callee);
           uses.require = true;
 
-          if (!isIgnoredRequireStatement(node, ignoreRequire)) {
+          if (hasDynamicArguments(node)) {
+            if (isDynamicRequireModulesEnabled) {
+              magicString.appendLeft(
+                node.end - 1,
+                `, ${JSON.stringify(
+                  dirname(id) === '.' ? null /* default behavior */ : virtualDynamicRequirePath
+                )}`
+              );
+            }
+            if (!ignoreDynamicRequires) {
+              checkDynamicRequire();
+              replacedDynamicRequires.push(node.callee);
+            }
+            return;
+          }
+
+          const requireStringArg = getRequireStringArg(node);
+          if (!ignoreRequire(requireStringArg)) {
             const usesReturnValue = parent.type !== 'ExpressionStatement';
             addRequireStatement(
-              getRequireStringArg(node),
+              requireStringArg,
               node,
               scope,
               usesReturnValue,
@@ -275,35 +283,14 @@ export default async function transformCommonjs(
             case 'require':
               uses.require = true;
               if (isNodeRequirePropertyAccess(parent)) {
-                // TODO Lukas reimplement?
-                if (hasDynamicModuleForPath(id, '/', dynamicRequireModules)) {
-                  if (parent.property.name === 'cache') {
-                    magicString.overwrite(node.start, node.end, dynamicRequireName, {
-                      storeName: true
-                    });
-                  }
-                }
-
                 return;
               }
-
-              if (isDynamicRequireModulesEnabled && isRequireStatement(parent, scope)) {
-                magicString.appendLeft(
-                  parent.end - 1,
-                  `, ${JSON.stringify(
-                    dirname(id) === '.' ? null /* default behavior */ : virtualDynamicRequirePath
-                  )}`
-                );
-              }
               if (!ignoreDynamicRequires) {
                 checkDynamicRequire();
                 if (isShorthandProperty(parent)) {
-                  magicString.appendRight(node.end, `: ${dynamicRequireName}`);
-                } else {
-                  magicString.overwrite(node.start, node.end, dynamicRequireName, {
-                    storeName: true
-                  });
+                  magicString.prependRight(node.start, 'require: ');
                 }
+                replacedDynamicRequires.push(node);
               }
               return;
             case 'module':
@@ -314,9 +301,7 @@ export default async function transformCommonjs(
             case 'global':
               uses.global = true;
               if (!ignoreGlobal) {
-                magicString.overwrite(node.start, node.end, `${helpersName}.commonjsGlobal`, {
-                  storeName: true
-                });
+                replacedGlobal.push(node);
               }
               return;
             case 'define':
@@ -348,9 +333,7 @@ export default async function transformCommonjs(
         case 'MemberExpression':
           if (!isDynamicRequireModulesEnabled && isModuleRequire(node, scope)) {
             uses.require = true;
-            magicString.overwrite(node.start, node.end, dynamicRequireName, {
-              storeName: true
-            });
+            replacedDynamicRequires.push(node);
             skippedNodes.add(node.object);
             skippedNodes.add(node.property);
           }
@@ -366,16 +349,17 @@ export default async function transformCommonjs(
           if (lexicalDepth === 0) {
             uses.global = true;
             if (!ignoreGlobal) {
-              magicString.overwrite(node.start, node.end, `${helpersName}.commonjsGlobal`, {
-                storeName: true
-              });
+              replacedGlobal.push(node);
             }
           }
           return;
         case 'TryStatement':
           if (currentTryBlockEnd === null) {
             currentTryBlockEnd = node.block.end;
           }
+          if (currentConditionalNodeEnd === null) {
+            currentConditionalNodeEnd = node.end;
+          }
           return;
         case 'UnaryExpression':
           // rewrite `typeof module`, `typeof module.exports` and `typeof exports` (https://github.com/rollup/rollup-plugin-commonjs/issues/151)
@@ -415,11 +399,25 @@ export default async function transformCommonjs(
   const moduleName = deconflict([...moduleAccessScopes], globals, `${nameBase}Module`);
   const requireName = deconflict([scope], globals, `require${capitalize(nameBase)}`);
   const isRequiredName = deconflict([scope], globals, `hasRequired${capitalize(nameBase)}`);
+  const helpersName = deconflict([scope], globals, 'commonjsHelpers');
+  const dynamicRequireName = deconflict([scope], globals, 'commonjsRequire');
   const deconflictedExportNames = Object.create(null);
   for (const [exportName, { scopes }] of exportsAssignmentsByName) {
     deconflictedExportNames[exportName] = deconflict([...scopes], globals, exportName);
   }
 
+  for (const node of replacedGlobal) {
+    magicString.overwrite(node.start, node.end, `${helpersName}.commonjsGlobal`, {
+      storeName: true
+    });
+  }
+  for (const node of replacedDynamicRequires) {
+    magicString.overwrite(node.start, node.end, dynamicRequireName, {
+      contentOnly: true,
+      storeName: true
+    });
+  }
+
   // We cannot wrap ES/mixed modules
   shouldWrap = !isEsModule && (shouldWrap || (uses.exports && moduleExportsAssignments.length > 0));
   const detectWrappedDefault =

diff --git a/packages/commonjs/test/fixtures/function/dynamic-module-require/_config.js b/packages/commonjs/test/fixtures/function/dynamic-module-require/_config.js
@@ -1,6 +1,4 @@
 module.exports = {
-  // TODO Lukas think about a way to re-implement
-  skip: true,
   description: 'supports dynamic require',
   pluginOptions: {
     dynamicRequireTargets: ['fixtures/function/dynamic-module-require/submodule.js']

diff --git a/packages/commonjs/test/fixtures/function/dynamic-require-cache-reference/_config.js b/packages/commonjs/test/fixtures/function/dynamic-require-cache-reference/_config.js
diff --git a/packages/commonjs/test/fixtures/function/dynamic-require-cache-reference/main.js b/packages/commonjs/test/fixtures/function/dynamic-require-cache-reference/main.js
diff --git a/...est/fixtures/function/dynamic-require-cache-reference/node_modules/custom-module/index.js b/...est/fixtures/function/dynamic-require-cache-reference/node_modules/custom-module/index.js
diff --git a/...st/fixtures/function/dynamic-require-cache-reference/node_modules/custom-module2/index.js b/...st/fixtures/function/dynamic-require-cache-reference/node_modules/custom-module2/index.js
diff --git a/...njs/test/fixtures/function/dynamic-require-cache-reference/node_modules/stealthy/index.js b/...njs/test/fixtures/function/dynamic-require-cache-reference/node_modules/stealthy/index.js
diff --git a/packages/commonjs/test/fixtures/function/dynamic-require-resolve-reference/main.js b/packages/commonjs/test/fixtures/function/dynamic-require-resolve-reference/main.js
@@ -1,3 +1,7 @@
+t.is(
+  require('custom-module')(),
+  '/fixtures/function/dynamic-require-resolve-reference/node_modules/custom-module2'
+);
 t.is(
   require('custom-module2')(),
   '/fixtures/function/dynamic-require-resolve-reference/node_modules/custom-module'

diff --git a/...t/fixtures/function/dynamic-require-resolve-reference/node_modules/custom-module/index.js b/...t/fixtures/function/dynamic-require-resolve-reference/node_modules/custom-module/index.js
diff --git a/.../fixtures/function/dynamic-require-resolve-reference/node_modules/custom-module2/index.js b/.../fixtures/function/dynamic-require-resolve-reference/node_modules/custom-module2/index.js
diff --git a/packages/commonjs/test/fixtures/function/try-catch-internal/main.js b/packages/commonjs/test/fixtures/function/try-catch-internal/main.js
@@ -5,3 +5,9 @@ try {
 } catch (err) {
   throw new Error(`Could not require: ${err}`);
 }
+
+try {
+  require('./throws.js');
+} catch (err) {
+  t.is(err.message, 'Expected error');
+}
diff --git a/packages/commonjs/test/fixtures/function/try-catch-internal/throws.js b/packages/commonjs/test/fixtures/function/try-catch-internal/throws.js
@@ -0,0 +1 @@
+throw new Error('Expected error');