From 53673f349d9a28e5be01b5502edd4971a54ddf9c Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 18 Apr 2026 22:10:36 +0100 Subject: [PATCH] module: fix sync hook short-circuit in require() in imported CJS - For imported CJS, if it's not customized by asynchronous hooks, make sure it won't use the quirky re-invented require in all cases. - When the imported CJS module is customized by synchronous hooks, in the synthetic module evalutation step, avoid calling the respective default step again. - Make the branching of loadCJSModuleWithModuleLoad() and loadCJSModuleWithSpecialRequire() more explicit, and fold the tentative fs read in the 'commonjs' translator into the share createCJSModuleWrap() helper instead of checking it twice in the same path. --- lib/internal/modules/cjs/loader.js | 35 +++-- lib/internal/modules/esm/load.js | 6 - lib/internal/modules/esm/loader.js | 133 +++++++++++++----- lib/internal/modules/esm/translators.js | 113 ++++++++++----- lib/internal/test_runner/mock/mock.js | 26 +++- ...ule-hooks-load-import-cjs-custom-source.js | 41 ++++++ 6 files changed, 265 insertions(+), 89 deletions(-) create mode 100644 test/module-hooks/test-module-hooks-load-import-cjs-custom-source.js diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index d7df00b3fdc96b..ccf4ace43b2e31 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -1072,20 +1072,26 @@ function defaultResolveImplForCJSLoading(specifier, parent, isMain, options) { return wrapResolveFilename(specifier, parent, isMain, options); } +/** + * @typedef {{ + * resolved?: {url?: string, format?: string, filename: string}, + * shouldSkipModuleHooks?: boolean, + * source?: string|ArrayBufferView|ArrayBuffer, + * requireResolveOptions?: ResolveFilenameOptions, + * }} CJSModuleLoadInternalOptions + */ + /** * Resolve a module request for CommonJS, invoking hooks from module.registerHooks() * if necessary. * @param {string} specifier * @param {Module|undefined} parent * @param {boolean} isMain - * @param {object} internalResolveOptions - * @param {boolean} internalResolveOptions.shouldSkipModuleHooks Whether to skip module hooks. - * @param {ResolveFilenameOptions} internalResolveOptions.requireResolveOptions Options from require.resolve(). - * Only used when it comes from require.resolve(). + * @param {CJSModuleLoadInternalOptions} internalOptions * @returns {{url?: string, format?: string, parentURL?: string, filename: string}} */ -function resolveForCJSWithHooks(specifier, parent, isMain, internalResolveOptions) { - const { requireResolveOptions, shouldSkipModuleHooks } = internalResolveOptions; +function resolveForCJSWithHooks(specifier, parent, isMain, internalOptions) { + const { requireResolveOptions, shouldSkipModuleHooks } = internalOptions; const defaultResolveImpl = requireResolveOptions ? wrapResolveFilename : defaultResolveImplForCJSLoading; // Fast path: no hooks, just return simple results. @@ -1232,10 +1238,10 @@ function loadBuiltinWithHooks(id, url, format) { * @param {string} request Specifier of module to load via `require` * @param {Module} parent Absolute path of the module importing the child * @param {boolean} isMain Whether the module is the main entry point - * @param {object|undefined} internalResolveOptions Additional options for loading the module + * @param {CJSModuleLoadInternalOptions|undefined} internalOptions Additional options for loading the module * @returns {object} */ -Module._load = function(request, parent, isMain, internalResolveOptions = kEmptyObject) { +Module._load = function(request, parent, isMain, internalOptions = kEmptyObject) { let relResolveCacheIdentifier; if (parent) { debug('Module._load REQUEST %s parent: %s', request, parent.id); @@ -1258,7 +1264,10 @@ Module._load = function(request, parent, isMain, internalResolveOptions = kEmpty } } - const resolveResult = resolveForCJSWithHooks(request, parent, isMain, internalResolveOptions); + // If the module has been resolved by a short-circuiting synchronous resolve hook, + // avoid running the default resolution from disk again. + const resolveResult = internalOptions.resolved ?? + resolveForCJSWithHooks(request, parent, isMain, internalOptions); let { format } = resolveResult; const { url, filename } = resolveResult; @@ -1345,6 +1354,14 @@ Module._load = function(request, parent, isMain, internalResolveOptions = kEmpty module[kLastModuleParent] = parent; } + // The module source was provided by a short-circuiting synchronous hook, + // assign them into the module to avoid triggering the default load step again. + if (internalOptions.source !== undefined) { + module[kModuleSource] ??= internalOptions.source; + module[kURL] ??= url; + module[kFormat] ??= format; + } + if (parent !== undefined) { relativeResolveCache[relResolveCacheIdentifier] = filename; } diff --git a/lib/internal/modules/esm/load.js b/lib/internal/modules/esm/load.js index e8658716f881a9..94879761553e02 100644 --- a/lib/internal/modules/esm/load.js +++ b/lib/internal/modules/esm/load.js @@ -145,7 +145,6 @@ function defaultLoadSync(url, context = kEmptyObject) { throwIfUnsupportedURLScheme(urlInstance, false); - let shouldBeReloadedByCJSLoader = false; if (urlInstance.protocol === 'node:') { source = null; format ??= 'builtin'; @@ -160,10 +159,6 @@ function defaultLoadSync(url, context = kEmptyObject) { // Now that we have the source for the module, run `defaultGetFormat` to detect its format. format ??= defaultGetFormat(urlInstance, context); - - // For backward compatibility reasons, we need to let go through Module._load - // again. - shouldBeReloadedByCJSLoader = (format === 'commonjs'); } validateAttributes(url, format, importAttributes); @@ -172,7 +167,6 @@ function defaultLoadSync(url, context = kEmptyObject) { format, responseURL, source, - shouldBeReloadedByCJSLoader, }; } diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 876ea6535f2187..979c74fe3abf3b 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -124,7 +124,19 @@ const { defaultLoadSync, throwUnknownModuleFormat } = require('internal/modules/ */ /** - * @typedef {{ format: ModuleFormat, source: ModuleSource, translatorKey: string }} TranslateContext + * @typedef {{format: string, url: string, isResolvedBySyncHooks: boolean}} ResolveResult + */ + +/** + * @typedef {{ + * url: string, + * format: ModuleFormat, + * source: ModuleSource, + * responseURL?: string, + * translatorKey: string, + * isResolvedBySyncHooks: boolean, + * isSourceLoadedSynchronously: boolean, + * }} TranslateContext */ /** @@ -385,19 +397,25 @@ class ModuleLoader { /** * Load a module and translate it into a ModuleWrap for require(esm). * This is run synchronously, and the translator always return a ModuleWrap synchronously. - * @param {string} url URL of the module to be translated. + * @param {ResolveResult} resolveResult Result from the resolve step. * @param {object} loadContext See {@link load} * @param {string|undefined} parentURL URL of the parent module. Undefined if it's the entry point. * @param {ModuleRequest} request Module request. * @returns {ModuleWrap} */ - loadAndTranslateForImportInRequiredESM(url, loadContext, parentURL, request) { + loadAndTranslateForImportInRequiredESM(resolveResult, loadContext, parentURL, request) { + const { url } = resolveResult; const loadResult = this.#loadSync(url, loadContext); // Use the synchronous commonjs translator which can deal with cycles. const formatFromLoad = loadResult.format; const translatorKey = (formatFromLoad === 'commonjs' || formatFromLoad === 'commonjs-typescript') ? 'commonjs-sync' : formatFromLoad; - const translateContext = { ...loadResult, translatorKey, __proto__: null }; + const translateContext = { + ...resolveResult, + ...loadResult, + translatorKey, + __proto__: null, + }; const wrap = this.#translate(url, translateContext, parentURL); assert(wrap instanceof ModuleWrap, `Translator used for require(${url}) should not be async`); @@ -446,12 +464,13 @@ class ModuleLoader { /** * Load a module and translate it into a ModuleWrap for require() in imported CJS. * This is run synchronously, and the translator always return a ModuleWrap synchronously. - * @param {string} url URL of the module to be translated. + * @param {ResolveResult} resolveResult Result from the resolve step. * @param {object} loadContext See {@link load} * @param {string|undefined} parentURL URL of the parent module. Undefined if it's the entry point. * @returns {ModuleWrap} */ - loadAndTranslateForRequireInImportedCJS(url, loadContext, parentURL) { + loadAndTranslateForRequireInImportedCJS(resolveResult, loadContext, parentURL) { + const { url } = resolveResult; const loadResult = this.#loadSync(url, loadContext); const formatFromLoad = loadResult.format; @@ -473,7 +492,12 @@ class ModuleLoader { translatorKey = 'require-commonjs-typescript'; } - const translateContext = { ...loadResult, translatorKey, __proto__: null }; + const translateContext = { + ...resolveResult, + ...loadResult, + translatorKey, + __proto__: null, + }; const wrap = this.#translate(url, translateContext, parentURL); assert(wrap instanceof ModuleWrap, `Translator used for require(${url}) should not be async`); return wrap; @@ -482,15 +506,21 @@ class ModuleLoader { /** * Load a module and translate it into a ModuleWrap for ordinary imported ESM. * This may be run asynchronously if there are asynchronous module loader hooks registered. - * @param {string} url URL of the module to be translated. + * @param {ResolveResult} resolveResult Result from the resolve step. * @param {object} loadContext See {@link load} * @param {string|undefined} parentURL URL of the parent module. Undefined if it's the entry point. * @returns {Promise|ModuleWrap} */ - loadAndTranslate(url, loadContext, parentURL) { + loadAndTranslate(resolveResult, loadContext, parentURL) { + const { url } = resolveResult; const maybePromise = this.load(url, loadContext); const afterLoad = (loadResult) => { - const translateContext = { ...loadResult, translatorKey: loadResult.format, __proto__: null }; + const translateContext = { + ...resolveResult, + ...loadResult, + translatorKey: loadResult.format, + __proto__: null, + }; return this.#translate(url, translateContext, parentURL); }; if (isPromise(maybePromise)) { @@ -506,7 +536,7 @@ class ModuleLoader { * the module should be linked by the time this returns. Otherwise it may still have * pending module requests. * @param {string} parentURL See {@link getOrCreateModuleJob} - * @param {{format: string, url: string}} resolveResult + * @param {ResolveResult} resolveResult * @param {ModuleRequest} request Module request. * @param {ModuleRequestType} requestType Type of the module request. * @returns {ModuleJobBase} The (possibly pending) module job @@ -535,11 +565,11 @@ class ModuleLoader { let moduleOrModulePromise; if (requestType === kRequireInImportedCJS) { - moduleOrModulePromise = this.loadAndTranslateForRequireInImportedCJS(url, context, parentURL); + moduleOrModulePromise = this.loadAndTranslateForRequireInImportedCJS(resolveResult, context, parentURL); } else if (requestType === kImportInRequiredESM) { - moduleOrModulePromise = this.loadAndTranslateForImportInRequiredESM(url, context, parentURL, request); + moduleOrModulePromise = this.loadAndTranslateForImportInRequiredESM(resolveResult, context, parentURL, request); } else { - moduleOrModulePromise = this.loadAndTranslate(url, context, parentURL); + moduleOrModulePromise = this.loadAndTranslate(resolveResult, context, parentURL); } if (requestType === kImportInRequiredESM || requestType === kRequireInImportedCJS || @@ -653,7 +683,7 @@ class ModuleLoader { * @param {string} [parentURL] The URL of the module where the module request is initiated. * It's undefined if it's from the root module. * @param {ModuleRequest} request Module request. - * @returns {Promise<{format: string, url: string}>|{format: string, url: string}} + * @returns {Promise|ResolveResult} */ #resolve(parentURL, request) { if (this.isForAsyncLoaderHookWorker) { @@ -689,15 +719,18 @@ 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 {{isResolvedByDefaultResolve: boolean}} out Output object to track whether the default resolve was used + * without polluting the user-visible resolve result. * @param {string|URL} specifier See {@link resolveSync}. * @param {{ parentURL?: string, importAttributes: ImportAttributes, conditions?: string[]}} context * See {@link resolveSync}. * @returns {{ format: string, url: string }} */ - #resolveAndMaybeBlockOnLoaderThread(specifier, context) { + #resolveAndMaybeBlockOnLoaderThread(out, specifier, context) { if (this.#asyncLoaderHooks?.resolveSync) { return this.#asyncLoaderHooks.resolveSync(specifier, context.parentURL, context.importAttributes); } + out.isResolvedByDefaultResolve = true; return this.#cachedDefaultResolve(specifier, context); } @@ -712,31 +745,45 @@ 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. - * @returns {{ format: string, url: string }} + * @returns {ResolveResult} */ resolveSync(parentURL, request, shouldSkipSyncHooks = false) { 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. + const out = { isResolvedByDefaultResolve: false, __proto__: null }; + let result; + let isResolvedBySyncHooks = false; if (!shouldSkipSyncHooks && syncResolveHooks.length) { // Has module.registerHooks() hooks, chain the asynchronous hooks in the default step. - return resolveWithSyncHooks(specifier, parentURL, importAttributes, this.#defaultConditions, - this.#resolveAndMaybeBlockOnLoaderThread.bind(this)); + result = resolveWithSyncHooks(specifier, parentURL, importAttributes, this.#defaultConditions, + 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, + parentURL, + importAttributes, + __proto__: null, + }; + result = this.#resolveAndMaybeBlockOnLoaderThread(out, specifier, context); } - const context = { - ...request, - conditions: this.#defaultConditions, - parentURL, - importAttributes, - __proto__: null, - }; - return this.#resolveAndMaybeBlockOnLoaderThread(specifier, context); + result.isResolvedBySyncHooks = isResolvedBySyncHooks; + return result; } /** * Provide source that is understood by one of Node's translators. Handles customization hooks, * if any. - * @typedef { {format: ModuleFormat, source: ModuleSource }} LoadResult + * @typedef {{ + * format: ModuleFormat, + * source: ModuleSource, + * responseURL?: string, + * isSourceLoadedSynchronously: boolean, + * }} LoadResult * @param {string} url The URL of the module to be loaded. * @param {object} context Metadata about the module * @returns {Promise | LoadResult}} @@ -752,14 +799,19 @@ class ModuleLoader { /** * This is the default load step for module.registerHooks(), which incorporates asynchronous hooks * from module.register() which are run in a blocking fashion for it to be synchronous. + * @param {{isSourceLoadedSynchronously: boolean}} out + * Output object to track whether the source was loaded synchronously without polluting + * the user-visible load result. * @param {string} url See {@link load} * @param {object} context See {@link load} * @returns {{ format: ModuleFormat, source: ModuleSource }} */ - #loadAndMaybeBlockOnLoaderThread(url, context) { + #loadAndMaybeBlockOnLoaderThread(out, url, context) { if (this.#asyncLoaderHooks?.loadSync) { + out.isSourceLoadedSynchronously = false; return this.#asyncLoaderHooks.loadSync(url, context); } + out.isSourceLoadedSynchronously = true; return defaultLoadSync(url, context); } @@ -770,17 +822,32 @@ class ModuleLoader { * This is here to support `require()` in imported CJS and `module.registerHooks()` hooks. * @param {string} url See {@link load} * @param {object} [context] See {@link load} - * @returns {{ format: ModuleFormat, source: ModuleSource }} + * @returns {LoadResult} */ #loadSync(url, context) { + // Use an output parameter to track the state and avoid polluting the user-visible resolve results. + const out = { + isSourceLoadedSynchronously: true, + __proto__: null, + }; + let result; if (syncLoadHooks.length) { // Has module.registerHooks() hooks, chain the asynchronous hooks in the default step. // TODO(joyeecheung): construct the ModuleLoadContext in the loaders directly instead // of converting them from plain objects in the hooks. - return loadWithSyncHooks(url, context.format, context.importAttributes, this.#defaultConditions, - this.#loadAndMaybeBlockOnLoaderThread.bind(this), validateLoadSloppy); + result = loadWithSyncHooks( + url, + context.format, + context.importAttributes, + this.#defaultConditions, + this.#loadAndMaybeBlockOnLoaderThread.bind(this, out), + validateLoadSloppy, + ); + } else { + result = this.#loadAndMaybeBlockOnLoaderThread(out, url, context); } - return this.#loadAndMaybeBlockOnLoaderThread(url, context); + result.isSourceLoadedSynchronously = out.isSourceLoadedSynchronously; + return result; } validateLoadResult(url, format) { diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index c947bf8f69c1bd..c453e2b54f5957 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -22,7 +22,6 @@ const { const { BuiltinModule } = require('internal/bootstrap/realm'); const assert = require('internal/assert'); -const fs = require('fs'); const { dirname, extname } = require('path'); const { assertBufferSource, @@ -97,6 +96,8 @@ const kShouldSkipModuleHooks = { __proto__: null, shouldSkipModuleHooks: true }; const kShouldNotSkipModuleHooks = { __proto__: null, shouldSkipModuleHooks: false }; /** + * This may be eventually removed when module.register() reaches end-of-life. + * * Loads a CommonJS module via the ESM Loader sync CommonJS translator. * This translator creates its own version of the `require` function passed into CommonJS modules. * Any monkey patches applied to the CommonJS Loader will not affect this module. @@ -106,8 +107,9 @@ const kShouldNotSkipModuleHooks = { __proto__: null, shouldSkipModuleHooks: fals * @param {string} url - The URL of the module. * @param {string} filename - The filename of the module. * @param {boolean} isMain - Whether the module is the entrypoint + * @param {TranslateContext} translateContext Context for the translator */ -function loadCJSModule(module, source, url, filename, isMain) { +function loadCJSModuleWithSpecialRequire(module, source, url, filename, isMain, translateContext) { // Use the full URL as the V8 resource name so that any search params // (e.g. ?node-test-mock) are preserved in coverage reports. const compileResult = compileFunctionForCJSLoader(source, url, false /* is_sea_main */, false); @@ -194,21 +196,24 @@ const cjsCache = new SafeMap(); /** * Creates a ModuleWrap object for a CommonJS module. * @param {string} url - The URL of the module. - * @param {{ format: ModuleFormat, source: ModuleSource }} translateContext Context for the translator + * @param {import('./loader').TranslateContext} translateContext Context for the translator * @param {string|undefined} parentURL URL of the module initiating the module loading for the first time. * Undefined if it's the entry point. - * @param {typeof loadCJSModule} [loadCJS] - The function to load the CommonJS module. * @returns {ModuleWrap} The ModuleWrap object for the CommonJS module. */ -function createCJSModuleWrap(url, translateContext, parentURL, loadCJS = loadCJSModule) { +function createCJSModuleWrap(url, translateContext, parentURL) { debug(`Translating CJSModule ${url}`, translateContext); const { format: sourceFormat } = translateContext; let { source } = translateContext; const isMain = (parentURL === undefined); const filename = urlToFilename(url); - // In case the source was not provided by the `load` step, we need fetch it now. - source = stringify(source ?? getSourceSync(new URL(url)).source); + try { + // In case the source was not provided by the `load` step, we need fetch it now. + source = stringify(source ?? getSourceSync(new URL(url)).source); + } catch { + // Continue regardless of error. + } const { exportNames, module } = cjsPreparseModuleExports(filename, source, sourceFormat); cjsCache.set(url, module); @@ -229,7 +234,19 @@ function createCJSModuleWrap(url, translateContext, parentURL, loadCJS = loadCJS debug(`Loading CJSModule ${url}`); if (!module.loaded) { - loadCJS(module, source, url, filename, !!isMain); + // For backward-compatibility, it's possible for async hooks to return a nullish value for + // CJS source associated with a `file:` URL - that usually means the source is not + // customized (is loaded by default load) or the hook author wants it to be reloaded + // through CJS routine. In this case, the source is obtained by calling the + // Module._load(). + if (translateContext.translatorKey === 'commonjs-sync' || + translateContext.isSourceLoadedSynchronously || + translateContext.source == null) { + loadCJSModuleWithModuleLoad(module, source, url, filename, !!isMain, translateContext); + } else { // CommonJS with source customized by async hooks + // This may be eventually removed when module.register() reaches end-of-life. + loadCJSModuleWithSpecialRequire(module, source, url, filename, !!isMain, translateContext); + } } let exports; @@ -303,55 +320,73 @@ function createCJSNoSourceModuleWrap(url, parentURL) { } translators.set('commonjs-sync', function requireCommonJS(url, translateContext, parentURL) { - return createCJSModuleWrap(url, translateContext, parentURL, loadCJSModuleWithModuleLoad); + return createCJSModuleWrap(url, translateContext, parentURL); }); -// Handle CommonJS modules referenced by `require` calls. -// This translator function must be sync, as `require` is sync. +// Handle CommonJS modules referenced by `require` calls using re-invented require. +// This path is only used by require() from imported CJS customized by the *async* +// loader hooks. translators.set('require-commonjs', (url, translateContext, parentURL) => { return createCJSModuleWrap(url, translateContext, parentURL); }); -// Handle CommonJS modules referenced by `require` calls. -// This translator function must be sync, as `require` is sync. +// Handle TypeScript CommonJS modules referenced by `require` calls using re-invented require. +// This path is only used by require() from imported CJS customized by the *async* +// loader hooks. translators.set('require-commonjs-typescript', (url, translateContext, parentURL) => { translateContext.source = stripTypeScriptModuleTypes(stringify(translateContext.source), url); return createCJSModuleWrap(url, translateContext, parentURL); }); // This goes through Module._load to accommodate monkey-patchers. -function loadCJSModuleWithModuleLoad(module, source, url, filename, isMain) { +/** + * Loads a CommonJS module through Module._load to accommodate monkey-patchers. + * If the module was resolved by synchronous hooks (i.e. not by the default resolver), + * passes the pre-resolved information and source to Module._load to avoid + * re-resolving and re-loading. + * @param {import('internal/modules/cjs/loader').Module} module - The module to load. + * @param {string} source - The source code of the module. + * @param {string} url - The URL of the module. + * @param {string} filename - The filename of the module. + * @param {boolean} isMain - Whether the module is the entrypoint + * @param {import('./loader').TranslateContext} translateContext Context for the translator + */ +function loadCJSModuleWithModuleLoad(module, source, url, filename, isMain, translateContext) { assert(module === CJSModule._cache[filename]); - // If it gets here in the translators, the hooks must have already been invoked - // in the loader. Skip them in the synthetic module evaluation step. - wrapModuleLoad(filename, undefined, isMain, kShouldSkipModuleHooks); + debug(`loadCJSModuleWithModuleLoad ${url}`); + let exports; + if (translateContext.isResolvedBySyncHooks) { + exports = wrapModuleLoad(filename, undefined, isMain, { + __proto__: null, + resolved: { + __proto__: null, + filename, + format: translateContext.format, + url, + }, + shouldSkipModuleHooks: true, + source, + }); + } else { + // If it gets here in the translators, the hooks must have already been invoked + // in the loader. Skip them in the synthetic module evaluation step. + exports = wrapModuleLoad(filename, undefined, isMain, kShouldSkipModuleHooks); + } + + // Patched Module._load implementations may return exports without updating the + // ESM-created cache entry. Mirror the returned value into the translator-owned + // module so the synthetic module namespace observes the loaded exports. + if (!module.loaded) { + module.exports = exports; + module.loaded = true; + } + module[kModuleExport] = exports; } // Handle CommonJS modules referenced by `import` statements or expressions, // or as the initial entry point when the ESM loader handles a CommonJS entry. translators.set('commonjs', function commonjsStrategy(url, translateContext, parentURL) { - // For backward-compatibility, it's possible to return a nullish value for - // CJS source associated with a `file:` URL - that usually means the source is not - // customized (is loaded by default load) or the hook author wants it to be reloaded - // through CJS routine. In this case, the source is obtained by calling the - // monkey-patchable CJS loader. - // TODO(joyeecheung): just use wrapModuleLoad and let the CJS loader - // invoke the off-thread hooks. Use a special parent to avoid invoking in-thread - // hooks twice. - const shouldReloadByCJSLoader = (translateContext.shouldBeReloadedByCJSLoader || translateContext.source == null); - const cjsLoader = shouldReloadByCJSLoader ? loadCJSModuleWithModuleLoad : loadCJSModule; - - try { - // We still need to read the FS to detect the exports. - // If you are reading this code to figure out how to patch Node.js module loading - // behavior - DO NOT depend on the patchability in new code: Node.js - // internals may stop going through the JavaScript fs module entirely. - // Prefer module.registerHooks() or other more formal fs hooks released in the future. - translateContext.source ??= fs.readFileSync(new URL(url), 'utf8'); - } catch { - // Continue regardless of error. - } - return createCJSModuleWrap(url, translateContext, parentURL, cjsLoader); + return createCJSModuleWrap(url, translateContext, parentURL); }); /** diff --git a/lib/internal/test_runner/mock/mock.js b/lib/internal/test_runner/mock/mock.js index fb1ed322b414fc..d15ace222b2132 100644 --- a/lib/internal/test_runner/mock/mock.js +++ b/lib/internal/test_runner/mock/mock.js @@ -51,8 +51,15 @@ const { validateOneOf, } = require('internal/validators'); const { MockTimers } = require('internal/test_runner/mock/mock_timers'); -const { Module } = require('internal/modules/cjs/loader'); -const { _load, _nodeModulePaths, _resolveFilename, isBuiltin } = Module; +const { + Module, +} = require('internal/modules/cjs/loader'); +const { + _load, + _nodeModulePaths, + _resolveFilename, + isBuiltin, +} = Module; function kDefaultFunction() {} const enableModuleMocking = getOptionValue('--experimental-test-module-mocks'); const kSupportedFormats = [ @@ -905,6 +912,21 @@ function setupSharedModuleState() { } function cjsMockModuleLoad(request, parent, isMain) { + // Imported mocked URLs may re-enter Module._load with the mock query attached. + // Strip it to pass into methods that expect a normal request. + // TODO(joyeecheung): it might be better to strip the search params from the filename in + // the translator but that might have a bigger blast radius as other mocker might have also + // come to rely on this to create multiple cache identities for the same module. + try { + const parsedRequest = URLParse(request); + if (parsedRequest?.searchParams.has(kMockSearchParam)) { + parsedRequest.searchParams.delete(kMockSearchParam); + request = parsedRequest.href; + } + } catch { + // Not a valid URL, treat as a normal request. + } + let resolved; if (isBuiltin(request)) { diff --git a/test/module-hooks/test-module-hooks-load-import-cjs-custom-source.js b/test/module-hooks/test-module-hooks-load-import-cjs-custom-source.js new file mode 100644 index 00000000000000..723f5158cebc9f --- /dev/null +++ b/test/module-hooks/test-module-hooks-load-import-cjs-custom-source.js @@ -0,0 +1,41 @@ +'use strict'; +// Test that imported CJS loaded from sync hooks are handled correctly and +// won't invoke the CJS loading steps again (in which case it would throw +// because the source is not on disk). + +const common = require('../common'); +const assert = require('assert'); +const { registerHooks } = require('module'); + +const virtualModules = new Map([ + ['virtual:shared', 'module.exports.greet = (name) => "hello " + name;'], + ['virtual:entry', 'const { greet } = require("virtual:shared");\nmodule.exports = greet("world");'], +]); + +const hook = registerHooks({ + resolve: common.mustCall((specifier, context, nextResolve) => { + if (virtualModules.has(specifier)) { + return { + url: specifier, + format: 'commonjs', + shortCircuit: true, + }; + } + return nextResolve(specifier, context); + }, 2), + load: common.mustCall((url, context, nextLoad) => { + if (virtualModules.has(url)) { + return { + format: 'commonjs', + source: virtualModules.get(url), + shortCircuit: true, + }; + } + return nextLoad(url, context); + }, 2), +}); + +import('virtual:entry').then(common.mustCall((ns) => { + assert.strictEqual(ns.default, 'hello world'); + hook.deregister(); +}));