FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

module: fix conditions override in synchronous resolve hooks · nodejs/node@fe0195f · GitHub

/ node Public

Commit fe0195f

Browse files
joyeecheung
authored and
aduh95
committed
module: fix conditions override in synchronous resolve hooks
1. Make sure that the conditions are converted into arrays when being passed into user hooks. 2. Pass the conditions from user hooks into the ESM resolution so that it takes effect. PR-URL: #59011 Fixes: #59003 Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com> Reviewed-By: Jacob Smith <jacob@frende.me>
1 parent 55a90ee commit fe0195f

10 files changed

Lines changed: 269 additions & 33 deletions

File tree

‎lib/internal/modules/cjs/loader.js‎

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ const {
5151
ReflectSet,
5252
RegExpPrototypeExec,
5353
SafeMap,
54+
SafeSet,
5455
String,
5556
StringPrototypeCharAt,
5657
StringPrototypeCharCodeAt,
@@ -154,6 +155,7 @@ const internalFsBinding = internalBinding('fs');
154155
const { safeGetenv } = internalBinding('credentials');
155156
const {
156157
getCjsConditions,
158+
getCjsConditionsArray,
157159
initializeCjsConditions,
158160
loadBuiltinModule,
159161
makeRequireFunction,
@@ -653,7 +655,7 @@ const EXPORTS_PATTERN = /^((?:@[^/\\%]+\/)?[^./\\%][^/\\%]*)(\/.*)?$/;
653655
* Resolves the exports for a given module path and request.
654656
* @param {string} nmPath The path to the module.
655657
* @param {string} request The request for the module.
656-
* @param {unknown} conditions
658+
* @param {Set<string>} conditions The conditions to use for resolution.
657659
* @returns {undefined|string}
658660
*/
659661
function resolveExports(nmPath, request, conditions) {
@@ -1068,17 +1070,30 @@ function resolveForCJSWithHooks(specifier, parent, isMain) {
10681070
function defaultResolve(specifier, context) {
10691071
// TODO(joyeecheung): parent and isMain should be part of context, then we
10701072
// no longer need to use a different defaultResolve for every resolution.
1073+
// In the hooks, context.conditions is passed around as an array, but internally
1074+
// the resolution helpers expect a SafeSet. Do the conversion here.
1075+
let conditionSet;
1076+
const conditions = context.conditions;
1077+
if (conditions !== undefined && conditions !== getCjsConditionsArray()) {
1078+
if (!ArrayIsArray(conditions)) {
1079+
throw new ERR_INVALID_ARG_VALUE('context.conditions', conditions,
1080+
'expected an array');
1081+
}
1082+
conditionSet = new SafeSet(conditions);
1083+
} else {
1084+
conditionSet = getCjsConditions();
1085+
}
10711086
defaultResolvedFilename = defaultResolveImpl(specifier, parent, isMain, {
10721087
__proto__: null,
1073-
conditions: context.conditions,
1088+
conditions: conditionSet,
10741089
});
10751090

10761091
defaultResolvedURL = convertCJSFilenameToURL(defaultResolvedFilename);
10771092
return { __proto__: null, url: defaultResolvedURL };
10781093
}
10791094

10801095
const resolveResult = resolveWithHooks(specifier, parentURL, /* importAttributes */ undefined,
1081-
getCjsConditions(), defaultResolve);
1096+
getCjsConditionsArray(), defaultResolve);
10821097
const { url } = resolveResult;
10831098
format = resolveResult.format;
10841099

@@ -1154,7 +1169,7 @@ function loadBuiltinWithHooks(id, url, format) {
11541169
url ??= `node:${id}`;
11551170
// TODO(joyeecheung): do we really want to invoke the load hook for the builtins?
11561171
const loadResult = loadWithHooks(url, format || 'builtin', /* importAttributes */ undefined,
1157-
getCjsConditions(), getDefaultLoad(url, id));
1172+
getCjsConditionsArray(), getDefaultLoad(url, id));
11581173
if (loadResult.format && loadResult.format !== 'builtin') {
11591174
return undefined; // Format has been overridden, return undefined for the caller to continue loading.
11601175
}
@@ -1306,7 +1321,7 @@ Module._load = function(request, parent, isMain) {
13061321
* @param {ResolveFilenameOptions} options Options object
13071322
* @typedef {object} ResolveFilenameOptions
13081323
* @property {string[]} paths Paths to search for modules in
1309-
* @property {string[]} conditions Conditions used for resolution.
1324+
* @property {Set<string>?} conditions The conditions to use for resolution.
13101325
* @returns {void|string}
13111326
*/
13121327
Module._resolveFilename = function(request, parent, isMain, options) {
@@ -1755,7 +1770,8 @@ function loadSource(mod, filename, formatFromNode) {
17551770
mod[kURL] = convertCJSFilenameToURL(filename);
17561771
}
17571772

1758-
const loadResult = loadWithHooks(mod[kURL], mod[kFormat], /* importAttributes */ undefined, getCjsConditions(),
1773+
const loadResult = loadWithHooks(mod[kURL], mod[kFormat], /* importAttributes */ undefined,
1774+
getCjsConditionsArray(),
17591775
getDefaultLoad(mod[kURL], filename));
17601776

17611777
// Reset the module properties with load hook results.

‎lib/internal/modules/esm/loader.js‎

Lines changed: 17 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -709,24 +709,30 @@ class ModuleLoader {
709709
if (this.#customizations) { // Only has module.register hooks.
710710
return this.#customizations.resolve(specifier, parentURL, importAttributes);
711711
}
712-
return this.#cachedDefaultResolve(specifier, parentURL, importAttributes);
712+
return this.#cachedDefaultResolve(specifier, {
713+
__proto__: null,
714+
conditions: this.#defaultConditions,
715+
parentURL,
716+
importAttributes,
717+
});
713718
}
714719

715720
/**
716721
* Either return a cached resolution, or perform the default resolution which is synchronous, and
717722
* cache the result.
718723
* @param {string} specifier See {@link resolve}.
719-
* @param {string} [parentURL] See {@link resolve}.
720-
* @param {ImportAttributes} importAttributes See {@link resolve}.
724+
* @param {{ parentURL?: string, importAttributes: ImportAttributes, conditions?: string[]}} context
721725
* @returns {{ format: string, url: string }}
722726
*/
723-
#cachedDefaultResolve(specifier, parentURL, importAttributes) {
727+
#cachedDefaultResolve(specifier, context) {
728+
const { parentURL, importAttributes } = context;
724729
const requestKey = this.#resolveCache.serializeKey(specifier, importAttributes);
725730
const cachedResult = this.#resolveCache.get(requestKey, parentURL);
726731
if (cachedResult != null) {
727732
return cachedResult;
728733
}
729-
const result = this.defaultResolve(specifier, parentURL, importAttributes);
734+
defaultResolve ??= require('internal/modules/esm/resolve').defaultResolve;
735+
const result = defaultResolve(specifier, context);
730736
this.#resolveCache.set(requestKey, parentURL, result);
731737
return result;
732738
}
@@ -754,14 +760,15 @@ class ModuleLoader {
754760
* This is the default resolve step for module.registerHooks(), which incorporates asynchronous hooks
755761
* from module.register() which are run in a blocking fashion for it to be synchronous.
756762
* @param {string|URL} specifier See {@link resolveSync}.
757-
* @param {{ parentURL?: string, importAttributes: ImportAttributes}} context See {@link resolveSync}.
763+
* @param {{ parentURL?: string, importAttributes: ImportAttributes, conditions?: string[]}} context
764+
* See {@link resolveSync}.
758765
* @returns {{ format: string, url: string }}
759766
*/
760767
#resolveAndMaybeBlockOnLoaderThread(specifier, context) {
761768
if (this.#customizations) {
762769
return this.#customizations.resolveSync(specifier, context.parentURL, context.importAttributes);
763770
}
764-
return this.#cachedDefaultResolve(specifier, context.parentURL, context.importAttributes);
771+
return this.#cachedDefaultResolve(specifier, context);
765772
}
766773

767774
/**
@@ -784,26 +791,12 @@ class ModuleLoader {
784791
return resolveWithHooks(specifier, parentURL, importAttributes, this.#defaultConditions,
785792
this.#resolveAndMaybeBlockOnLoaderThread.bind(this));
786793
}
787-
return this.#resolveAndMaybeBlockOnLoaderThread(specifier, { parentURL, importAttributes });
788-
}
789-
790-
/**
791-
* Our `defaultResolve` is synchronous and can be used in both
792-
* `resolve` and `resolveSync`. This function is here just to avoid
793-
* repeating the same code block twice in those functions.
794-
* @returns {string}
795-
*/
796-
defaultResolve(originalSpecifier, parentURL, importAttributes) {
797-
defaultResolve ??= require('internal/modules/esm/resolve').defaultResolve;
798-
799-
const context = {
794+
return this.#resolveAndMaybeBlockOnLoaderThread(specifier, {
800795
__proto__: null,
801796
conditions: this.#defaultConditions,
802-
importAttributes,
803797
parentURL,
804-
};
805-
806-
return defaultResolve(originalSpecifier, context);
798+
importAttributes,
799+
});
807800
}
808801

809802
/**

‎lib/internal/modules/helpers.js‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,9 @@ function toRealPath(requestPath) {
6666

6767
/** @type {Set<string>} */
6868
let cjsConditions;
69+
/** @type {string[]} */
70+
let cjsConditionsArray;
71+
6972
/**
7073
* Define the conditions that apply to the CommonJS loader.
7174
* @returns {void}
@@ -75,15 +78,17 @@ function initializeCjsConditions() {
7578
const noAddons = getOptionValue('--no-addons');
7679
const addonConditions = noAddons ? [] : ['node-addons'];
7780
// TODO: Use this set when resolving pkg#exports conditions in loader.js.
78-
cjsConditions = new SafeSet([
81+
cjsConditionsArray = [
7982
'require',
8083
'node',
8184
...addonConditions,
8285
...userConditions,
83-
]);
86+
];
8487
if (getOptionValue('--experimental-require-module')) {
85-
cjsConditions.add('module-sync');
88+
cjsConditionsArray.push('module-sync');
8689
}
90+
ObjectFreeze(cjsConditionsArray);
91+
cjsConditions = new SafeSet(cjsConditionsArray);
8792
}
8893

8994
/**
@@ -97,6 +102,13 @@ function getCjsConditions() {
97102
return cjsConditions;
98103
}
99104

105+
function getCjsConditionsArray() {
106+
if (cjsConditionsArray === undefined) {
107+
initializeCjsConditions();
108+
}
109+
return cjsConditionsArray;
110+
}
111+
100112
/**
101113
* Provide one of Node.js' public modules to user code.
102114
* @param {string} id - The identifier/specifier of the builtin module to load
@@ -418,6 +430,7 @@ module.exports = {
418430
flushCompileCache,
419431
getBuiltinModule,
420432
getCjsConditions,
433+
getCjsConditionsArray,
421434
getCompileCacheDir,
422435
initializeCjsConditions,
423436
loadBuiltinModule,

‎test/fixtures/es-modules/custom-condition/node_modules/foo/default.cjs‎

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎test/fixtures/es-modules/custom-condition/node_modules/foo/foo-esm.mjs‎

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎test/fixtures/es-modules/custom-condition/node_modules/foo/foo.cjs‎

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎test/fixtures/es-modules/custom-condition/node_modules/foo/package.json‎

Lines changed: 28 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎test/module-hooks/test-module-hooks-custom-conditions-cjs.js‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
// Similar to test-module-hooks-custom-conditions.mjs, but checking the
2+
// real require() instead of the re-invented one for imported CJS.
3+
'use strict';
4+
const common = require('../common');
5+
const { registerHooks } = require('node:module');
6+
const assert = require('node:assert');
7+
const { cjs, esm } = require('../fixtures/es-modules/custom-condition/load.cjs');
8+
9+
(async () => {
10+
// Without hooks, the default condition is used.
11+
assert.strictEqual(cjs('foo').result, 'default');
12+
assert.strictEqual((await esm('foo')).result, 'default');
13+
14+
// Prepending 'foo' to the conditions array in the resolve hook should
15+
// allow a CJS to be resolved with that condition.
16+
{
17+
const hooks = registerHooks({
18+
resolve(specifier, context, nextResolve) {
19+
assert(Array.isArray(context.conditions));
20+
context.conditions = ['foo', ...context.conditions];
21+
return nextResolve(specifier, context);
22+
},
23+
});
24+
assert.strictEqual(cjs('foo/second').result, 'foo');
25+
assert.strictEqual((await esm('foo/second')).result, 'foo');
26+
hooks.deregister();
27+
}
28+
29+
// Prepending 'foo-esm' to the conditions array in the resolve hook should
30+
// allow a ESM to be resolved with that condition.
31+
{
32+
const hooks = registerHooks({
33+
resolve(specifier, context, nextResolve) {
34+
assert(Array.isArray(context.conditions));
35+
context.conditions = ['foo-esm', ...context.conditions];
36+
return nextResolve(specifier, context);
37+
},
38+
});
39+
assert.strictEqual(cjs('foo/third').result, 'foo-esm');
40+
assert.strictEqual((await esm('foo/third')).result, 'foo-esm');
41+
hooks.deregister();
42+
}
43+
44+
// Duplicating the 'foo' condition in the resolve hook should not change the result.
45+
{
46+
const hooks = registerHooks({
47+
resolve(specifier, context, nextResolve) {
48+
assert(Array.isArray(context.conditions));
49+
context.conditions = ['foo', ...context.conditions, 'foo'];
50+
return nextResolve(specifier, context);
51+
},
52+
});
53+
assert.strictEqual(cjs('foo/fourth').result, 'foo');
54+
assert.strictEqual((await esm('foo/fourth')).result, 'foo');
55+
hooks.deregister();
56+
}
57+
})().then(common.mustCall());

‎test/module-hooks/test-module-hooks-custom-conditions-special-values.js‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
// Check various special values of `conditions` in the context object
2+
// when using synchronous module hooks to override the loaders in a
3+
// CJS module.
4+
'use strict';
5+
const common = require('../common');
6+
const { registerHooks } = require('node:module');
7+
const assert = require('node:assert');
8+
const { cjs, esm } = require('../fixtures/es-modules/custom-condition/load.cjs');
9+
10+
(async () => {
11+
// Setting it to undefined would lead to the default conditions being used.
12+
{
13+
const hooks = registerHooks({
14+
resolve(specifier, context, nextResolve) {
15+
context.conditions = undefined;
16+
return nextResolve(specifier, context);
17+
},
18+
});
19+
assert.strictEqual(cjs('foo').result, 'default');
20+
assert.strictEqual((await esm('foo')).result, 'default');
21+
hooks.deregister();
22+
}
23+
24+
// Setting it to an empty array would lead to the default conditions being used.
25+
{
26+
const hooks = registerHooks({
27+
resolve(specifier, context, nextResolve) {
28+
context.conditions = [];
29+
return nextResolve(specifier, context);
30+
},
31+
});
32+
assert.strictEqual(cjs('foo/second').result, 'default');
33+
assert.strictEqual((await esm('foo/second')).result, 'default');
34+
hooks.deregister();
35+
}
36+
37+
// If the exports have no default export, it should error.
38+
{
39+
const hooks = registerHooks({
40+
resolve(specifier, context, nextResolve) {
41+
context.conditions = [];
42+
return nextResolve(specifier, context);
43+
},
44+
});
45+
assert.throws(() => cjs('foo/no-default'), {
46+
code: 'ERR_PACKAGE_PATH_NOT_EXPORTED',
47+
});
48+
await assert.rejects(esm('foo/no-default'), {
49+
code: 'ERR_PACKAGE_PATH_NOT_EXPORTED',
50+
});
51+
hooks.deregister();
52+
}
53+
54+
// If the exports have no default export, it should error.
55+
{
56+
const hooks = registerHooks({
57+
resolve(specifier, context, nextResolve) {
58+
context.conditions = 'invalid';
59+
return nextResolve(specifier, context);
60+
},
61+
});
62+
assert.throws(() => cjs('foo/third'), {
63+
code: 'ERR_INVALID_ARG_VALUE',
64+
});
65+
await assert.rejects(esm('foo/third'), {
66+
code: 'ERR_INVALID_ARG_VALUE',
67+
});
68+
hooks.deregister();
69+
}
70+
})().then(common.mustCall());

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL