From 4f8c7b043bccb0faa2fa7ddde03d250df0b40ab4 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 9 Jul 2025 17:14:15 +0200 Subject: [PATCH 1/2] 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. --- lib/internal/modules/cjs/loader.js | 28 ++++++-- lib/internal/modules/esm/loader.js | 41 +++++------ lib/internal/modules/helpers.js | 19 ++++- .../node_modules/foo/default.cjs | 2 + .../node_modules/foo/foo-esm.mjs | 2 + .../custom-condition/node_modules/foo/foo.cjs | 2 + .../node_modules/foo/package.json | 28 ++++++++ ...test-module-hooks-custom-conditions-cjs.js | 57 +++++++++++++++ ...-hooks-custom-conditions-special-values.js | 70 +++++++++++++++++++ .../test-module-hooks-custom-conditions.mjs | 53 ++++++++++++++ 10 files changed, 269 insertions(+), 33 deletions(-) create mode 100644 test/fixtures/es-modules/custom-condition/node_modules/foo/default.cjs create mode 100644 test/fixtures/es-modules/custom-condition/node_modules/foo/foo-esm.mjs create mode 100644 test/fixtures/es-modules/custom-condition/node_modules/foo/foo.cjs create mode 100644 test/fixtures/es-modules/custom-condition/node_modules/foo/package.json create mode 100644 test/module-hooks/test-module-hooks-custom-conditions-cjs.js create mode 100644 test/module-hooks/test-module-hooks-custom-conditions-special-values.js create mode 100644 test/module-hooks/test-module-hooks-custom-conditions.mjs diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 80d6af2987145f..493059fb0049d7 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -51,6 +51,7 @@ const { ReflectSet, RegExpPrototypeExec, SafeMap, + SafeSet, String, StringPrototypeCharAt, StringPrototypeCharCodeAt, @@ -154,6 +155,7 @@ const internalFsBinding = internalBinding('fs'); const { safeGetenv } = internalBinding('credentials'); const { getCjsConditions, + getCjsConditionsArray, initializeCjsConditions, loadBuiltinModule, makeRequireFunction, @@ -652,7 +654,7 @@ const EXPORTS_PATTERN = /^((?:@[^/\\%]+\/)?[^./\\%][^/\\%]*)(\/.*)?$/; * Resolves the exports for a given module path and request. * @param {string} nmPath The path to the module. * @param {string} request The request for the module. - * @param {unknown} conditions + * @param {Set} conditions The conditions to use for resolution. * @returns {undefined|string} */ function resolveExports(nmPath, request, conditions) { @@ -1067,9 +1069,22 @@ function resolveForCJSWithHooks(specifier, parent, isMain) { function defaultResolve(specifier, context) { // TODO(joyeecheung): parent and isMain should be part of context, then we // no longer need to use a different defaultResolve for every resolution. + // In the hooks, context.conditions is passed around as an array, but internally + // the resolution helpers expect a SafeSet. Do the conversion here. + let conditionSet; + const conditions = context.conditions; + if (conditions !== undefined && conditions !== getCjsConditionsArray()) { + if (!ArrayIsArray(conditions)) { + throw new ERR_INVALID_ARG_VALUE('context.conditions', conditions, + 'expected an array'); + } + conditionSet = new SafeSet(conditions); + } else { + conditionSet = getCjsConditions(); + } defaultResolvedFilename = defaultResolveImpl(specifier, parent, isMain, { __proto__: null, - conditions: context.conditions, + conditions: conditionSet, }); defaultResolvedURL = convertCJSFilenameToURL(defaultResolvedFilename); @@ -1077,7 +1092,7 @@ function resolveForCJSWithHooks(specifier, parent, isMain) { } const resolveResult = resolveWithHooks(specifier, parentURL, /* importAttributes */ undefined, - getCjsConditions(), defaultResolve); + getCjsConditionsArray(), defaultResolve); const { url } = resolveResult; format = resolveResult.format; @@ -1153,7 +1168,7 @@ function loadBuiltinWithHooks(id, url, format) { url ??= `node:${id}`; // TODO(joyeecheung): do we really want to invoke the load hook for the builtins? const loadResult = loadWithHooks(url, format || 'builtin', /* importAttributes */ undefined, - getCjsConditions(), getDefaultLoad(url, id)); + getCjsConditionsArray(), getDefaultLoad(url, id)); if (loadResult.format && loadResult.format !== 'builtin') { return undefined; // Format has been overridden, return undefined for the caller to continue loading. } @@ -1305,7 +1320,7 @@ Module._load = function(request, parent, isMain) { * @param {ResolveFilenameOptions} options Options object * @typedef {object} ResolveFilenameOptions * @property {string[]} paths Paths to search for modules in - * @property {string[]} conditions Conditions used for resolution. + * @param {Set} conditions The conditions to use for resolution. * @returns {void|string} */ Module._resolveFilename = function(request, parent, isMain, options) { @@ -1754,7 +1769,8 @@ function loadSource(mod, filename, formatFromNode) { mod[kURL] = convertCJSFilenameToURL(filename); } - const loadResult = loadWithHooks(mod[kURL], mod[kFormat], /* importAttributes */ undefined, getCjsConditions(), + const loadResult = loadWithHooks(mod[kURL], mod[kFormat], /* importAttributes */ undefined, + getCjsConditionsArray(), getDefaultLoad(mod[kURL], filename)); // Reset the module properties with load hook results. diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 19904ec4b360e5..13734987231cd6 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -709,24 +709,30 @@ class ModuleLoader { if (this.#customizations) { // Only has module.register hooks. return this.#customizations.resolve(specifier, parentURL, importAttributes); } - return this.#cachedDefaultResolve(specifier, parentURL, importAttributes); + return this.#cachedDefaultResolve(specifier, { + __proto__: null, + conditions: this.#defaultConditions, + parentURL, + importAttributes, + }); } /** * Either return a cached resolution, or perform the default resolution which is synchronous, and * cache the result. * @param {string} specifier See {@link resolve}. - * @param {string} [parentURL] See {@link resolve}. - * @param {ImportAttributes} importAttributes See {@link resolve}. + * @param {{ parentURL?: string, importAttributes: ImportAttributes, conditions?: string[]}} context * @returns {{ format: string, url: string }} */ - #cachedDefaultResolve(specifier, parentURL, importAttributes) { + #cachedDefaultResolve(specifier, context) { + const { parentURL, importAttributes } = context; const requestKey = this.#resolveCache.serializeKey(specifier, importAttributes); const cachedResult = this.#resolveCache.get(requestKey, parentURL); if (cachedResult != null) { return cachedResult; } - const result = this.defaultResolve(specifier, parentURL, importAttributes); + defaultResolve ??= require('internal/modules/esm/resolve').defaultResolve; + const result = defaultResolve(specifier, context); this.#resolveCache.set(requestKey, parentURL, result); return result; } @@ -754,14 +760,15 @@ class ModuleLoader { * This is the default resolve step for module.registerHooks(), which incorporates asynchronous hooks * from module.register() which are run in a blocking fashion for it to be synchronous. * @param {string|URL} specifier See {@link resolveSync}. - * @param {{ parentURL?: string, importAttributes: ImportAttributes}} context See {@link resolveSync}. + * @param {{ parentURL?: string, importAttributes: ImportAttributes, conditions?: string[]}} context + * See {@link resolveSync}. * @returns {{ format: string, url: string }} */ #resolveAndMaybeBlockOnLoaderThread(specifier, context) { if (this.#customizations) { return this.#customizations.resolveSync(specifier, context.parentURL, context.importAttributes); } - return this.#cachedDefaultResolve(specifier, context.parentURL, context.importAttributes); + return this.#cachedDefaultResolve(specifier, context); } /** @@ -784,26 +791,12 @@ class ModuleLoader { return resolveWithHooks(specifier, parentURL, importAttributes, this.#defaultConditions, this.#resolveAndMaybeBlockOnLoaderThread.bind(this)); } - return this.#resolveAndMaybeBlockOnLoaderThread(specifier, { parentURL, importAttributes }); - } - - /** - * Our `defaultResolve` is synchronous and can be used in both - * `resolve` and `resolveSync`. This function is here just to avoid - * repeating the same code block twice in those functions. - * @returns {string} - */ - defaultResolve(originalSpecifier, parentURL, importAttributes) { - defaultResolve ??= require('internal/modules/esm/resolve').defaultResolve; - - const context = { + return this.#resolveAndMaybeBlockOnLoaderThread(specifier, { __proto__: null, conditions: this.#defaultConditions, - importAttributes, parentURL, - }; - - return defaultResolve(originalSpecifier, context); + importAttributes, + }); } /** diff --git a/lib/internal/modules/helpers.js b/lib/internal/modules/helpers.js index 17a1c3d1f01ef1..9a47dc92d9ce9d 100644 --- a/lib/internal/modules/helpers.js +++ b/lib/internal/modules/helpers.js @@ -66,6 +66,9 @@ function toRealPath(requestPath) { /** @type {Set} */ let cjsConditions; +/** @type {string[]} */ +let cjsConditionsArray; + /** * Define the conditions that apply to the CommonJS loader. * @returns {void} @@ -75,15 +78,17 @@ function initializeCjsConditions() { const noAddons = getOptionValue('--no-addons'); const addonConditions = noAddons ? [] : ['node-addons']; // TODO: Use this set when resolving pkg#exports conditions in loader.js. - cjsConditions = new SafeSet([ + cjsConditionsArray = [ 'require', 'node', ...addonConditions, ...userConditions, - ]); + ]; if (getOptionValue('--experimental-require-module')) { - cjsConditions.add('module-sync'); + cjsConditionsArray.push('module-sync'); } + ObjectFreeze(cjsConditionsArray); + cjsConditions = new SafeSet(cjsConditionsArray); } /** @@ -97,6 +102,13 @@ function getCjsConditions() { return cjsConditions; } +function getCjsConditionsArray() { + if (cjsConditionsArray === undefined) { + initializeCjsConditions(); + } + return cjsConditionsArray; +} + /** * Provide one of Node.js' public modules to user code. * @param {string} id - The identifier/specifier of the builtin module to load @@ -418,6 +430,7 @@ module.exports = { flushCompileCache, getBuiltinModule, getCjsConditions, + getCjsConditionsArray, getCompileCacheDir, initializeCjsConditions, loadBuiltinModule, diff --git a/test/fixtures/es-modules/custom-condition/node_modules/foo/default.cjs b/test/fixtures/es-modules/custom-condition/node_modules/foo/default.cjs new file mode 100644 index 00000000000000..fd595339e01723 --- /dev/null +++ b/test/fixtures/es-modules/custom-condition/node_modules/foo/default.cjs @@ -0,0 +1,2 @@ +exports.result = 'default'; + diff --git a/test/fixtures/es-modules/custom-condition/node_modules/foo/foo-esm.mjs b/test/fixtures/es-modules/custom-condition/node_modules/foo/foo-esm.mjs new file mode 100644 index 00000000000000..87499b2e3f3e16 --- /dev/null +++ b/test/fixtures/es-modules/custom-condition/node_modules/foo/foo-esm.mjs @@ -0,0 +1,2 @@ +export const result = 'foo-esm'; + diff --git a/test/fixtures/es-modules/custom-condition/node_modules/foo/foo.cjs b/test/fixtures/es-modules/custom-condition/node_modules/foo/foo.cjs new file mode 100644 index 00000000000000..a948034aa47ae6 --- /dev/null +++ b/test/fixtures/es-modules/custom-condition/node_modules/foo/foo.cjs @@ -0,0 +1,2 @@ +exports.result = 'foo'; + diff --git a/test/fixtures/es-modules/custom-condition/node_modules/foo/package.json b/test/fixtures/es-modules/custom-condition/node_modules/foo/package.json new file mode 100644 index 00000000000000..ee7eab9fc74963 --- /dev/null +++ b/test/fixtures/es-modules/custom-condition/node_modules/foo/package.json @@ -0,0 +1,28 @@ +{ + "exports": { + ".": { + "foo": "./foo.cjs", + "foo-esm": "./foo-esm.mjs", + "default": "./default.cjs" + }, + "./second": { + "foo": "./foo.cjs", + "foo-esm": "./foo-esm.mjs", + "default": "./default.cjs" + }, + "./third": { + "foo": "./foo.cjs", + "foo-esm": "./foo-esm.mjs", + "default": "./default.cjs" + }, + "./fourth": { + "foo": "./foo.cjs", + "foo-esm": "./foo-esm.mjs", + "default": "./default.cjs" + }, + "./no-default": { + "foo": "./foo.cjs", + "foo-esm": "./foo-esm.mjs" + } + } +} \ No newline at end of file diff --git a/test/module-hooks/test-module-hooks-custom-conditions-cjs.js b/test/module-hooks/test-module-hooks-custom-conditions-cjs.js new file mode 100644 index 00000000000000..6f7c746a89c666 --- /dev/null +++ b/test/module-hooks/test-module-hooks-custom-conditions-cjs.js @@ -0,0 +1,57 @@ +// Similar to test-module-hooks-custom-conditions.mjs, but checking the +// real require() instead of the re-invented one for imported CJS. +'use strict'; +const common = require('../common'); +const { registerHooks } = require('node:module'); +const assert = require('node:assert'); +const { cjs, esm } = require('../fixtures/es-modules/custom-condition/load.cjs'); + +(async () => { + // Without hooks, the default condition is used. + assert.strictEqual(cjs('foo').result, 'default'); + assert.strictEqual((await esm('foo')).result, 'default'); + + // Prepending 'foo' to the conditions array in the resolve hook should + // allow a CJS to be resolved with that condition. + { + const hooks = registerHooks({ + resolve(specifier, context, nextResolve) { + assert(Array.isArray(context.conditions)); + context.conditions = ['foo', ...context.conditions]; + return nextResolve(specifier, context); + }, + }); + assert.strictEqual(cjs('foo/second').result, 'foo'); + assert.strictEqual((await esm('foo/second')).result, 'foo'); + hooks.deregister(); + } + + // Prepending 'foo-esm' to the conditions array in the resolve hook should + // allow a ESM to be resolved with that condition. + { + const hooks = registerHooks({ + resolve(specifier, context, nextResolve) { + assert(Array.isArray(context.conditions)); + context.conditions = ['foo-esm', ...context.conditions]; + return nextResolve(specifier, context); + }, + }); + assert.strictEqual(cjs('foo/third').result, 'foo-esm'); + assert.strictEqual((await esm('foo/third')).result, 'foo-esm'); + hooks.deregister(); + } + + // Duplicating the 'foo' condition in the resolve hook should not change the result. + { + const hooks = registerHooks({ + resolve(specifier, context, nextResolve) { + assert(Array.isArray(context.conditions)); + context.conditions = ['foo', ...context.conditions, 'foo']; + return nextResolve(specifier, context); + }, + }); + assert.strictEqual(cjs('foo/fourth').result, 'foo'); + assert.strictEqual((await esm('foo/fourth')).result, 'foo'); + hooks.deregister(); + } +})().catch(common.mustNotCall()); diff --git a/test/module-hooks/test-module-hooks-custom-conditions-special-values.js b/test/module-hooks/test-module-hooks-custom-conditions-special-values.js new file mode 100644 index 00000000000000..e88b1b2740b193 --- /dev/null +++ b/test/module-hooks/test-module-hooks-custom-conditions-special-values.js @@ -0,0 +1,70 @@ +// Check various special values of `conditions` in the context object +// when using synchronous module hooks to override the loaders in a +// CJS module. +'use strict'; +const common = require('../common'); +const { registerHooks } = require('node:module'); +const assert = require('node:assert'); +const { cjs, esm } = require('../fixtures/es-modules/custom-condition/load.cjs'); + +(async () => { + // Setting it to undefined would lead to the default conditions being used. + { + const hooks = registerHooks({ + resolve(specifier, context, nextResolve) { + context.conditions = undefined; + return nextResolve(specifier, context); + }, + }); + assert.strictEqual(cjs('foo').result, 'default'); + assert.strictEqual((await esm('foo')).result, 'default'); + hooks.deregister(); + } + + // Setting it to an empty array would lead to the default conditions being used. + { + const hooks = registerHooks({ + resolve(specifier, context, nextResolve) { + context.conditions = []; + return nextResolve(specifier, context); + }, + }); + assert.strictEqual(cjs('foo/second').result, 'default'); + assert.strictEqual((await esm('foo/second')).result, 'default'); + hooks.deregister(); + } + + // If the exports have no default export, it should error. + { + const hooks = registerHooks({ + resolve(specifier, context, nextResolve) { + context.conditions = []; + return nextResolve(specifier, context); + }, + }); + assert.throws(() => cjs('foo/no-default'), { + code: 'ERR_PACKAGE_PATH_NOT_EXPORTED', + }); + await assert.rejects(esm('foo/no-default'), { + code: 'ERR_PACKAGE_PATH_NOT_EXPORTED', + }); + hooks.deregister(); + } + + // If the exports have no default export, it should error. + { + const hooks = registerHooks({ + resolve(specifier, context, nextResolve) { + context.conditions = 'invalid'; + return nextResolve(specifier, context); + }, + }); + assert.throws(() => cjs('foo/third'), { + code: 'ERR_INVALID_ARG_VALUE', + }); + await assert.rejects(esm('foo/third'), { + code: 'ERR_INVALID_ARG_VALUE', + }); + hooks.deregister(); + } +})().catch(common.mustNotCall()); diff --git a/test/module-hooks/test-module-hooks-custom-conditions.mjs b/test/module-hooks/test-module-hooks-custom-conditions.mjs new file mode 100644 index 00000000000000..ef2022cf190902 --- /dev/null +++ b/test/module-hooks/test-module-hooks-custom-conditions.mjs @@ -0,0 +1,53 @@ +// This tests that custom conditions can be used in module resolution hooks. +import '../common/index.mjs'; +import { registerHooks } from 'node:module'; +import assert from 'node:assert'; +import { cjs, esm } from '../fixtures/es-modules/custom-condition/load.cjs'; + +// Without hooks, the default condition is used. +assert.strictEqual(cjs('foo').result, 'default'); +assert.strictEqual((await esm('foo')).result, 'default'); + +// Prepending 'foo' to the conditions array in the resolve hook should +// allow a CJS to be resolved with that condition. +{ + const hooks = registerHooks({ + resolve(specifier, context, nextResolve) { + assert(Array.isArray(context.conditions)); + context.conditions = ['foo', ...context.conditions]; + return nextResolve(specifier, context); + }, + }); + assert.strictEqual(cjs('foo/second').result, 'foo'); + assert.strictEqual((await esm('foo/second')).result, 'foo'); + hooks.deregister(); +} + +// Prepending 'foo-esm' to the conditions array in the resolve hook should +// allow a ESM to be resolved with that condition. +{ + const hooks = registerHooks({ + resolve(specifier, context, nextResolve) { + assert(Array.isArray(context.conditions)); + context.conditions = ['foo-esm', ...context.conditions]; + return nextResolve(specifier, context); + }, + }); + assert.strictEqual(cjs('foo/third').result, 'foo-esm'); + assert.strictEqual((await esm('foo/third')).result, 'foo-esm'); + hooks.deregister(); +} + +// Duplicating the 'foo' condition in the resolve hook should not change the result. +{ + const hooks = registerHooks({ + resolve(specifier, context, nextResolve) { + assert(Array.isArray(context.conditions)); + context.conditions = ['foo', ...context.conditions, 'foo']; + return nextResolve(specifier, context); + }, + }); + assert.strictEqual(cjs('foo/fourth').result, 'foo'); + assert.strictEqual((await esm('foo/fourth')).result, 'foo'); + hooks.deregister(); +} From 99865a7e271958fc519c49db5d0c850fbdddeb33 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 22 Jul 2025 14:19:32 +0200 Subject: [PATCH 2/2] fixup! module: fix conditions override in synchronous resolve hooks --- lib/internal/modules/cjs/loader.js | 2 +- lib/internal/modules/esm/loader.js | 2 +- test/module-hooks/test-module-hooks-custom-conditions-cjs.js | 2 +- .../test-module-hooks-custom-conditions-special-values.js | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 493059fb0049d7..3b16099848a3a9 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -1320,7 +1320,7 @@ Module._load = function(request, parent, isMain) { * @param {ResolveFilenameOptions} options Options object * @typedef {object} ResolveFilenameOptions * @property {string[]} paths Paths to search for modules in - * @param {Set} conditions The conditions to use for resolution. + * @property {Set?} conditions The conditions to use for resolution. * @returns {void|string} */ Module._resolveFilename = function(request, parent, isMain, options) { diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 13734987231cd6..bc36fa2e26b8d0 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -761,7 +761,7 @@ class ModuleLoader { * from module.register() which are run in a blocking fashion for it to be synchronous. * @param {string|URL} specifier See {@link resolveSync}. * @param {{ parentURL?: string, importAttributes: ImportAttributes, conditions?: string[]}} context - * See {@link resolveSync}. + * See {@link resolveSync}. * @returns {{ format: string, url: string }} */ #resolveAndMaybeBlockOnLoaderThread(specifier, context) { diff --git a/test/module-hooks/test-module-hooks-custom-conditions-cjs.js b/test/module-hooks/test-module-hooks-custom-conditions-cjs.js index 6f7c746a89c666..4dcb9c3a20f19e 100644 --- a/test/module-hooks/test-module-hooks-custom-conditions-cjs.js +++ b/test/module-hooks/test-module-hooks-custom-conditions-cjs.js @@ -54,4 +54,4 @@ const { cjs, esm } = require('../fixtures/es-modules/custom-condition/load.cjs') assert.strictEqual((await esm('foo/fourth')).result, 'foo'); hooks.deregister(); } -})().catch(common.mustNotCall()); +})().then(common.mustCall()); diff --git a/test/module-hooks/test-module-hooks-custom-conditions-special-values.js b/test/module-hooks/test-module-hooks-custom-conditions-special-values.js index e88b1b2740b193..9a84c788c77d05 100644 --- a/test/module-hooks/test-module-hooks-custom-conditions-special-values.js +++ b/test/module-hooks/test-module-hooks-custom-conditions-special-values.js @@ -67,4 +67,4 @@ const { cjs, esm } = require('../fixtures/es-modules/custom-condition/load.cjs') }); hooks.deregister(); } -})().catch(common.mustNotCall()); +})().then(common.mustCall());