From 0f4c3dd0943db5bc5c66cb68cee752f35f7a7705 Mon Sep 17 00:00:00 2001 From: Bob Put Date: Tue, 2 Jun 2026 21:56:42 -0400 Subject: [PATCH] module: use CJS conditions for hook-loaded require Pass CommonJS package export conditions through synchronous resolution that blocks on async loader hooks when the request originates from require() in a CommonJS module loaded by module.register(). This keeps async resolve hooks from seeing ESM import conditions for require.resolve(). Fixes: https://github.com/nodejs/node/issues/51327 Signed-off-by: Bob Put --- lib/internal/modules/esm/hooks.js | 35 +++++++++---- lib/internal/modules/esm/loader.js | 26 +++++++--- lib/internal/modules/esm/translators.js | 8 ++- ...nc-loader-hooks-cjs-require-conditions.mjs | 50 +++++++++++++++++++ 4 files changed, 103 insertions(+), 16 deletions(-) create mode 100644 test/module-hooks/test-async-loader-hooks-cjs-require-conditions.mjs diff --git a/lib/internal/modules/esm/hooks.js b/lib/internal/modules/esm/hooks.js index 78ffb5f834a989..582979f71e71a0 100644 --- a/lib/internal/modules/esm/hooks.js +++ b/lib/internal/modules/esm/hooks.js @@ -115,11 +115,11 @@ function defineImportAssertionAlias(context) { * Calling the asynchronous `load` hook asynchronously. * @property {(url: string, context: object, defaultLoad: Function) => LoadResult} [loadSync] * Calling the asynchronous `load` hook synchronously. - * @property {(originalSpecifier: string, parentURL: string, - * importAttributes: Record) => Promise} resolve + * @property {(originalSpecifier: string, parentURL: string, importAttributes: + * Record, conditions?: string[]) => Promise} resolve * Calling the asynchronous `resolve` hook asynchronously. - * @property {(originalSpecifier: string, parentURL: string, - * importAttributes: Record) => ResolveResult} [resolveSync] + * @property {(originalSpecifier: string, parentURL: string, importAttributes: + * Record, conditions?: string[]) => ResolveResult} [resolveSync] * Calling the asynchronous `resolve` hook synchronously. * @property {(specifier: string, parentURL: string) => any} register Register asynchronous loader hooks * @property {() => void} waitForLoaderHookInitialization Force loading of hooks. @@ -219,18 +219,20 @@ class AsyncLoaderHooksOnLoaderHookWorker { * @param {string} [parentURL] The URL path of the module's parent. * @param {ImportAttributes} [importAttributes] Attributes from the import * statement or expression. + * @param {string[]} [conditions] Package export conditions for this request. * @returns {Promise<{ format: string, url: URL['href'] }>} */ async resolve( originalSpecifier, parentURL, importAttributes = { __proto__: null }, + conditions = getDefaultConditions(), ) { throwIfInvalidParentURL(parentURL); const chain = this.#chains.resolve; const context = { - conditions: getDefaultConditions(), + conditions, importAttributes, parentURL, }; @@ -834,15 +836,30 @@ class AsyncLoaderHooksProxiedToLoaderHookWorker { * @param {string} [parentURL] The URL path of the module's parent. * @param {ImportAttributes} importAttributes Attributes from the import * statement or expression. + * @param {string[]} [conditions] Package export conditions for this request. * @returns {{ format: string, url: URL['href'] }} */ - resolve(originalSpecifier, parentURL, importAttributes) { - return asyncLoaderHookWorker.makeAsyncRequest('resolve', undefined, originalSpecifier, parentURL, importAttributes); + resolve(originalSpecifier, parentURL, importAttributes, conditions) { + return asyncLoaderHookWorker.makeAsyncRequest( + 'resolve', + undefined, + originalSpecifier, + parentURL, + importAttributes, + conditions, + ); } - resolveSync(originalSpecifier, parentURL, importAttributes) { + resolveSync(originalSpecifier, parentURL, importAttributes, conditions) { // This happens only as a result of `import.meta.resolve` calls, which must be sync per spec. - return asyncLoaderHookWorker.makeSyncRequest('resolve', undefined, originalSpecifier, parentURL, importAttributes); + return asyncLoaderHookWorker.makeSyncRequest( + 'resolve', + undefined, + originalSpecifier, + parentURL, + importAttributes, + conditions, + ); } /** diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 5099bc44f8a207..02672991aba236 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -61,6 +61,7 @@ const { setImportMetaResolveInitializer, } = internalBinding('module_wrap'); const { + getCjsConditionsArray, urlToFilename, } = require('internal/modules/helpers'); const { @@ -600,7 +601,9 @@ class ModuleLoader { let maybePromise; if (requestType === kRequireInImportedCJS || requestType === kImportInRequiredESM) { // In these two cases, resolution must be synchronous. - maybePromise = this.resolveSync(parentURL, request); + const conditions = requestType === kRequireInImportedCJS ? + getCjsConditionsArray() : this.#defaultConditions; + maybePromise = this.resolveSync(parentURL, request, false, conditions); assert(!isPromise(maybePromise)); } else { maybePromise = this.#resolve(parentURL, request); @@ -680,7 +683,7 @@ class ModuleLoader { const specifier = `${request.specifier}`; const importAttributes = request.attributes ?? kEmptyObject; // TODO(joyeecheung): invoke the synchronous hooks in the default step on loader thread. - return this.#asyncLoaderHooks.resolve(specifier, parentURL, importAttributes); + return this.#asyncLoaderHooks.resolve(specifier, parentURL, importAttributes, request.conditions); } return this.resolveSync(parentURL, request); @@ -718,7 +721,12 @@ class ModuleLoader { */ #resolveAndMaybeBlockOnLoaderThread(out, specifier, context) { if (this.#asyncLoaderHooks?.resolveSync) { - return this.#asyncLoaderHooks.resolveSync(specifier, context.parentURL, context.importAttributes); + return this.#asyncLoaderHooks.resolveSync( + specifier, + context.parentURL, + context.importAttributes, + context.conditions, + ); } out.isResolvedByDefaultResolve = true; return this.#cachedDefaultResolve(specifier, context); @@ -735,9 +743,15 @@ class ModuleLoader { * @param {boolean} [shouldSkipSyncHooks] Whether to skip the synchronous hooks registered by module.registerHooks(). * This is used to maintain compatibility for the re-invented require.resolve (in imported CJS customized * by module.register()`) which invokes the CJS resolution separately from the hook chain. + * @param {string[]} [conditions] Package export conditions for this request. * @returns {ResolveResult} */ - resolveSync(parentURL, request, shouldSkipSyncHooks = false) { + resolveSync( + parentURL, + request, + shouldSkipSyncHooks = false, + conditions = request.conditions ?? this.#defaultConditions, + ) { const specifier = `${request.specifier}`; const importAttributes = request.attributes ?? kEmptyObject; // Use an output parameter to track the state and avoid polluting the user-visible resolve results. @@ -747,14 +761,14 @@ class ModuleLoader { let isResolvedBySyncHooks = false; if (!shouldSkipSyncHooks && syncResolveHooks.length) { // Has module.registerHooks() hooks, chain the asynchronous hooks in the default step. - result = resolveWithSyncHooks(specifier, parentURL, importAttributes, this.#defaultConditions, + result = resolveWithSyncHooks(specifier, parentURL, importAttributes, conditions, this.#resolveAndMaybeBlockOnLoaderThread.bind(this, out)); // If the default step ran, sync hooks did not short-circuit the resolution. isResolvedBySyncHooks = !out.isResolvedByDefaultResolve; } else { const context = { ...request, - conditions: this.#defaultConditions, + conditions, parentURL, importAttributes, __proto__: null, diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index c453e2b54f5957..3724a125aa7a80 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -25,6 +25,7 @@ const assert = require('internal/assert'); const { dirname, extname } = require('path'); const { assertBufferSource, + getCjsConditionsArray, loadBuiltinModule, stringify, stripBOM, @@ -178,7 +179,12 @@ function loadCJSModuleWithSpecialRequire(module, source, url, filename, isMain, } } - const request = { specifier, __proto__: null, attributes: kEmptyObject }; + const request = { + specifier, + __proto__: null, + attributes: kEmptyObject, + conditions: getCjsConditionsArray(), + }; // Skip sync hooks in resolveSync since resolveForCJSWithHooks already ran them above. const { url: resolvedURL } = cascadedLoader.resolveSync(url, request, /* shouldSkipSyncHooks */ true); return urlToFilename(resolvedURL); diff --git a/test/module-hooks/test-async-loader-hooks-cjs-require-conditions.mjs b/test/module-hooks/test-async-loader-hooks-cjs-require-conditions.mjs new file mode 100644 index 00000000000000..bf334094925fb5 --- /dev/null +++ b/test/module-hooks/test-async-loader-hooks-cjs-require-conditions.mjs @@ -0,0 +1,50 @@ +import '../common/index.mjs'; +import assert from 'node:assert'; +import { register } from 'node:module'; + +const loader = ` + import assert from 'node:assert'; + + function assertCjsConditions(context) { + assert.ok( + context.conditions.includes('require'), + \`Conditions should include "require": \${JSON.stringify(context.conditions)}\`, + ); + assert.ok( + !context.conditions.includes('import'), + \`Conditions should not include "import": \${JSON.stringify(context.conditions)}\`, + ); + } + + export async function resolve(specifier, context, nextResolve) { + if (specifier === 'custom:cjs') { + return { url: 'custom:cjs', format: 'commonjs', shortCircuit: true }; + } + + if (specifier === 'node:target') { + assertCjsConditions(context); + return { url: 'node:fs', shortCircuit: true }; + } + + return nextResolve(specifier, context); + } + + export async function load(url, context, nextLoad) { + if (url === 'custom:cjs') { + return { + format: 'commonjs', + source: \` + module.exports = require.resolve('node:target'); + \`, + shortCircuit: true, + }; + } + + return nextLoad(url, context); + } +`; + +register(`data:text/javascript,${encodeURIComponent(loader)}`); + +const ns = await import('custom:cjs'); +assert.strictEqual(ns.default, 'node:fs');