From 5031456414c6bea8c1bfeb57b6ff983fcf700262 Mon Sep 17 00:00:00 2001 From: Magnar Ovedal Myrtveit Date: Wed, 5 Aug 2026 07:35:19 +0000 Subject: [PATCH] fix(commonjs): treat a require in a catch clause as guarded `ignoreTryCatch` defaults to `true` so that a `require` of a possibly-uninstalled external is left alone rather than hoisted into a top-level import. The guard window was computed from the try block alone (`node.block.end`), so a `require` in the `catch` clause fell outside it and was converted anyway. That turns a failure the author chose to handle into an unconditional one: the emitted `import` is evaluated before any of the surrounding code runs, so the module cannot be loaded at all even when the guarded branch would never execute. When the specifier is unresolvable the import also has no chance of ever succeeding, so the whole bundle becomes unloadable. Track the end of the whole `TryStatement` separately from the end of its block, and use that for the `ignoreTryCatch` decision. `currentTryBlockEnd` is deliberately left as it was, because it also feeds the `toBeRemoved` comparison against `currentConditionalNodeEnd`; widening it in place would have changed which nodes get removed. --- packages/commonjs/README.md | 8 ++-- packages/commonjs/src/generate-imports.js | 11 ++++-- packages/commonjs/src/transform-commonjs.js | 9 ++++- .../function/try-catch-handler/_config.js | 3 ++ .../function/try-catch-handler/main.js | 13 +++++++ .../commonjs/test/snapshots/function.js.snap | 39 +++++++++++++++++++ 6 files changed, 75 insertions(+), 8 deletions(-) create mode 100644 packages/commonjs/test/fixtures/function/try-catch-handler/_config.js create mode 100644 packages/commonjs/test/fixtures/function/try-catch-handler/main.js diff --git a/packages/commonjs/README.md b/packages/commonjs/README.md index 53afe5217..85b6a039a 100644 --- a/packages/commonjs/README.md +++ b/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` diff --git a/packages/commonjs/src/generate-imports.js b/packages/commonjs/src/generate-imports.js index e2e4a965a..2bddf9b18 100644 --- a/packages/commonjs/src/generate-imports.js +++ b/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) { diff --git a/packages/commonjs/src/transform-commonjs.js b/packages/commonjs/src/transform-commonjs.js index 2bc22ae88..fb64dad61 100644 --- a/packages/commonjs/src/transform-commonjs.js +++ b/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; } diff --git a/packages/commonjs/test/fixtures/function/try-catch-handler/_config.js b/packages/commonjs/test/fixtures/function/try-catch-handler/_config.js new file mode 100644 index 000000000..f4c9d4a85 --- /dev/null +++ b/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' +}; diff --git a/packages/commonjs/test/fixtures/function/try-catch-handler/main.js b/packages/commonjs/test/fixtures/function/try-catch-handler/main.js new file mode 100644 index 000000000..2269de8bf --- /dev/null +++ b/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'); diff --git a/packages/commonjs/test/snapshots/function.js.snap b/packages/commonjs/test/snapshots/function.js.snap index 1ecce0bb1..186df8f46 100644 --- a/packages/commonjs/test/snapshots/function.js.snap +++ b/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';