fix(commonjs): treat a require in a catch clause as guarded

packages/commonjs/README.md

@@ -184,12 +184,14 @@ Default: `true`
 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 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.
+- `true`: All external `require` calls inside a `try-catch` statement will be left unconverted.
+- `false`: All external `require` calls inside a `try-catch` statement will be converted as if the `try-catch` clause is not there.
+- `remove`: Remove all external `require` calls from inside any `try-catch` statement.
 - `string[]`: Pass an array containing the IDs to left unconverted.
 - `((id: string) => boolean|'remove')`: Pass a function that control individual IDs.
 
+This covers the whole statement, so a `require` in the `catch` or `finally` clause is treated the same as one in the `try` block.
+
 Note that non-external requires will not be ignored by this option.
 
 ### `ignoreDynamicRequires`

packages/commonjs/src/generate-imports.js

@@ -65,7 +65,7 @@ export function getRequireHandlers() {
     node,
     scope,
     usesReturnValue,
-    isInsideTryBlock,
+    isInsideTryStatement,
     isInsideConditional,
     toBeRemoved
   ) {
@@ -74,7 +74,7 @@ export function getRequireHandlers() {
       node,
       scope,
       usesReturnValue,
-      isInsideTryBlock,
+      isInsideTryStatement,
       isInsideConditional,
       toBeRemoved
     });
@@ -170,9 +170,12 @@ function processRequireExpressions(
     const name = generateRequireName(requires);
     let usesRequired = false;
     let needsImport = false;
-    for (const { node, usesReturnValue, toBeRemoved, isInsideTryBlock } of requires) {
+    for (const { node, usesReturnValue, toBeRemoved, isInsideTryStatement } of requires) {
+      // A `require` anywhere inside a try statement is guarded, including in the catch
+      // clause: hoisting it to a top-level import would turn a failure the author chose to
+      // handle into an unconditional module-evaluation error.
       const { canConvertRequire, shouldRemoveRequire } =
-        isInsideTryBlock && isWrappedId(resolvedId, EXTERNAL_SUFFIX)
+        isInsideTryStatement && isWrappedId(resolvedId, EXTERNAL_SUFFIX)
           ? getIgnoreTryCatchRequireStatementMode(source)
           : { canConvertRequire: true, shouldRemoveRequire: false };
       if (shouldRemoveRequire) {

packages/commonjs/src/transform-commonjs.js

@@ -79,6 +79,7 @@ export default async function transformCommonjs(
   let programDepth = 0;
   let classBodyDepth = 0;
   let currentTryBlockEnd = null;
+  let currentTryStatementEnd = null;
   let shouldWrap = false;
 
   const globals = new Set();
@@ -118,6 +119,9 @@ export default async function transformCommonjs(
       if (currentTryBlockEnd !== null && node.start > currentTryBlockEnd) {
         currentTryBlockEnd = null;
       }
+      if (currentTryStatementEnd !== null && node.start > currentTryStatementEnd) {
+        currentTryStatementEnd = null;
+      }
       if (currentConditionalNodeEnd !== null && node.start > currentConditionalNodeEnd) {
         currentConditionalNodeEnd = null;
       }
@@ -249,7 +253,7 @@ export default async function transformCommonjs(
               node,
               scope,
               usesReturnValue,
-              currentTryBlockEnd !== null,
+              currentTryStatementEnd !== null,
               currentConditionalNodeEnd !== null,
               toBeRemoved
             );
@@ -383,6 +387,9 @@ export default async function transformCommonjs(
           if (currentTryBlockEnd === null) {
             currentTryBlockEnd = node.block.end;
           }
+          if (currentTryStatementEnd === null) {
+            currentTryStatementEnd = node.end;
+          }
           if (currentConditionalNodeEnd === null) {
             currentConditionalNodeEnd = node.end;
           }

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

@@ -0,0 +1,3 @@
+module.exports = {
+  description: 'does not hoist a require in a catch clause into a top-level import'
+};

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

@@ -0,0 +1,13 @@
+/* eslint-disable global-require */
+
+let handled = 'no error';
+
+try {
+  handled = 'no error';
+} catch (error) {
+  // The handler never runs here, but the require must still stay inside it. Hoisting it
+  // to a top-level import would make this module fail to evaluate at all.
+  handled = require('uninstalled-external-module');
+}
+
+t.is(handled, 'no error');

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

@@ -10678,6 +10678,45 @@ module.exports = main;
 }
 `;
 
+exports[`try-catch-handler 1`] = `
+{
+  "main.js": "'use strict';
+
+function getDefaultExportFromCjs (x) {
+	return x && x.__esModule && Object.prototype.hasOwnProperty.call(x, 'default') ? x['default'] : x;
+}
+
+var main$1 = {};
+
+/* eslint-disable global-require */
+
+var hasRequiredMain;
+
+function requireMain () {
+	if (hasRequiredMain) return main$1;
+	hasRequiredMain = 1;
+	let handled = 'no error';
+
+	try {
+	  handled = 'no error';
+	} catch (error) {
+	  // The handler never runs here, but the require must still stay inside it. Hoisting it
+	  // to a top-level import would make this module fail to evaluate at all.
+	  handled = require('uninstalled-external-module');
+	}
+
+	t.is(handled, 'no error');
+	return main$1;
+}
+
+var mainExports = requireMain();
+var main = /*@__PURE__*/getDefaultExportFromCjs(mainExports);
+
+module.exports = main;
+",
+}
+`;
+
 exports[`try-catch-ids-array 1`] = `
 {
   "main.js": "'use strict';