Skip to content

Commit 4c34dd5

Browse files
lukastaegertlukastaegert
committed
fix(commonjs): Warn when plugins do not pass options to resolveId (#1038)
1 parent b1cd6a2 commit 4c34dd5

File tree

9 files changed

+168
-71
lines changed

9 files changed

+168
-71
lines changed

‎packages/commonjs/README.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,8 @@ You can also provide a [minimatch pattern](https://github.com/isaacs/minimatch),
6868
Type: `string | string[]`<br>
6969
Default: `[]`
7070

71+
_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._
72+
7173
Some modules contain dynamic `require` calls, or require modules that contain circular dependencies, which are not handled well by static imports.
7274
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.
7375

‎packages/commonjs/src/index.js

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ export default function commonjs(options = {}) {
9999
};
100100
};
101101

102-
const resolveId = getResolveId(extensions);
102+
const { currentlyResolving, resolveId } = getResolveId(extensions);
103103

104104
const sourceMap = options.sourceMap !== false;
105105

@@ -204,7 +204,11 @@ export default function commonjs(options = {}) {
204204
'The namedExports option from "@rollup/plugin-commonjs" is deprecated. Named exports are now handled automatically.'
205205
);
206206
}
207-
requireResolver = getRequireResolver(extensions, detectCyclesAndConditional);
207+
requireResolver = getRequireResolver(
208+
extensions,
209+
detectCyclesAndConditional,
210+
currentlyResolving
211+
);
208212
},
209213

210214
buildEnd() {
@@ -260,15 +264,13 @@ export default function commonjs(options = {}) {
260264

261265
// entry suffix is just appended to not mess up relative external resolution
262266
if (id.endsWith(ENTRY_SUFFIX)) {
263-
return getEntryProxy(
264-
id.slice(0, -ENTRY_SUFFIX.length),
265-
defaultIsModuleExports,
266-
this.getModuleInfo
267-
);
267+
const acutalId = id.slice(0, -ENTRY_SUFFIX.length);
268+
return getEntryProxy(acutalId, getDefaultIsModuleExports(acutalId), this.getModuleInfo);
268269
}
269270

270271
if (isWrappedId(id, ES_IMPORT_SUFFIX)) {
271-
return getEsImportProxy(unwrapId(id, ES_IMPORT_SUFFIX), defaultIsModuleExports);
272+
const actualId = unwrapId(id, ES_IMPORT_SUFFIX);
273+
return getEsImportProxy(actualId, getDefaultIsModuleExports(actualId));
272274
}
273275

274276
if (id === DYNAMIC_MODULES_ID) {

‎packages/commonjs/src/resolve-id.js

Lines changed: 97 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -50,79 +50,114 @@ export function resolveExtensions(importee, importer, extensions) {
5050
}
5151

5252
export default function getResolveId(extensions) {
53-
return async function resolveId(importee, importer, resolveOptions) {
54-
// We assume that all requires are pre-resolved
55-
const customOptions = resolveOptions.custom;
56-
if (customOptions && customOptions['node-resolve'] && customOptions['node-resolve'].isRequire) {
57-
return null;
58-
}
59-
if (isWrappedId(importee, WRAPPED_SUFFIX)) {
60-
return unwrapId(importee, WRAPPED_SUFFIX);
61-
}
53+
const currentlyResolving = new Map();
6254

63-
if (
64-
importee.endsWith(ENTRY_SUFFIX) ||
65-
isWrappedId(importee, MODULE_SUFFIX) ||
66-
isWrappedId(importee, EXPORTS_SUFFIX) ||
67-
isWrappedId(importee, PROXY_SUFFIX) ||
68-
isWrappedId(importee, ES_IMPORT_SUFFIX) ||
69-
isWrappedId(importee, EXTERNAL_SUFFIX) ||
70-
importee.startsWith(HELPERS_ID) ||
71-
importee === DYNAMIC_MODULES_ID
72-
) {
73-
return importee;
74-
}
55+
return {
56+
/**
57+
* This is a Maps of importers to Sets of require sources being resolved at
58+
* the moment by resolveRequireSourcesAndUpdateMeta
59+
*/
60+
currentlyResolving,
61+
async resolveId(importee, importer, resolveOptions) {
62+
const customOptions = resolveOptions.custom;
63+
// All logic below is specific to ES imports.
64+
// Also, if we do not skip this logic for requires that are resolved while
65+
// transforming a commonjs file, it can easily lead to deadlocks.
66+
if (
67+
customOptions &&
68+
customOptions['node-resolve'] &&
69+
customOptions['node-resolve'].isRequire
70+
) {
71+
return null;
72+
}
73+
const currentlyResolvingForParent = currentlyResolving.get(importer);
74+
if (currentlyResolvingForParent && currentlyResolvingForParent.has(importee)) {
75+
this.warn({
76+
code: 'THIS_RESOLVE_WITHOUT_OPTIONS',
77+
message:
78+
'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.',
79+
url: 'https://rollupjs.org/guide/en/#resolveid'
80+
});
81+
return null;
82+
}
83+
84+
if (isWrappedId(importee, WRAPPED_SUFFIX)) {
85+
return unwrapId(importee, WRAPPED_SUFFIX);
86+
}
7587

76-
if (importer) {
7788
if (
78-
importer === DYNAMIC_MODULES_ID ||
79-
// Proxies are only importing resolved ids, no need to resolve again
80-
isWrappedId(importer, PROXY_SUFFIX) ||
81-
isWrappedId(importer, ES_IMPORT_SUFFIX) ||
82-
importer.endsWith(ENTRY_SUFFIX)
89+
importee.endsWith(ENTRY_SUFFIX) ||
90+
isWrappedId(importee, MODULE_SUFFIX) ||
91+
isWrappedId(importee, EXPORTS_SUFFIX) ||
92+
isWrappedId(importee, PROXY_SUFFIX) ||
93+
isWrappedId(importee, ES_IMPORT_SUFFIX) ||
94+
isWrappedId(importee, EXTERNAL_SUFFIX) ||
95+
importee.startsWith(HELPERS_ID) ||
96+
importee === DYNAMIC_MODULES_ID
8397
) {
8498
return importee;
8599
}
86-
if (isWrappedId(importer, EXTERNAL_SUFFIX)) {
87-
// We need to return null for unresolved imports so that the proper warning is shown
88-
if (!(await this.resolve(importee, importer, { skipSelf: true }))) {
89-
return null;
100+
101+
if (importer) {
102+
if (
103+
importer === DYNAMIC_MODULES_ID ||
104+
// Proxies are only importing resolved ids, no need to resolve again
105+
isWrappedId(importer, PROXY_SUFFIX) ||
106+
isWrappedId(importer, ES_IMPORT_SUFFIX) ||
107+
importer.endsWith(ENTRY_SUFFIX)
108+
) {
109+
return importee;
110+
}
111+
if (isWrappedId(importer, EXTERNAL_SUFFIX)) {
112+
// We need to return null for unresolved imports so that the proper warning is shown
113+
if (
114+
!(await this.resolve(
115+
importee,
116+
importer,
117+
Object.assign({ skipSelf: true }, resolveOptions)
118+
))
119+
) {
120+
return null;
121+
}
122+
// For other external imports, we need to make sure they are handled as external
123+
return { id: importee, external: true };
90124
}
91-
// For other external imports, we need to make sure they are handled as external
92-
return { id: importee, external: true };
93125
}
94-
}
95126

96-
if (importee.startsWith('\0')) {
97-
return null;
98-
}
127+
if (importee.startsWith('\0')) {
128+
return null;
129+
}
99130

100-
// If this is an entry point or ESM import, we need to figure out if the importee is wrapped and
101-
// if that is the case, we need to add a proxy.
102-
const resolved =
103-
(await this.resolve(importee, importer, Object.assign({ skipSelf: true }, resolveOptions))) ||
104-
resolveExtensions(importee, importer, extensions);
105-
// Make sure that even if other plugins resolve again, we ignore our own proxies
106-
if (
107-
!resolved ||
108-
resolved.external ||
109-
resolved.id.endsWith(ENTRY_SUFFIX) ||
110-
isWrappedId(resolved.id, ES_IMPORT_SUFFIX)
111-
) {
131+
// If this is an entry point or ESM import, we need to figure out if the importee is wrapped and
132+
// if that is the case, we need to add a proxy.
133+
const resolved =
134+
(await this.resolve(
135+
importee,
136+
importer,
137+
Object.assign({ skipSelf: true }, resolveOptions)
138+
)) || resolveExtensions(importee, importer, extensions);
139+
// Make sure that even if other plugins resolve again, we ignore our own proxies
140+
if (
141+
!resolved ||
142+
resolved.external ||
143+
resolved.id.endsWith(ENTRY_SUFFIX) ||
144+
isWrappedId(resolved.id, ES_IMPORT_SUFFIX)
145+
) {
146+
return resolved;
147+
}
148+
const moduleInfo = await this.load(resolved);
149+
if (resolveOptions.isEntry) {
150+
moduleInfo.moduleSideEffects = true;
151+
// We must not precede entry proxies with a `\0` as that will mess up relative external resolution
152+
return resolved.id + ENTRY_SUFFIX;
153+
}
154+
const {
155+
meta: { commonjs: commonjsMeta }
156+
} = moduleInfo;
157+
if (commonjsMeta && commonjsMeta.isCommonJS === IS_WRAPPED_COMMONJS) {
158+
return { id: wrapId(resolved.id, ES_IMPORT_SUFFIX), meta: { commonjs: { resolved } } };
159+
}
112160
return resolved;
113161
}
114-
const moduleInfo = await this.load(resolved);
115-
if (resolveOptions.isEntry) {
116-
moduleInfo.moduleSideEffects = true;
117-
// We must not precede entry proxies with a `\0` as that will mess up relative external resolution
118-
return resolved.id + ENTRY_SUFFIX;
119-
}
120-
const {
121-
meta: { commonjs: commonjsMeta }
122-
} = moduleInfo;
123-
if (commonjsMeta && commonjsMeta.isCommonJS === IS_WRAPPED_COMMONJS) {
124-
return { id: wrapId(resolved.id, ES_IMPORT_SUFFIX), meta: { commonjs: { resolved } } };
125-
}
126-
return resolved;
127162
};
128163
}

‎packages/commonjs/src/resolve-require-sources.js

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import {
99
} from './helpers';
1010
import { resolveExtensions } from './resolve-id';
1111

12-
export function getRequireResolver(extensions, detectCyclesAndConditional) {
12+
export function getRequireResolver(extensions, detectCyclesAndConditional, currentlyResolving) {
1313
const knownCjsModuleTypes = Object.create(null);
1414
const requiredIds = Object.create(null);
1515
const unconditionallyRequiredIds = Object.create(null);
@@ -161,16 +161,20 @@ export function getRequireResolver(extensions, detectCyclesAndConditional) {
161161
parentMeta.requires = [];
162162
parentMeta.isRequiredCommonJS = Object.create(null);
163163
setInitialParentType(parentId, isParentCommonJS);
164+
const currentlyResolvingForParent = currentlyResolving.get(parentId) || new Set();
165+
currentlyResolving.set(parentId, currentlyResolvingForParent);
164166
const requireTargets = await Promise.all(
165167
sources.map(async ({ source, isConditional }) => {
166168
// Never analyze or proxy internal modules
167169
if (source.startsWith('\0')) {
168170
return { id: source, allowProxy: false };
169171
}
172+
currentlyResolvingForParent.add(source);
170173
const resolved =
171174
(await rollupContext.resolve(source, parentId, {
172175
custom: { 'node-resolve': { isRequire: true } }
173176
})) || resolveExtensions(source, parentId, extensions);
177+
currentlyResolvingForParent.delete(source);
174178
if (!resolved) {
175179
return { id: wrapId(source, EXTERNAL_SUFFIX), allowProxy: false };
176180
}
@@ -201,6 +205,10 @@ export function getRequireResolver(extensions, detectCyclesAndConditional) {
201205
isCommonJS
202206
};
203207
});
208+
},
209+
isCurrentlyResolving(source, parentId) {
210+
const currentlyResolvingForParent = currentlyResolving.get(parentId);
211+
return currentlyResolvingForParent && currentlyResolvingForParent.has(source);
204212
}
205213
};
206214
}

‎packages/commonjs/test/fixtures/function/warn-this-resolve-without-options/_config.js

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
const assert = require('assert');
2+
3+
const warnings = [];
4+
5+
module.exports = {
6+
description: 'Warns when another plugin uses this.resolve without forwarding options',
7+
options: {
8+
onwarn(warning) {
9+
warnings.push(warning);
10+
},
11+
plugins: [
12+
{
13+
name: 'test',
14+
resolveId(source, importer) {
15+
return this.resolve(source, importer, { skipSelf: true });
16+
},
17+
buildEnd() {
18+
assert.strictEqual(warnings.length, 1);
19+
assert.strictEqual(
20+
warnings[0].message,
21+
'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.'
22+
);
23+
assert.strictEqual(warnings[0].pluginCode, 'THIS_RESOLVE_WITHOUT_OPTIONS');
24+
assert.strictEqual(warnings[0].url, 'https://rollupjs.org/guide/en/#resolveid');
25+
}
26+
}
27+
]
28+
}
29+
};

‎packages/commonjs/test/fixtures/function/warn-this-resolve-without-options/foo.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
module.exports = 21;

‎packages/commonjs/test/fixtures/function/warn-this-resolve-without-options/main.js

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
const foo = require('./foo');
2+
3+
module.exports = foo * 2;

‎packages/commonjs/test/snapshots/function.js.md

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7713,3 +7713,20 @@ Generated by [AVA](https://avajs.dev).
77137713
module.exports = main;␊
77147714
`,
77157715
}
7716+
7717+
## warn-this-resolve-without-options
7718+
7719+
> Snapshot 1
7720+
7721+
{
7722+
'main.js': `'use strict';␊
7723+
␊
7724+
var foo$1 = 21;␊
7725+
␊
7726+
const foo = foo$1;␊
7727+
␊
7728+
var main = foo * 2;␊
7729+
␊
7730+
module.exports = main;␊
7731+
`,
7732+
}

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

24 Bytes
Binary file not shown.

0 commit comments

Comments
 (0)