diff --git a/packages/commonjs/README.md b/packages/commonjs/README.md index 9f8724204..127b7e688 100644 --- a/packages/commonjs/README.md +++ b/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)`
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` diff --git a/packages/commonjs/src/generate-imports.js b/packages/commonjs/src/generate-imports.js index 34f66e518..7aa0c33a7 100644 --- a/packages/commonjs/src/generate-imports.js +++ b/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)};`); + } } } } diff --git a/packages/commonjs/src/index.js b/packages/commonjs/src/index.js index fe73d0f8d..45eb23c98 100644 --- a/packages/commonjs/src/index.js +++ b/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' }; }; diff --git a/packages/commonjs/src/transform-commonjs.js b/packages/commonjs/src/transform-commonjs.js index f0c43c362..7b53b5ab8 100644 --- a/packages/commonjs/src/transform-commonjs.js +++ b/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 ? '' diff --git a/packages/commonjs/test/fixtures/form/try-catch-remove/_config.js b/packages/commonjs/test/fixtures/form/try-catch-remove/_config.js deleted file mode 100644 index de5563f0d..000000000 --- a/packages/commonjs/test/fixtures/form/try-catch-remove/_config.js +++ /dev/null @@ -1,5 +0,0 @@ -module.exports = { - options: { - ignoreTryCatch: (id) => id === 'uninstalled-external-module' ? 'remove' : false - } -}; diff --git a/packages/commonjs/test/fixtures/form/try-catch-remove/output.js b/packages/commonjs/test/fixtures/form/try-catch-remove/output.js deleted file mode 100644 index e18f11df0..000000000 --- a/packages/commonjs/test/fixtures/form/try-catch-remove/output.js +++ /dev/null @@ -1,12 +0,0 @@ -/* eslint-disable global-require */ -import * as commonjsHelpers from "_commonjsHelpers.js"; -import { commonjsRequire as commonjsRequire } from "_commonjs-dynamic-modules"; -import { __exports as input } from "\u0000fixtures/form/try-catch-remove/input.js?commonjs-exports" - -try { - -} catch (ignored) { - /* ignore */ -} - -export { input as __moduleExports, input as default }; diff --git a/packages/commonjs/test/fixtures/function/try-catch-internal/_config.js b/packages/commonjs/test/fixtures/function/try-catch-internal/_config.js new file mode 100644 index 000000000..fec6f7626 --- /dev/null +++ b/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 + } +}; diff --git a/packages/commonjs/test/fixtures/function/try-catch-internal/dep.js b/packages/commonjs/test/fixtures/function/try-catch-internal/dep.js new file mode 100644 index 000000000..94ecacb72 --- /dev/null +++ b/packages/commonjs/test/fixtures/function/try-catch-internal/dep.js @@ -0,0 +1 @@ +exports.foo = 'foo'; diff --git a/packages/commonjs/test/fixtures/function/try-catch-internal/main.js b/packages/commonjs/test/fixtures/function/try-catch-internal/main.js new file mode 100644 index 000000000..df773bb29 --- /dev/null +++ b/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}`); +} diff --git a/packages/commonjs/test/fixtures/function/try-catch-remove/_config.js b/packages/commonjs/test/fixtures/function/try-catch-remove/_config.js new file mode 100644 index 000000000..c7d4363f4 --- /dev/null +++ b/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) + } +}; diff --git a/packages/commonjs/test/fixtures/form/try-catch-remove/input.js b/packages/commonjs/test/fixtures/function/try-catch-remove/main.js similarity index 56% rename from packages/commonjs/test/fixtures/form/try-catch-remove/input.js rename to packages/commonjs/test/fixtures/function/try-catch-remove/main.js index 1aae6b291..fa5fa693e 100644 --- a/packages/commonjs/test/fixtures/form/try-catch-remove/input.js +++ b/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.'); } diff --git a/packages/commonjs/test/snapshots/function.js.md b/packages/commonjs/test/snapshots/function.js.md index e09892341..68e51acb0 100644 --- a/packages/commonjs/test/snapshots/function.js.md +++ b/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;␊ + `, + } diff --git a/packages/commonjs/test/snapshots/function.js.snap b/packages/commonjs/test/snapshots/function.js.snap index 732515802..568120c83 100644 Binary files a/packages/commonjs/test/snapshots/function.js.snap and b/packages/commonjs/test/snapshots/function.js.snap differ