feat(commonjs): limit ignoreTryCatch to external requires (#1038)

packages/commonjs/README.md

@@ -144,15 +144,17 @@ Sometimes you have to leave require statements unconverted. Pass an array contai
 Type: `boolean | 'remove' | string[] | ((id: string) => boolean)`<br>
 Default: `true`
 
-In most cases, where `require` calls are inside a `try-catch` clause, they should be left unconverted as it requires an optional dependency that may or may not be installed beside the rolled up package.
+In most cases, where `require` calls to external dependencies are inside a `try-catch` clause, they should be left unconverted as it requires an optional dependency that may or may not be installed beside the rolled up package.
 Due to the conversion of `require` to a static `import` - the call is hoisted to the top of the file, outside of the `try-catch` clause.
 
-- `true`: All `require` calls inside a `try` will be left unconverted.
-- `false`: All `require` calls inside a `try` will be converted as if the `try-catch` clause is not there.
-- `remove`: Remove all `require` calls from inside any `try` block.
+- `true`: All external `require` calls inside a `try` will be left unconverted.
+- `false`: All external `require` calls inside a `try` will be converted as if the `try-catch` clause is not there.
+- `remove`: Remove all external `require` calls from inside any `try` block.
 - `string[]`: Pass an array containing the IDs to left unconverted.
 - `((id: string) => boolean|'remove')`: Pass a function that control individual IDs.
 
+Note that non-external requires will not be ignored by this option.
+
 ### `ignoreDynamicRequires`
 
 Type: `boolean`

packages/commonjs/src/generate-imports.js

@@ -5,8 +5,10 @@ import { sync as nodeResolveSync } from 'resolve';
 import {
   DYNAMIC_MODULES_ID,
   EXPORTS_SUFFIX,
+  EXTERNAL_SUFFIX,
   HELPERS_ID,
   IS_WRAPPED_COMMONJS,
+  isWrappedId,
   MODULE_SUFFIX,
   wrapId
 } from './helpers';
@@ -96,14 +98,27 @@ export function hasDynamicModuleForPath(source, id, dynamicRequireModules) {
 export function getRequireHandlers() {
   const requireExpressions = [];
 
-  function addRequireStatement(sourceId, node, scope, usesReturnValue, toBeRemoved) {
-    requireExpressions.push({ sourceId, node, scope, usesReturnValue, toBeRemoved });
+  function addRequireStatement(
+    sourceId,
+    node,
+    scope,
+    usesReturnValue,
+    isInsideTryBlock,
+    toBeRemoved
+  ) {
+    requireExpressions.push({
+      sourceId,
+      node,
+      scope,
+      usesReturnValue,
+      isInsideTryBlock,
+      toBeRemoved
+    });
   }
 
   async function rewriteRequireExpressionsAndGetImportBlock(
     magicString,
     topLevelDeclarations,
-    topLevelRequireDeclarators,
     reassignedNames,
     helpersName,
     dynamicRequireName,
@@ -114,7 +129,8 @@ export function getRequireHandlers() {
     resolveRequireSourcesAndGetMeta,
     needsRequireWrapper,
     isEsModule,
-    usesRequire
+    usesRequire,
+    getIgnoreTryCatchRequireStatementMode
   ) {
     const imports = [];
     imports.push(`import * as ${helpersName} from "${HELPERS_ID}";`);
@@ -141,7 +157,13 @@ export function getRequireHandlers() {
       needsRequireWrapper ? IS_WRAPPED_COMMONJS : !isEsModule,
       Object.keys(requiresBySource)
     );
-    processRequireExpressions(imports, requireTargets, requiresBySource, magicString);
+    processRequireExpressions(
+      imports,
+      requireTargets,
+      requiresBySource,
+      getIgnoreTryCatchRequireStatementMode,
+      magicString
+    );
     return {
       importBlock: imports.length ? `${imports.join('\n')}\n\n` : '',
       usesRequireWrapper
@@ -156,37 +178,59 @@ export function getRequireHandlers() {
 
 function collectSources(requireExpressions) {
   const requiresBySource = Object.create(null);
-  for (const { sourceId, node, scope, usesReturnValue, toBeRemoved } of requireExpressions) {
+  for (const requireExpression of requireExpressions) {
+    const { sourceId } = requireExpression;
     if (!requiresBySource[sourceId]) {
       requiresBySource[sourceId] = [];
     }
     const requires = requiresBySource[sourceId];
-    requires.push({ node, scope, usesReturnValue, toBeRemoved });
+    requires.push(requireExpression);
   }
   return requiresBySource;
 }
 
-function processRequireExpressions(imports, requireTargets, requiresBySource, magicString) {
+function processRequireExpressions(
+  imports,
+  requireTargets,
+  requiresBySource,
+  getIgnoreTryCatchRequireStatementMode,
+  magicString
+) {
   const generateRequireName = getGenerateRequireName();
-  for (const { source, id: resolveId, isCommonJS } of requireTargets) {
+  for (const { source, id: resolvedId, isCommonJS } of requireTargets) {
     const requires = requiresBySource[source];
     const name = generateRequireName(requires);
-    if (isCommonJS === IS_WRAPPED_COMMONJS) {
-      for (const { node } of requires) {
-        magicString.overwrite(node.start, node.end, `${name}()`);
-      }
-      imports.push(`import { __require as ${name} } from ${JSON.stringify(resolveId)};`);
-    } else {
-      let usesRequired = false;
-      for (const { node, usesReturnValue, toBeRemoved } of requires) {
+    let usesRequired = false;
+    let needsImport = false;
+    for (const { node, usesReturnValue, toBeRemoved, isInsideTryBlock } of requires) {
+      const { canConvertRequire, shouldRemoveRequire } =
+        isInsideTryBlock && isWrappedId(resolvedId, EXTERNAL_SUFFIX)
+          ? getIgnoreTryCatchRequireStatementMode(source)
+          : { canConvertRequire: true, shouldRemoveRequire: false };
+      if (shouldRemoveRequire) {
         if (usesReturnValue) {
+          magicString.overwrite(node.start, node.end, 'undefined');
+        } else {
+          magicString.remove(toBeRemoved.start, toBeRemoved.end);
+        }
+      } else if (canConvertRequire) {
+        needsImport = true;
+        if (isCommonJS === IS_WRAPPED_COMMONJS) {
+          magicString.overwrite(node.start, node.end, `${name}()`);
+        } else if (usesReturnValue) {
           usesRequired = true;
           magicString.overwrite(node.start, node.end, name);
         } else {
           magicString.remove(toBeRemoved.start, toBeRemoved.end);
         }
       }
-      imports.push(`import ${usesRequired ? `${name} from ` : ''}${JSON.stringify(resolveId)};`);
+    }
+    if (needsImport) {
+      if (isCommonJS === IS_WRAPPED_COMMONJS) {
+        imports.push(`import { __require as ${name} } from ${JSON.stringify(resolvedId)};`);
+      } else {
+        imports.push(`import ${usesRequired ? `${name} from ` : ''}${JSON.stringify(resolvedId)};`);
+      }
     }
   }
 }

packages/commonjs/src/index.js

@@ -93,7 +93,7 @@ export default function commonjs(options = {}) {
 
     return {
       canConvertRequire: mode !== 'remove' && mode !== true,
-      shouldRemoveRequireStatement: mode === 'remove'
+      shouldRemoveRequire: mode === 'remove'
     };
   };
 

packages/commonjs/src/transform-commonjs.js

@@ -8,8 +8,8 @@ import MagicString from 'magic-string';
 
 import {
   getKeypath,
-  isDefineCompiledEsm,
   hasDefineEsmProperty,
+  isDefineCompiledEsm,
   isFalsy,
   isReference,
   isShorthandProperty,
@@ -75,7 +75,6 @@ export default async function transformCommonjs(
   // TODO technically wrong since globals isn't populated yet, but ¯\_(ツ)_/¯
   const helpersName = deconflict([scope], globals, 'commonjsHelpers');
   const dynamicRequireName = deconflict([scope], globals, 'commonjsRequire');
-  let hasRemovedRequire = false;
 
   const { addRequireStatement, rewriteRequireExpressionsAndGetImportBlock } = getRequireHandlers();
 
@@ -84,7 +83,6 @@ export default async function transformCommonjs(
   // where `foo` is later reassigned. (This happens in the wild. CommonJS, sigh)
   const reassignedNames = new Set();
   const topLevelDeclarations = [];
-  const topLevelRequireDeclarators = new Set();
   const skippedNodes = new Set();
   const moduleAccessScopes = new Set([scope]);
   const exportsAccessScopes = new Set([scope]);
@@ -223,51 +221,14 @@ export default async function transformCommonjs(
 
           if (!isIgnoredRequireStatement(node, ignoreRequire)) {
             const usesReturnValue = parent.type !== 'ExpressionStatement';
-
-            let canConvertRequire = true;
-            let shouldRemoveRequireStatement = false;
-
-            if (currentTryBlockEnd !== null) {
-              const ignoreTryCatchRequire = getIgnoreTryCatchRequireStatementMode(
-                node.arguments[0].value
-              );
-              ({ canConvertRequire, shouldRemoveRequireStatement } = ignoreTryCatchRequire);
-              if (shouldRemoveRequireStatement) {
-                hasRemovedRequire = true;
-              }
-            }
-
-            const sourceId = getRequireStringArg(node);
-            if (shouldRemoveRequireStatement) {
-              if (usesReturnValue) {
-                magicString.overwrite(node.start, node.end, `undefined`);
-              } else {
-                magicString.remove(parent.start, parent.end);
-              }
-              return;
-            }
-
-            if (canConvertRequire) {
-              addRequireStatement(
-                sourceId,
-                node,
-                scope,
-                usesReturnValue,
-                parent.type === 'ExpressionStatement' ? parent : node
-              );
-            }
-
-            if (usesReturnValue) {
-              if (
-                parent.type === 'VariableDeclarator' &&
-                !scope.parent &&
-                parent.id.type === 'Identifier'
-              ) {
-                // This will allow us to reuse this variable name as the imported variable if it is not reassigned
-                // and does not conflict with variables in other places where this is imported
-                topLevelRequireDeclarators.add(parent);
-              }
-            }
+            addRequireStatement(
+              getRequireStringArg(node),
+              node,
+              scope,
+              usesReturnValue,
+              currentTryBlockEnd !== null,
+              parent.type === 'ExpressionStatement' ? parent : node
+            );
           }
           return;
         }
@@ -423,7 +384,6 @@ export default async function transformCommonjs(
       uses.module ||
       uses.exports ||
       uses.require ||
-      hasRemovedRequire ||
       topLevelDefineCompiledEsmExpressions.length > 0
     ) &&
     (ignoreGlobal || !uses.global)
@@ -453,7 +413,6 @@ export default async function transformCommonjs(
   const { importBlock, usesRequireWrapper } = await rewriteRequireExpressionsAndGetImportBlock(
     magicString,
     topLevelDeclarations,
-    topLevelRequireDeclarators,
     reassignedNames,
     helpersName,
     dynamicRequireName,
@@ -464,7 +423,8 @@ export default async function transformCommonjs(
     resolveRequireSourcesAndGetMeta,
     needsRequireWrapper,
     isEsModule,
-    uses.require
+    uses.require,
+    getIgnoreTryCatchRequireStatementMode
   );
   const exportBlock = isEsModule
     ? ''

packages/commonjs/test/fixtures/function/try-catch-internal/_config.js

@@ -0,0 +1,7 @@
+module.exports = {
+  description:
+    'inlines internal require statements in try-catch blocks even when try-catch is ignored',
+  pluginOptions: {
+    ignoreTryCatch: true
+  }
+};

packages/commonjs/test/fixtures/function/try-catch-internal/dep.js

@@ -0,0 +1 @@
+exports.foo = 'foo';

packages/commonjs/test/fixtures/function/try-catch-internal/main.js

@@ -0,0 +1,7 @@
+/* eslint-disable global-require */
+
+try {
+  t.is(require('./dep.js').foo, 'foo');
+} catch (err) {
+  throw new Error(`Could not require: ${err}`);
+}

packages/commonjs/test/fixtures/function/try-catch-remove/_config.js

@@ -0,0 +1,5 @@
+module.exports = {
+  pluginOptions: {
+    ignoreTryCatch: (id) => (id === 'uninstalled-external-module' ? 'remove' : false)
+  }
+};

packages/commonjs/test/fixtures/form/try-catch-remove/input.js → packages/commonjs/test/fixtures/function/try-catch-remove/main.js

@@ -3,5 +3,5 @@
 try {
   require('uninstalled-external-module');
 } catch (ignored) {
-  /* ignore */
+  throw new Error('This should no longer be reached as the require is removed.');
 }

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

@@ -7030,3 +7030,41 @@ Generated by [AVA](https://avajs.dev).
       module.exports = main;␊
       `,
     }
+
+## try-catch-internal
+
+> Snapshot 1
+
+    {
+      'main.js': `'use strict';␊
+      ␊
+      var main = {};␊
+      ␊
+      var dep = {};␊
+      ␊
+      dep.foo = 'foo';␊
+      ␊
+      /* eslint-disable global-require */␊
+      ␊
+      try {␊
+        t.is(dep.foo, 'foo');␊
+      } catch (err) {␊
+        throw new Error(`Could not require: ${err}`);␊
+      }␊
+      ␊
+      module.exports = main;␊
+      `,
+    }
+
+## try-catch-remove
+
+> Snapshot 1
+
+    {
+      'main.js': `'use strict';␊
+      ␊
+      var main = {};␊
+      ␊
+      module.exports = main;␊
+      `,
+    }