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';