diff --git a/packages/commonjs/README.md b/packages/commonjs/README.md index 44a7652db..225ed182c 100644 --- a/packages/commonjs/README.md +++ b/packages/commonjs/README.md @@ -68,6 +68,8 @@ You can also provide a [minimatch pattern](https://github.com/isaacs/minimatch), Type: `string | string[]`
Default: `[]` +_Note: In previous versions, this option would spin up a rather comprehensive mock environment that was capable of handling modules that manipulate `require.cache`. This is no longer supported. If you rely on this e.g. when using request-promise-native, use version 21 of this plugin._ + Some modules contain dynamic `require` calls, or require modules that contain circular dependencies, which are not handled well by static imports. Including those modules as `dynamicRequireTargets` will simulate a CommonJS (NodeJS-like) environment for them with support for dynamic dependencies. It also enables `strictRequires` for those modules, see above. diff --git a/packages/commonjs/src/index.js b/packages/commonjs/src/index.js index b53fa7fd4..3c8f5f642 100644 --- a/packages/commonjs/src/index.js +++ b/packages/commonjs/src/index.js @@ -99,7 +99,7 @@ export default function commonjs(options = {}) { }; }; - const resolveId = getResolveId(extensions); + const { currentlyResolving, resolveId } = getResolveId(extensions); const sourceMap = options.sourceMap !== false; @@ -204,7 +204,11 @@ export default function commonjs(options = {}) { 'The namedExports option from "@rollup/plugin-commonjs" is deprecated. Named exports are now handled automatically.' ); } - requireResolver = getRequireResolver(extensions, detectCyclesAndConditional); + requireResolver = getRequireResolver( + extensions, + detectCyclesAndConditional, + currentlyResolving + ); }, buildEnd() { @@ -260,15 +264,13 @@ export default function commonjs(options = {}) { // entry suffix is just appended to not mess up relative external resolution if (id.endsWith(ENTRY_SUFFIX)) { - return getEntryProxy( - id.slice(0, -ENTRY_SUFFIX.length), - defaultIsModuleExports, - this.getModuleInfo - ); + const acutalId = id.slice(0, -ENTRY_SUFFIX.length); + return getEntryProxy(acutalId, getDefaultIsModuleExports(acutalId), this.getModuleInfo); } if (isWrappedId(id, ES_IMPORT_SUFFIX)) { - return getEsImportProxy(unwrapId(id, ES_IMPORT_SUFFIX), defaultIsModuleExports); + const actualId = unwrapId(id, ES_IMPORT_SUFFIX); + return getEsImportProxy(actualId, getDefaultIsModuleExports(actualId)); } if (id === DYNAMIC_MODULES_ID) { diff --git a/packages/commonjs/src/resolve-id.js b/packages/commonjs/src/resolve-id.js index 43066afad..7fa7f04fe 100644 --- a/packages/commonjs/src/resolve-id.js +++ b/packages/commonjs/src/resolve-id.js @@ -50,79 +50,114 @@ export function resolveExtensions(importee, importer, extensions) { } export default function getResolveId(extensions) { - return async function resolveId(importee, importer, resolveOptions) { - // We assume that all requires are pre-resolved - const customOptions = resolveOptions.custom; - if (customOptions && customOptions['node-resolve'] && customOptions['node-resolve'].isRequire) { - return null; - } - if (isWrappedId(importee, WRAPPED_SUFFIX)) { - return unwrapId(importee, WRAPPED_SUFFIX); - } + const currentlyResolving = new Map(); - if ( - importee.endsWith(ENTRY_SUFFIX) || - isWrappedId(importee, MODULE_SUFFIX) || - isWrappedId(importee, EXPORTS_SUFFIX) || - isWrappedId(importee, PROXY_SUFFIX) || - isWrappedId(importee, ES_IMPORT_SUFFIX) || - isWrappedId(importee, EXTERNAL_SUFFIX) || - importee.startsWith(HELPERS_ID) || - importee === DYNAMIC_MODULES_ID - ) { - return importee; - } + return { + /** + * This is a Maps of importers to Sets of require sources being resolved at + * the moment by resolveRequireSourcesAndUpdateMeta + */ + currentlyResolving, + async resolveId(importee, importer, resolveOptions) { + const customOptions = resolveOptions.custom; + // All logic below is specific to ES imports. + // Also, if we do not skip this logic for requires that are resolved while + // transforming a commonjs file, it can easily lead to deadlocks. + if ( + customOptions && + customOptions['node-resolve'] && + customOptions['node-resolve'].isRequire + ) { + return null; + } + const currentlyResolvingForParent = currentlyResolving.get(importer); + if (currentlyResolvingForParent && currentlyResolvingForParent.has(importee)) { + this.warn({ + code: 'THIS_RESOLVE_WITHOUT_OPTIONS', + message: + 'It appears a plugin has implemented a "resolveId" hook that uses "this.resolve" without forwarding the third "options" parameter of "resolveId". This is problematic as it can lead to wrong module resolutions especially for the node-resolve plugin and in certain cases cause early exit errors for the commonjs plugin.\nIn rare cases, this warning can appear if the same file is both imported and required from the same mixed ES/CommonJS module, in which case it can be ignored.', + url: 'https://rollupjs.org/guide/en/#resolveid' + }); + return null; + } + + if (isWrappedId(importee, WRAPPED_SUFFIX)) { + return unwrapId(importee, WRAPPED_SUFFIX); + } - if (importer) { if ( - importer === DYNAMIC_MODULES_ID || - // Proxies are only importing resolved ids, no need to resolve again - isWrappedId(importer, PROXY_SUFFIX) || - isWrappedId(importer, ES_IMPORT_SUFFIX) || - importer.endsWith(ENTRY_SUFFIX) + importee.endsWith(ENTRY_SUFFIX) || + isWrappedId(importee, MODULE_SUFFIX) || + isWrappedId(importee, EXPORTS_SUFFIX) || + isWrappedId(importee, PROXY_SUFFIX) || + isWrappedId(importee, ES_IMPORT_SUFFIX) || + isWrappedId(importee, EXTERNAL_SUFFIX) || + importee.startsWith(HELPERS_ID) || + importee === DYNAMIC_MODULES_ID ) { return importee; } - if (isWrappedId(importer, EXTERNAL_SUFFIX)) { - // We need to return null for unresolved imports so that the proper warning is shown - if (!(await this.resolve(importee, importer, { skipSelf: true }))) { - return null; + + if (importer) { + if ( + importer === DYNAMIC_MODULES_ID || + // Proxies are only importing resolved ids, no need to resolve again + isWrappedId(importer, PROXY_SUFFIX) || + isWrappedId(importer, ES_IMPORT_SUFFIX) || + importer.endsWith(ENTRY_SUFFIX) + ) { + return importee; + } + if (isWrappedId(importer, EXTERNAL_SUFFIX)) { + // We need to return null for unresolved imports so that the proper warning is shown + if ( + !(await this.resolve( + importee, + importer, + Object.assign({ skipSelf: true }, resolveOptions) + )) + ) { + return null; + } + // For other external imports, we need to make sure they are handled as external + return { id: importee, external: true }; } - // For other external imports, we need to make sure they are handled as external - return { id: importee, external: true }; } - } - if (importee.startsWith('\0')) { - return null; - } + if (importee.startsWith('\0')) { + return null; + } - // If this is an entry point or ESM import, we need to figure out if the importee is wrapped and - // if that is the case, we need to add a proxy. - const resolved = - (await this.resolve(importee, importer, Object.assign({ skipSelf: true }, resolveOptions))) || - resolveExtensions(importee, importer, extensions); - // Make sure that even if other plugins resolve again, we ignore our own proxies - if ( - !resolved || - resolved.external || - resolved.id.endsWith(ENTRY_SUFFIX) || - isWrappedId(resolved.id, ES_IMPORT_SUFFIX) - ) { + // If this is an entry point or ESM import, we need to figure out if the importee is wrapped and + // if that is the case, we need to add a proxy. + const resolved = + (await this.resolve( + importee, + importer, + Object.assign({ skipSelf: true }, resolveOptions) + )) || resolveExtensions(importee, importer, extensions); + // Make sure that even if other plugins resolve again, we ignore our own proxies + if ( + !resolved || + resolved.external || + resolved.id.endsWith(ENTRY_SUFFIX) || + isWrappedId(resolved.id, ES_IMPORT_SUFFIX) + ) { + return resolved; + } + const moduleInfo = await this.load(resolved); + if (resolveOptions.isEntry) { + moduleInfo.moduleSideEffects = true; + // We must not precede entry proxies with a `\0` as that will mess up relative external resolution + return resolved.id + ENTRY_SUFFIX; + } + const { + meta: { commonjs: commonjsMeta } + } = moduleInfo; + if (commonjsMeta && commonjsMeta.isCommonJS === IS_WRAPPED_COMMONJS) { + return { id: wrapId(resolved.id, ES_IMPORT_SUFFIX), meta: { commonjs: { resolved } } }; + } return resolved; } - const moduleInfo = await this.load(resolved); - if (resolveOptions.isEntry) { - moduleInfo.moduleSideEffects = true; - // We must not precede entry proxies with a `\0` as that will mess up relative external resolution - return resolved.id + ENTRY_SUFFIX; - } - const { - meta: { commonjs: commonjsMeta } - } = moduleInfo; - if (commonjsMeta && commonjsMeta.isCommonJS === IS_WRAPPED_COMMONJS) { - return { id: wrapId(resolved.id, ES_IMPORT_SUFFIX), meta: { commonjs: { resolved } } }; - } - return resolved; }; } diff --git a/packages/commonjs/src/resolve-require-sources.js b/packages/commonjs/src/resolve-require-sources.js index f96ace50e..d392a0755 100644 --- a/packages/commonjs/src/resolve-require-sources.js +++ b/packages/commonjs/src/resolve-require-sources.js @@ -9,7 +9,7 @@ import { } from './helpers'; import { resolveExtensions } from './resolve-id'; -export function getRequireResolver(extensions, detectCyclesAndConditional) { +export function getRequireResolver(extensions, detectCyclesAndConditional, currentlyResolving) { const knownCjsModuleTypes = Object.create(null); const requiredIds = Object.create(null); const unconditionallyRequiredIds = Object.create(null); @@ -161,16 +161,20 @@ export function getRequireResolver(extensions, detectCyclesAndConditional) { parentMeta.requires = []; parentMeta.isRequiredCommonJS = Object.create(null); setInitialParentType(parentId, isParentCommonJS); + const currentlyResolvingForParent = currentlyResolving.get(parentId) || new Set(); + currentlyResolving.set(parentId, currentlyResolvingForParent); const requireTargets = await Promise.all( sources.map(async ({ source, isConditional }) => { // Never analyze or proxy internal modules if (source.startsWith('\0')) { return { id: source, allowProxy: false }; } + currentlyResolvingForParent.add(source); const resolved = (await rollupContext.resolve(source, parentId, { custom: { 'node-resolve': { isRequire: true } } })) || resolveExtensions(source, parentId, extensions); + currentlyResolvingForParent.delete(source); if (!resolved) { return { id: wrapId(source, EXTERNAL_SUFFIX), allowProxy: false }; } @@ -201,6 +205,10 @@ export function getRequireResolver(extensions, detectCyclesAndConditional) { isCommonJS }; }); + }, + isCurrentlyResolving(source, parentId) { + const currentlyResolvingForParent = currentlyResolving.get(parentId); + return currentlyResolvingForParent && currentlyResolvingForParent.has(source); } }; } diff --git a/packages/commonjs/test/fixtures/function/warn-this-resolve-without-options/_config.js b/packages/commonjs/test/fixtures/function/warn-this-resolve-without-options/_config.js new file mode 100644 index 000000000..a79b74cb8 --- /dev/null +++ b/packages/commonjs/test/fixtures/function/warn-this-resolve-without-options/_config.js @@ -0,0 +1,29 @@ +const assert = require('assert'); + +const warnings = []; + +module.exports = { + description: 'Warns when another plugin uses this.resolve without forwarding options', + options: { + onwarn(warning) { + warnings.push(warning); + }, + plugins: [ + { + name: 'test', + resolveId(source, importer) { + return this.resolve(source, importer, { skipSelf: true }); + }, + buildEnd() { + assert.strictEqual(warnings.length, 1); + assert.strictEqual( + warnings[0].message, + 'It appears a plugin has implemented a "resolveId" hook that uses "this.resolve" without forwarding the third "options" parameter of "resolveId". This is problematic as it can lead to wrong module resolutions especially for the node-resolve plugin and in certain cases cause early exit errors for the commonjs plugin.\nIn rare cases, this warning can appear if the same file is both imported and required from the same mixed ES/CommonJS module, in which case it can be ignored.' + ); + assert.strictEqual(warnings[0].pluginCode, 'THIS_RESOLVE_WITHOUT_OPTIONS'); + assert.strictEqual(warnings[0].url, 'https://rollupjs.org/guide/en/#resolveid'); + } + } + ] + } +}; diff --git a/packages/commonjs/test/fixtures/function/warn-this-resolve-without-options/foo.js b/packages/commonjs/test/fixtures/function/warn-this-resolve-without-options/foo.js new file mode 100644 index 000000000..ce0fffb75 --- /dev/null +++ b/packages/commonjs/test/fixtures/function/warn-this-resolve-without-options/foo.js @@ -0,0 +1 @@ +module.exports = 21; diff --git a/packages/commonjs/test/fixtures/function/warn-this-resolve-without-options/main.js b/packages/commonjs/test/fixtures/function/warn-this-resolve-without-options/main.js new file mode 100644 index 000000000..ba12948f6 --- /dev/null +++ b/packages/commonjs/test/fixtures/function/warn-this-resolve-without-options/main.js @@ -0,0 +1,3 @@ +const foo = require('./foo'); + +module.exports = foo * 2; diff --git a/packages/commonjs/test/snapshots/function.js.md b/packages/commonjs/test/snapshots/function.js.md index 8f1eaa57b..f34b58c15 100644 --- a/packages/commonjs/test/snapshots/function.js.md +++ b/packages/commonjs/test/snapshots/function.js.md @@ -7713,3 +7713,20 @@ Generated by [AVA](https://avajs.dev). module.exports = main;␊ `, } + +## warn-this-resolve-without-options + +> Snapshot 1 + + { + 'main.js': `'use strict';␊ + ␊ + var foo$1 = 21;␊ + ␊ + const foo = foo$1;␊ + ␊ + var main = foo * 2;␊ + ␊ + module.exports = main;␊ + `, + } diff --git a/packages/commonjs/test/snapshots/function.js.snap b/packages/commonjs/test/snapshots/function.js.snap index c4fac9a4f..e9e8b3c33 100644 Binary files a/packages/commonjs/test/snapshots/function.js.snap and b/packages/commonjs/test/snapshots/function.js.snap differ