From 7e19cd011ea94e6a92dafbf9bff2e95148d96a1f Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 8 Sep 2025 16:59:31 +0200 Subject: [PATCH 1/7] esm: populate separate cache for require(esm) in imported CJS Otherwise if the ESM happens to be cached separately by the ESM loader before it gets loaded with `require(esm)` from within an imported CJS file (which uses a re-invented require() with a couple of quirks, including a separate cache), it won't be able to load the esm properly from the cache. PR-URL: https://github.com/nodejs/node/pull/59679 Refs: https://github.com/nodejs/node/issues/59666 Refs: https://github.com/nodejs/node/issues/52697 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Chengzhong Wu --- lib/internal/modules/cjs/loader.js | 138 ++++++++++-------- lib/internal/modules/esm/loader.js | 47 ++++-- lib/internal/modules/esm/translators.js | 35 ++++- lib/internal/modules/helpers.js | 23 ++- src/env_properties.h | 4 +- src/module_wrap.cc | 7 + .../test-require-esm-from-imported-cjs.js | 36 +++++ .../require-esm-in-cjs-cache/app.cjs | 1 + .../require-esm-in-cjs-cache/hooks.js | 12 ++ .../instrument-sync.js | 7 + .../require-esm-in-cjs-cache/instrument.js | 3 + .../require-esm-in-cjs-cache/package.json | 3 + .../require-esm-in-cjs-cache/test.js | 1 + 13 files changed, 239 insertions(+), 78 deletions(-) create mode 100644 test/es-module/test-require-esm-from-imported-cjs.js create mode 100644 test/fixtures/es-modules/require-esm-in-cjs-cache/app.cjs create mode 100644 test/fixtures/es-modules/require-esm-in-cjs-cache/hooks.js create mode 100644 test/fixtures/es-modules/require-esm-in-cjs-cache/instrument-sync.js create mode 100644 test/fixtures/es-modules/require-esm-in-cjs-cache/instrument.js create mode 100644 test/fixtures/es-modules/require-esm-in-cjs-cache/package.json create mode 100644 test/fixtures/es-modules/require-esm-in-cjs-cache/test.js diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index b87f557c16820e..d40b074a11efa9 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -69,7 +69,8 @@ const { module_export_names_private_symbol, module_circular_visited_private_symbol, module_export_private_symbol, - module_parent_private_symbol, + module_first_parent_private_symbol, + module_last_parent_private_symbol, }, isInsideNodeModules, } = internalBinding('util'); @@ -94,9 +95,13 @@ const kModuleCircularVisited = module_circular_visited_private_symbol; */ const kModuleExport = module_export_private_symbol; /** - * {@link Module} parent module. + * {@link Module} The first parent module that loads a module with require(). */ -const kModuleParent = module_parent_private_symbol; +const kFirstModuleParent = module_first_parent_private_symbol; +/** + * {@link Module} The last parent module that loads a module with require(). + */ +const kLastModuleParent = module_last_parent_private_symbol; const kIsMainSymbol = Symbol('kIsMainSymbol'); const kIsCachedByESMLoader = Symbol('kIsCachedByESMLoader'); @@ -117,6 +122,7 @@ module.exports = { findLongestRegisteredExtension, resolveForCJSWithHooks, loadSourceForCJSWithHooks: loadSource, + populateCJSExportsFromESM, wrapSafe, wrapModuleLoad, kIsMainSymbol, @@ -320,7 +326,8 @@ function Module(id = '', parent) { this.id = id; this.path = path.dirname(id); setOwnProperty(this, 'exports', {}); - this[kModuleParent] = parent; + this[kFirstModuleParent] = parent; + this[kLastModuleParent] = parent; updateChildren(parent, this, false); this.filename = null; this.loaded = false; @@ -400,7 +407,7 @@ ObjectDefineProperty(BuiltinModule.prototype, 'isPreloading', isPreloadingDesc); * @this {Module} */ function getModuleParent() { - return this[kModuleParent]; + return this[kFirstModuleParent]; } /** @@ -409,7 +416,7 @@ function getModuleParent() { * @param {Module} value */ function setModuleParent(value) { - this[kModuleParent] = value; + this[kFirstModuleParent] = value; } let debug = debuglog('module', (fn) => { @@ -972,7 +979,7 @@ function getExportsForCircularRequire(module) { const requiredESM = module[kRequiredModuleSymbol]; if (requiredESM && requiredESM.getStatus() !== kEvaluated) { let message = `Cannot require() ES Module ${module.id} in a cycle.`; - const parent = module[kModuleParent]; + const parent = module[kLastModuleParent]; if (parent) { message += ` (from ${parent.filename})`; } @@ -1252,6 +1259,8 @@ Module._load = function(request, parent, isMain) { // load hooks for the module keyed by the (potentially customized) filename. module[kURL] = url; module[kFormat] = format; + } else { + module[kLastModuleParent] = parent; } if (parent !== undefined) { @@ -1371,7 +1380,8 @@ Module._resolveFilename = function(request, parent, isMain, options) { const requireStack = []; for (let cursor = parent; cursor; - cursor = cursor[kModuleParent]) { + // TODO(joyeecheung): it makes more sense to use kLastModuleParent here. + cursor = cursor[kFirstModuleParent]) { ArrayPrototypePush(requireStack, cursor.filename || cursor.id); } let message = `Cannot find module '${request}'`; @@ -1485,7 +1495,7 @@ function loadESMFromCJS(mod, filename, format, source) { // ESM won't be accessible via process.mainModule. setOwnProperty(process, 'mainModule', undefined); } else { - const parent = mod[kModuleParent]; + const parent = mod[kLastModuleParent]; requireModuleWarningMode ??= getOptionValue('--trace-require-module'); if (requireModuleWarningMode) { @@ -1534,54 +1544,66 @@ function loadESMFromCJS(mod, filename, format, source) { wrap, namespace, } = cascadedLoader.importSyncForRequire(mod, filename, source, isMain, parent); - // Tooling in the ecosystem have been using the __esModule property to recognize - // transpiled ESM in consuming code. For example, a 'log' package written in ESM: - // - // export default function log(val) { console.log(val); } - // - // Can be transpiled as: - // - // exports.__esModule = true; - // exports.default = function log(val) { console.log(val); } - // - // The consuming code may be written like this in ESM: - // - // import log from 'log' - // - // Which gets transpiled to: - // - // const _mod = require('log'); - // const log = _mod.__esModule ? _mod.default : _mod; - // - // So to allow transpiled consuming code to recognize require()'d real ESM - // as ESM and pick up the default exports, we add a __esModule property by - // building a source text module facade for any module that has a default - // export and add .__esModule = true to the exports. This maintains the - // enumerability of the re-exported names and the live binding of the exports, - // without incurring a non-trivial per-access overhead on the exports. - // - // The source of the facade is defined as a constant per-isolate property - // required_module_default_facade_source_string, which looks like this - // - // export * from 'original'; - // export { default } from 'original'; - // export const __esModule = true; - // - // And the 'original' module request is always resolved by - // createRequiredModuleFacade() to `wrap` which is a ModuleWrap wrapping - // over the original module. - - // We don't do this to modules that are marked as CJS ESM or that - // don't have default exports to avoid the unnecessary overhead. - // If __esModule is already defined, we will also skip the extension - // to allow users to override it. - if (ObjectHasOwn(namespace, 'module.exports')) { - mod.exports = namespace['module.exports']; - } else if (!ObjectHasOwn(namespace, 'default') || ObjectHasOwn(namespace, '__esModule')) { - mod.exports = namespace; - } else { - mod.exports = createRequiredModuleFacade(wrap); - } + + populateCJSExportsFromESM(mod, wrap, namespace); + } +} + +/** + * Populate the exports of a CJS module entry from an ESM module's namespace object for + * require(esm). + * @param {Module} mod CJS module instance + * @param {ModuleWrap} wrap ESM ModuleWrap instance. + * @param {object} namespace The ESM namespace object. + */ +function populateCJSExportsFromESM(mod, wrap, namespace) { + // Tooling in the ecosystem have been using the __esModule property to recognize + // transpiled ESM in consuming code. For example, a 'log' package written in ESM: + // + // export default function log(val) { console.log(val); } + // + // Can be transpiled as: + // + // exports.__esModule = true; + // exports.default = function log(val) { console.log(val); } + // + // The consuming code may be written like this in ESM: + // + // import log from 'log' + // + // Which gets transpiled to: + // + // const _mod = require('log'); + // const log = _mod.__esModule ? _mod.default : _mod; + // + // So to allow transpiled consuming code to recognize require()'d real ESM + // as ESM and pick up the default exports, we add a __esModule property by + // building a source text module facade for any module that has a default + // export and add .__esModule = true to the exports. This maintains the + // enumerability of the re-exported names and the live binding of the exports, + // without incurring a non-trivial per-access overhead on the exports. + // + // The source of the facade is defined as a constant per-isolate property + // required_module_default_facade_source_string, which looks like this + // + // export * from 'original'; + // export { default } from 'original'; + // export const __esModule = true; + // + // And the 'original' module request is always resolved by + // createRequiredModuleFacade() to `wrap` which is a ModuleWrap wrapping + // over the original module. + + // We don't do this to modules that are marked as CJS ESM or that + // don't have default exports to avoid the unnecessary overhead. + // If __esModule is already defined, we will also skip the extension + // to allow users to override it. + if (ObjectHasOwn(namespace, 'module.exports')) { + mod.exports = namespace['module.exports']; + } else if (!ObjectHasOwn(namespace, 'default') || ObjectHasOwn(namespace, '__esModule')) { + mod.exports = namespace; + } else { + mod.exports = createRequiredModuleFacade(wrap); } } @@ -1772,7 +1794,7 @@ function reconstructErrorStack(err, parentPath, parentSource) { */ function getRequireESMError(mod, pkg, content, filename) { // This is an error path because `require` of a `.js` file in a `"type": "module"` scope is not allowed. - const parent = mod[kModuleParent]; + const parent = mod[kFirstModuleParent]; const parentPath = parent?.filename; const packageJsonPath = pkg?.path; const usesEsm = containsModuleSyntax(content, filename); diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 78985575beb3df..8da8807baebf04 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -17,6 +17,7 @@ const { const { kIsExecuting, kRequiredModuleSymbol, + Module: CJSModule, } = require('internal/modules/cjs/loader'); const { imported_cjs_symbol } = internalBinding('symbols'); @@ -89,13 +90,18 @@ function newLoadCache() { return new LoadCache(); } +let _translators; +function lazyLoadTranslators() { + _translators ??= require('internal/modules/esm/translators'); + return _translators; +} + /** * Lazy-load translators to avoid potentially unnecessary work at startup (ex if ESM is not used). * @returns {import('./translators.js').Translators} */ function getTranslators() { - const { translators } = require('internal/modules/esm/translators'); - return translators; + return lazyLoadTranslators().translators; } /** @@ -496,7 +502,7 @@ class ModuleLoader { const { source } = loadResult; const isMain = (parentURL === undefined); - const wrap = this.#translate(url, finalFormat, source, isMain); + const wrap = this.#translate(url, finalFormat, source, parentURL); assert(wrap instanceof ModuleWrap, `Translator used for require(${url}) should not be async`); if (process.env.WATCH_REPORT_DEPENDENCIES && process.send) { @@ -532,10 +538,10 @@ class ModuleLoader { * @param {string} format Format of the module to be translated. This is used to find * matching translators. * @param {ModuleSource} source Source of the module to be translated. - * @param {boolean} isMain Whether the module to be translated is the entry point. + * @param {string|undefined} parentURL URL of the parent module. Undefined if it's the entry point. * @returns {ModuleWrap} */ - #translate(url, format, source, isMain) { + #translate(url, format, source, parentURL) { this.validateLoadResult(url, format); const translator = getTranslators().get(format); @@ -543,7 +549,20 @@ class ModuleLoader { throw new ERR_UNKNOWN_MODULE_FORMAT(format, url); } - const result = FunctionPrototypeCall(translator, this, url, source, isMain); + // Populate the CJS cache with a facade for ESM in case subsequent require(esm) is + // looking it up from the cache. The parent module of the CJS cache entry would be the + // first CJS module that loads it with require(). This is an approximation, because + // ESM caches more and it does not get re-loaded and updated every time an `import` is + // encountered, unlike CJS require(), and we only use the parent entry to provide + // more information in error messages. + if (format === 'module') { + const parentFilename = urlToFilename(parentURL); + const parent = parentFilename ? CJSModule._cache[parentFilename] : undefined; + const cjsModule = lazyLoadTranslators().cjsEmplaceModuleCacheEntryForURL(url, parent); + debug('cjsEmplaceModuleCacheEntryForURL', url, parent, cjsModule); + } + + const result = FunctionPrototypeCall(translator, this, url, source, parentURL === undefined); assert(result instanceof ModuleWrap); return result; } @@ -553,10 +572,10 @@ class ModuleLoader { * This is run synchronously, and the translator always return a ModuleWrap synchronously. * @param {string} url URL of the module to be translated. * @param {object} loadContext See {@link load} - * @param {boolean} isMain Whether the module to be translated is the entry point. + * @param {string|undefined} parentURL URL of the parent module. Undefined if it's the entry point. * @returns {ModuleWrap} */ - loadAndTranslateForRequireInImportedCJS(url, loadContext, isMain) { + loadAndTranslateForRequireInImportedCJS(url, loadContext, parentURL) { const { format: formatFromLoad, source } = this.#loadSync(url, loadContext); if (formatFromLoad === 'wasm') { // require(wasm) is not supported. @@ -577,7 +596,7 @@ class ModuleLoader { finalFormat = 'require-commonjs-typescript'; } - const wrap = this.#translate(url, finalFormat, source, isMain); + const wrap = this.#translate(url, finalFormat, source, parentURL); assert(wrap instanceof ModuleWrap, `Translator used for require(${url}) should not be async`); return wrap; } @@ -587,13 +606,13 @@ class ModuleLoader { * This may be run asynchronously if there are asynchronous module loader hooks registered. * @param {string} url URL of the module to be translated. * @param {object} loadContext See {@link load} - * @param {boolean} isMain Whether the module to be translated is the entry point. + * @param {string|undefined} parentURL URL of the parent module. Undefined if it's the entry point. * @returns {Promise|ModuleWrap} */ - loadAndTranslate(url, loadContext, isMain) { + loadAndTranslate(url, loadContext, parentURL) { const maybePromise = this.load(url, loadContext); const afterLoad = ({ format, source }) => { - return this.#translate(url, format, source, isMain); + return this.#translate(url, format, source, parentURL); }; if (isPromise(maybePromise)) { return maybePromise.then(afterLoad); @@ -619,9 +638,9 @@ class ModuleLoader { const isMain = parentURL === undefined; let moduleOrModulePromise; if (isForRequireInImportedCJS) { - moduleOrModulePromise = this.loadAndTranslateForRequireInImportedCJS(url, context, isMain); + moduleOrModulePromise = this.loadAndTranslateForRequireInImportedCJS(url, context, parentURL); } else { - moduleOrModulePromise = this.loadAndTranslate(url, context, isMain); + moduleOrModulePromise = this.loadAndTranslate(url, context, parentURL); } const inspectBrk = ( diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index 757f093becd112..c0cb6ef091708e 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -44,6 +44,7 @@ const { findLongestRegisteredExtension, resolveForCJSWithHooks, loadSourceForCJSWithHooks, + populateCJSExportsFromESM, } = require('internal/modules/cjs/loader'); const { fileURLToPath, pathToFileURL, URL } = require('internal/url'); let debug = require('internal/util/debuglog').debuglog('esm', (fn) => { @@ -147,9 +148,19 @@ function loadCJSModule(module, source, url, filename, isMain) { } specifier = `${pathToFileURL(path)}`; } + + // FIXME(node:59666) Currently, the ESM loader re-invents require() here for imported CJS and this + // requires a separate cache to be populated as well as introducing several quirks. This is not ideal. const job = cascadedLoader.getModuleJobForRequireInImportedCJS(specifier, url, importAttributes); job.runSync(); - return cjsCache.get(job.url).exports; + const mod = cjsCache.get(job.url); + assert(mod, `Imported CJS module ${url} failed to load module ${job.url} using require()`); + if (!job.module.synthetic && !mod.loaded) { + debug('populateCJSExportsFromESM from require(esm) in imported CJS', url, mod, job.module); + populateCJSExportsFromESM(mod, job.module, job.module.getNamespace()); + mod.loaded = true; + } + return mod.exports; }; setOwnProperty(requireFn, 'resolve', function resolve(specifier) { if (!StringPrototypeStartsWith(specifier, 'node:')) { @@ -170,6 +181,7 @@ function loadCJSModule(module, source, url, filename, isMain) { // TODO: can we use a weak map instead? const cjsCache = new SafeMap(); + /** * Creates a ModuleWrap object for a CommonJS module. * @param {string} url - The URL of the module. @@ -321,16 +333,17 @@ translators.set('commonjs', function commonjsStrategy(url, source, isMain) { /** * Get or create an entry in the CJS module cache for the given filename. * @param {string} filename CJS module filename + * @param {CJSModule} parent The parent CJS module * @returns {CJSModule} the cached CJS module entry */ -function cjsEmplaceModuleCacheEntry(filename, exportNames) { +function cjsEmplaceModuleCacheEntry(filename, parent) { // TODO: Do we want to keep hitting the user mutable CJS loader here? let cjsMod = CJSModule._cache[filename]; if (cjsMod) { return cjsMod; } - cjsMod = new CJSModule(filename); + cjsMod = new CJSModule(filename, parent); cjsMod.filename = filename; cjsMod.paths = CJSModule._nodeModulePaths(cjsMod.path); cjsMod[kIsCachedByESMLoader] = true; @@ -339,6 +352,22 @@ function cjsEmplaceModuleCacheEntry(filename, exportNames) { return cjsMod; } +/** + * Emplace a CJS module cache entry for the given URL. + * @param {string} url The module URL + * @param {CJSModule} parent The parent CJS module + * @returns {CJSModule|undefined} the cached CJS module entry, undefined if url cannot be used to identify a CJS entry. + */ +exports.cjsEmplaceModuleCacheEntryForURL = function cjsEmplaceModuleCacheEntryForURL(url, parent) { + const filename = urlToFilename(url); + if (!filename) { + return; + } + const cjsModule = cjsEmplaceModuleCacheEntry(filename, parent); + cjsCache.set(url, cjsModule); + return cjsModule; +}; + /** * Pre-parses a CommonJS module's exports and re-exports. * @param {string} filename - The filename of the module. diff --git a/lib/internal/modules/helpers.js b/lib/internal/modules/helpers.js index f906d69b7359ac..d1938bb4c92a3a 100644 --- a/lib/internal/modules/helpers.js +++ b/lib/internal/modules/helpers.js @@ -22,7 +22,7 @@ const { validateString } = require('internal/validators'); const fs = require('fs'); // Import all of `fs` so that it can be monkey-patched. const internalFS = require('internal/fs/utils'); const path = require('path'); -const { pathToFileURL, fileURLToPath } = require('internal/url'); +const { pathToFileURL, fileURLToPath, URL } = require('internal/url'); const assert = require('internal/assert'); const { getOptionValue } = require('internal/options'); @@ -288,12 +288,31 @@ function normalizeReferrerURL(referrerName) { /** + * Coerce a URL string to a filename. This is used by the ESM loader + * to map ESM URLs to entries in the CJS module cache on a best-effort basis. + * TODO(joyeecheung): this can be rather expensive, cache the result on the + * ModuleWrap wherever we can. * @param {string|undefined} url URL to convert to filename */ function urlToFilename(url) { if (url && StringPrototypeStartsWith(url, 'file://')) { - return fileURLToPath(url); + let urlObj; + try { + urlObj = new URL(url); + } catch { + // Not a proper URL, return as-is as the cache key. + return url; + } + try { + return fileURLToPath(urlObj); + } catch { + // This is generally only possible when the URL is provided by a custom loader. + // Just use the path and ignore whether it's absolute or not as there's no such + // requirement for CJS cache. + return urlObj.pathname; + } } + // Not a file URL, return as-is. return url; } diff --git a/src/env_properties.h b/src/env_properties.h index 82225b0a53dd82..c7082150f68e28 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -30,7 +30,8 @@ V(module_export_names_private_symbol, "node:module_export_names") \ V(module_circular_visited_private_symbol, "node:module_circular_visited") \ V(module_export_private_symbol, "node:module_export") \ - V(module_parent_private_symbol, "node:module_parent") \ + V(module_first_parent_private_symbol, "node:module_first_parent") \ + V(module_last_parent_private_symbol, "node:module_last_parent") \ V(napi_type_tag, "node:napi:type_tag") \ V(napi_wrapper, "node:napi:wrapper") \ V(untransferable_object_private_symbol, "node:untransferableObject") \ @@ -373,6 +374,7 @@ V(stream_count_string, "streamCount") \ V(subject_string, "subject") \ V(subjectaltname_string, "subjectaltname") \ + V(synthetic_string, "synthetic") \ V(syscall_string, "syscall") \ V(table_string, "table") \ V(target_string, "target") \ diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 1ff4971d6fedf6..68c92b4dd18131 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -420,6 +420,13 @@ void ModuleWrap::New(const FunctionCallbackInfo& args) { } } + if (that->Set(context, + realm->isolate_data()->synthetic_string(), + v8::Boolean::New(isolate, synthetic)) + .IsNothing()) { + return; + } + if (!that->Set(context, realm->isolate_data()->url_string(), url) .FromMaybe(false)) { return; diff --git a/test/es-module/test-require-esm-from-imported-cjs.js b/test/es-module/test-require-esm-from-imported-cjs.js new file mode 100644 index 00000000000000..b03b41beb1208e --- /dev/null +++ b/test/es-module/test-require-esm-from-imported-cjs.js @@ -0,0 +1,36 @@ +'use strict'; + +// This tests that the require(esm) works from an imported CJS module +// when the require-d ESM is cached separately. + +require('../common'); +const { spawnSyncAndAssert } = require('../common/child_process'); +const fixtures = require('../common/fixtures'); + +spawnSyncAndAssert( + process.execPath, + [ + '--experimental-require-module', + '--import', + fixtures.fileURL('es-modules', 'require-esm-in-cjs-cache', 'instrument-sync.js'), + fixtures.path('es-modules', 'require-esm-in-cjs-cache', 'app.cjs'), + ], + { + trim: true, + stdout: / default: { hello: 'world' }/ + } +); + +spawnSyncAndAssert( + process.execPath, + [ + '--experimental-require-module', + '--import', + fixtures.fileURL('es-modules', 'require-esm-in-cjs-cache', 'instrument.js'), + fixtures.path('es-modules', 'require-esm-in-cjs-cache', 'app.cjs'), + ], + { + trim: true, + stdout: / default: { hello: 'world' }/ + } +); diff --git a/test/fixtures/es-modules/require-esm-in-cjs-cache/app.cjs b/test/fixtures/es-modules/require-esm-in-cjs-cache/app.cjs new file mode 100644 index 00000000000000..66a9fdfe61a623 --- /dev/null +++ b/test/fixtures/es-modules/require-esm-in-cjs-cache/app.cjs @@ -0,0 +1 @@ +console.log(require('./test.js')); diff --git a/test/fixtures/es-modules/require-esm-in-cjs-cache/hooks.js b/test/fixtures/es-modules/require-esm-in-cjs-cache/hooks.js new file mode 100644 index 00000000000000..f33a40fba77319 --- /dev/null +++ b/test/fixtures/es-modules/require-esm-in-cjs-cache/hooks.js @@ -0,0 +1,12 @@ +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; + +export async function load(url, context, nextLoad) { + const result = await nextLoad(url, context); + + if (result.format === 'commonjs' && !result.source) { + result.source = readFileSync(fileURLToPath(url), 'utf8'); + } + + return result; +} diff --git a/test/fixtures/es-modules/require-esm-in-cjs-cache/instrument-sync.js b/test/fixtures/es-modules/require-esm-in-cjs-cache/instrument-sync.js new file mode 100644 index 00000000000000..99d3a6608e028e --- /dev/null +++ b/test/fixtures/es-modules/require-esm-in-cjs-cache/instrument-sync.js @@ -0,0 +1,7 @@ +import * as mod from 'node:module'; + +mod.registerHooks({ + load(url, context, nextLoad) { + return nextLoad(url, context); + }, +}); diff --git a/test/fixtures/es-modules/require-esm-in-cjs-cache/instrument.js b/test/fixtures/es-modules/require-esm-in-cjs-cache/instrument.js new file mode 100644 index 00000000000000..1c99b7e9c759c9 --- /dev/null +++ b/test/fixtures/es-modules/require-esm-in-cjs-cache/instrument.js @@ -0,0 +1,3 @@ +import * as mod from 'node:module'; + +mod.register(new URL('hooks.js', import.meta.url).toString()); diff --git a/test/fixtures/es-modules/require-esm-in-cjs-cache/package.json b/test/fixtures/es-modules/require-esm-in-cjs-cache/package.json new file mode 100644 index 00000000000000..3dbc1ca591c055 --- /dev/null +++ b/test/fixtures/es-modules/require-esm-in-cjs-cache/package.json @@ -0,0 +1,3 @@ +{ + "type": "module" +} diff --git a/test/fixtures/es-modules/require-esm-in-cjs-cache/test.js b/test/fixtures/es-modules/require-esm-in-cjs-cache/test.js new file mode 100644 index 00000000000000..64dc1e8aaabff4 --- /dev/null +++ b/test/fixtures/es-modules/require-esm-in-cjs-cache/test.js @@ -0,0 +1 @@ +export default { hello: 'world' }; From 041e97757bde5080d4c09b181327f08874c2fba5 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 17 Sep 2025 15:13:59 +0200 Subject: [PATCH 2/7] module: only put directly require-d ESM into require.cache This reduces the impact of https://redirect.github.com/nodejs/node/pull/59679 by delaying the require.cache population of ESM until they are directly required. After that, it's necessary for them to be in the cache to maintain correctness. PR-URL: https://github.com/nodejs/node/pull/59874 Refs: https://github.com/nodejs/node/issues/59868 Reviewed-By: James M Snell Reviewed-By: Geoffrey Booth --- lib/internal/modules/esm/loader.js | 14 ---------- lib/internal/modules/esm/translators.js | 19 +++++++++++--- .../es-module/test-esm-in-require-cache-2.mjs | 26 +++++++++++++++++++ test/es-module/test-esm-in-require-cache.js | 25 ++++++++++++++++++ .../es-modules/esm-in-require-cache/esm.mjs | 1 + .../esm-in-require-cache/import-esm.mjs | 1 + .../import-require-esm.mjs | 1 + .../esm-in-require-cache/require-esm.cjs | 9 +++++++ .../require-import-esm.cjs | 1 + 9 files changed, 80 insertions(+), 17 deletions(-) create mode 100644 test/es-module/test-esm-in-require-cache-2.mjs create mode 100644 test/es-module/test-esm-in-require-cache.js create mode 100644 test/fixtures/es-modules/esm-in-require-cache/esm.mjs create mode 100644 test/fixtures/es-modules/esm-in-require-cache/import-esm.mjs create mode 100644 test/fixtures/es-modules/esm-in-require-cache/import-require-esm.mjs create mode 100644 test/fixtures/es-modules/esm-in-require-cache/require-esm.cjs create mode 100644 test/fixtures/es-modules/esm-in-require-cache/require-import-esm.cjs diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 8da8807baebf04..00ad25e28f86de 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -17,7 +17,6 @@ const { const { kIsExecuting, kRequiredModuleSymbol, - Module: CJSModule, } = require('internal/modules/cjs/loader'); const { imported_cjs_symbol } = internalBinding('symbols'); @@ -549,19 +548,6 @@ class ModuleLoader { throw new ERR_UNKNOWN_MODULE_FORMAT(format, url); } - // Populate the CJS cache with a facade for ESM in case subsequent require(esm) is - // looking it up from the cache. The parent module of the CJS cache entry would be the - // first CJS module that loads it with require(). This is an approximation, because - // ESM caches more and it does not get re-loaded and updated every time an `import` is - // encountered, unlike CJS require(), and we only use the parent entry to provide - // more information in error messages. - if (format === 'module') { - const parentFilename = urlToFilename(parentURL); - const parent = parentFilename ? CJSModule._cache[parentFilename] : undefined; - const cjsModule = lazyLoadTranslators().cjsEmplaceModuleCacheEntryForURL(url, parent); - debug('cjsEmplaceModuleCacheEntryForURL', url, parent, cjsModule); - } - const result = FunctionPrototypeCall(translator, this, url, source, parentURL === undefined); assert(result instanceof ModuleWrap); return result; diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index c0cb6ef091708e..738be93a14b608 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -153,9 +153,22 @@ function loadCJSModule(module, source, url, filename, isMain) { // requires a separate cache to be populated as well as introducing several quirks. This is not ideal. const job = cascadedLoader.getModuleJobForRequireInImportedCJS(specifier, url, importAttributes); job.runSync(); - const mod = cjsCache.get(job.url); - assert(mod, `Imported CJS module ${url} failed to load module ${job.url} using require()`); - if (!job.module.synthetic && !mod.loaded) { + let mod = cjsCache.get(job.url); + assert(job.module, `Imported CJS module ${url} failed to load module ${job.url} using require() due to race condition`); + + if (job.module.synthetic) { + assert(mod, `Imported CJS module ${url} failed to load module ${job.url} using require() due to missed cache`); + return mod.exports; + } + + // The module being required is a source text module. + if (!mod) { + mod = cjsEmplaceModuleCacheEntry(job.url); + cjsCache.set(job.url, mod); + } + // The module has been cached by the re-invented require. Update the exports object + // from the namespace object and return the evaluated exports. + if (!mod.loaded) { debug('populateCJSExportsFromESM from require(esm) in imported CJS', url, mod, job.module); populateCJSExportsFromESM(mod, job.module, job.module.getNamespace()); mod.loaded = true; diff --git a/test/es-module/test-esm-in-require-cache-2.mjs b/test/es-module/test-esm-in-require-cache-2.mjs new file mode 100644 index 00000000000000..b751883029b934 --- /dev/null +++ b/test/es-module/test-esm-in-require-cache-2.mjs @@ -0,0 +1,26 @@ +// This tests the behavior of ESM in require.cache when it's loaded from import. + +import '../common/index.mjs'; +import assert from 'node:assert'; +import * as fixtures from '../common/fixtures.mjs'; +const filename = fixtures.path('es-modules', 'esm-in-require-cache', 'esm.mjs'); +import { Module } from 'node:module'; + +// Imported ESM should not be in the require cache. +let { name } = await import('../fixtures/es-modules/esm-in-require-cache/import-esm.mjs'); +assert.strictEqual(name, 'esm'); +assert(!Module._cache[filename]); + +({ name } = await import('../fixtures/es-modules/esm-in-require-cache/esm.mjs')); +assert.strictEqual(name, 'esm'); +assert(!Module._cache[filename]); + +// Requiring ESM indirectly should not put it in the cache. +({ name } = await import('../fixtures/es-modules/esm-in-require-cache/require-import-esm.cjs')); +assert.strictEqual(name, 'esm'); +assert(!Module._cache[filename]); + +// After being required directly, it should be in the cache. +({ name } = await import('../fixtures/es-modules/esm-in-require-cache/import-require-esm.mjs')); +assert.strictEqual(name, 'esm'); +assert(Module._cache[filename]); diff --git a/test/es-module/test-esm-in-require-cache.js b/test/es-module/test-esm-in-require-cache.js new file mode 100644 index 00000000000000..d3365ce14f80b6 --- /dev/null +++ b/test/es-module/test-esm-in-require-cache.js @@ -0,0 +1,25 @@ +// This tests the behavior of ESM in require.cache when it's loaded from require. +'use strict'; +require('../common'); +const assert = require('node:assert'); +const fixtures = require('../common/fixtures'); +const filename = fixtures.path('es-modules', 'esm-in-require-cache', 'esm.mjs'); + +// Requiring ESM indirectly should not put it in the cache. +let { name } = require('../fixtures/es-modules/esm-in-require-cache/import-esm.mjs'); +assert.strictEqual(name, 'esm'); +assert(!require.cache[filename]); + +({ name } = require('../fixtures/es-modules/esm-in-require-cache/require-import-esm.cjs')); +assert.strictEqual(name, 'esm'); +assert(!require.cache[filename]); + +// After being required directly, it should be in the cache. +({ name } = require('../fixtures/es-modules/esm-in-require-cache/esm.mjs')); +assert.strictEqual(name, 'esm'); +assert(require.cache[filename]); +delete require.cache[filename]; + +({ name } = require('../fixtures/es-modules/esm-in-require-cache/import-require-esm.mjs')); +assert.strictEqual(name, 'esm'); +assert(require.cache[filename]); diff --git a/test/fixtures/es-modules/esm-in-require-cache/esm.mjs b/test/fixtures/es-modules/esm-in-require-cache/esm.mjs new file mode 100644 index 00000000000000..787bbe86d3e771 --- /dev/null +++ b/test/fixtures/es-modules/esm-in-require-cache/esm.mjs @@ -0,0 +1 @@ +export const name = 'esm'; diff --git a/test/fixtures/es-modules/esm-in-require-cache/import-esm.mjs b/test/fixtures/es-modules/esm-in-require-cache/import-esm.mjs new file mode 100644 index 00000000000000..b70b27fe1620b9 --- /dev/null +++ b/test/fixtures/es-modules/esm-in-require-cache/import-esm.mjs @@ -0,0 +1 @@ +export { name } from './esm.mjs'; diff --git a/test/fixtures/es-modules/esm-in-require-cache/import-require-esm.mjs b/test/fixtures/es-modules/esm-in-require-cache/import-require-esm.mjs new file mode 100644 index 00000000000000..1d8e14adb2f5ed --- /dev/null +++ b/test/fixtures/es-modules/esm-in-require-cache/import-require-esm.mjs @@ -0,0 +1 @@ +export { name, cache } from './require-esm.cjs' diff --git a/test/fixtures/es-modules/esm-in-require-cache/require-esm.cjs b/test/fixtures/es-modules/esm-in-require-cache/require-esm.cjs new file mode 100644 index 00000000000000..52ead05a698255 --- /dev/null +++ b/test/fixtures/es-modules/esm-in-require-cache/require-esm.cjs @@ -0,0 +1,9 @@ +const path = require('path'); + +const name = require('./esm.mjs').name; +exports.name = name; + +const filename = path.join(__dirname, 'esm.mjs'); +const cache = require.cache[filename]; +exports.cache = require.cache[filename]; + diff --git a/test/fixtures/es-modules/esm-in-require-cache/require-import-esm.cjs b/test/fixtures/es-modules/esm-in-require-cache/require-import-esm.cjs new file mode 100644 index 00000000000000..ddf16383f7a4df --- /dev/null +++ b/test/fixtures/es-modules/esm-in-require-cache/require-import-esm.cjs @@ -0,0 +1 @@ +exports.name = require('./import-esm.mjs').name; From 4aff83ce5d74ed97b35daaa54cd0ea47dd3bbe82 Mon Sep 17 00:00:00 2001 From: Marco Ippolito Date: Thu, 2 Oct 2025 05:49:26 +0200 Subject: [PATCH 3/7] module: use sync cjs when importing cts PR-URL: https://github.com/nodejs/node/pull/60072 Fixes: https://github.com/nodejs/node/issues/59963 Reviewed-By: Yongsheng Zhang Reviewed-By: Geoffrey Booth --- lib/internal/modules/esm/loader.js | 4 +++- test/es-module/test-typescript-commonjs.mjs | 10 ++++++++++ test/es-module/test-typescript-module.mjs | 11 +++++++++++ test/fixtures/typescript/cts/issue-59963/a.cts | 3 +++ test/fixtures/typescript/cts/issue-59963/b.mts | 2 ++ test/fixtures/typescript/cts/issue-59963/c.cts | 2 ++ test/fixtures/typescript/mts/issue-59963/a.mts | 3 +++ test/fixtures/typescript/mts/issue-59963/b.cts | 3 +++ test/fixtures/typescript/mts/issue-59963/c.mts | 1 + 9 files changed, 38 insertions(+), 1 deletion(-) create mode 100644 test/fixtures/typescript/cts/issue-59963/a.cts create mode 100644 test/fixtures/typescript/cts/issue-59963/b.mts create mode 100644 test/fixtures/typescript/cts/issue-59963/c.cts create mode 100644 test/fixtures/typescript/mts/issue-59963/a.mts create mode 100644 test/fixtures/typescript/mts/issue-59963/b.cts create mode 100644 test/fixtures/typescript/mts/issue-59963/c.mts diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 00ad25e28f86de..b8f3aaf0a3bca9 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -493,7 +493,9 @@ class ModuleLoader { const loadResult = this.#loadSync(url, { format, importAttributes }); // Use the synchronous commonjs translator which can deal with cycles. - const finalFormat = loadResult.format === 'commonjs' ? 'commonjs-sync' : loadResult.format; + const finalFormat = + loadResult.format === 'commonjs' || + loadResult.format === 'commonjs-typescript' ? 'commonjs-sync' : loadResult.format; if (finalFormat === 'wasm') { assert.fail('WASM is currently unsupported by require(esm)'); diff --git a/test/es-module/test-typescript-commonjs.mjs b/test/es-module/test-typescript-commonjs.mjs index 90978015de8924..c7f323208ff896 100644 --- a/test/es-module/test-typescript-commonjs.mjs +++ b/test/es-module/test-typescript-commonjs.mjs @@ -186,3 +186,13 @@ test('expect failure of a .cts file requiring esm in node_modules', async () => match(result.stderr, /ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING/); strictEqual(result.code, 1); }); + +test('cts -> require mts -> import cts', async () => { + const result = await spawnPromisified(process.execPath, [ + fixtures.path('typescript/cts/issue-59963/a.cts'), + ]); + + strictEqual(result.stderr, ''); + strictEqual(result.stdout, 'Hello from c.cts\n'); + strictEqual(result.code, 0); +}); diff --git a/test/es-module/test-typescript-module.mjs b/test/es-module/test-typescript-module.mjs index 1aae0cde864b8b..5b3327306bc7f2 100644 --- a/test/es-module/test-typescript-module.mjs +++ b/test/es-module/test-typescript-module.mjs @@ -133,3 +133,14 @@ test('execute .ts file importing a module', async () => { strictEqual(result.stdout, 'Hello, TypeScript!\n'); strictEqual(result.code, 0); }); + +test('mts -> import cts -> require mts', async () => { + const result = await spawnPromisified(process.execPath, [ + '--no-warnings', + fixtures.path('typescript/mts/issue-59963/a.mts'), + ]); + + strictEqual(result.stderr, ''); + strictEqual(result.stdout, 'Hello from c.mts\n'); + strictEqual(result.code, 0); +}); diff --git a/test/fixtures/typescript/cts/issue-59963/a.cts b/test/fixtures/typescript/cts/issue-59963/a.cts new file mode 100644 index 00000000000000..8826591bd39f02 --- /dev/null +++ b/test/fixtures/typescript/cts/issue-59963/a.cts @@ -0,0 +1,3 @@ +const { message } = require("./b.mts"); +interface Foo {}; +console.log(message); diff --git a/test/fixtures/typescript/cts/issue-59963/b.mts b/test/fixtures/typescript/cts/issue-59963/b.mts new file mode 100644 index 00000000000000..bd3cf1998f9b65 --- /dev/null +++ b/test/fixtures/typescript/cts/issue-59963/b.mts @@ -0,0 +1,2 @@ +interface Foo {}; +export { message } from "./c.cts"; diff --git a/test/fixtures/typescript/cts/issue-59963/c.cts b/test/fixtures/typescript/cts/issue-59963/c.cts new file mode 100644 index 00000000000000..20dc004237b945 --- /dev/null +++ b/test/fixtures/typescript/cts/issue-59963/c.cts @@ -0,0 +1,2 @@ +const message: string = "Hello from c.cts"; +module.exports = { message }; diff --git a/test/fixtures/typescript/mts/issue-59963/a.mts b/test/fixtures/typescript/mts/issue-59963/a.mts new file mode 100644 index 00000000000000..99c938acf5e9b8 --- /dev/null +++ b/test/fixtures/typescript/mts/issue-59963/a.mts @@ -0,0 +1,3 @@ +import { message } from "./b.cts"; +interface Foo {}; +console.log(message); diff --git a/test/fixtures/typescript/mts/issue-59963/b.cts b/test/fixtures/typescript/mts/issue-59963/b.cts new file mode 100644 index 00000000000000..75b724f21c4b3e --- /dev/null +++ b/test/fixtures/typescript/mts/issue-59963/b.cts @@ -0,0 +1,3 @@ +const { message } = require("./c.mts"); +interface Foo {}; +module.exports = { message }; diff --git a/test/fixtures/typescript/mts/issue-59963/c.mts b/test/fixtures/typescript/mts/issue-59963/c.mts new file mode 100644 index 00000000000000..4e7f88ba124584 --- /dev/null +++ b/test/fixtures/typescript/mts/issue-59963/c.mts @@ -0,0 +1 @@ +export const message: string = "Hello from c.mts"; From 63468af32a5549735e4596ef6b5f9eae8c9f4ac9 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 14 Oct 2025 18:28:22 +0200 Subject: [PATCH 4/7] module: handle null source from async loader hooks in sync hooks This relaxes the validation in sync hooks so that it accepts the quirky nullish source returned by the default step of the async loader when the module being loaded is CommonJS. When there are no customization hooks registered, a saner synchronous default load step is used to use a property instead of a reset nullish source to signify that the module should go through the CJS monkey patching routes and reduce excessive reloading from disk. PR-URL: https://github.com/nodejs/node/pull/59929 Fixes: https://github.com/nodejs/node/issues/59384 Fixes: https://github.com/nodejs/node/issues/57327 Refs: https://github.com/nodejs/node/issues/59666 Refs: https://github.com/dygabo/load_module_test Reviewed-By: Geoffrey Booth Reviewed-By: Jacob Smith --- lib/internal/modules/cjs/loader.js | 8 +- lib/internal/modules/customization_hooks.js | 44 ++++-- lib/internal/modules/esm/hooks.js | 2 +- lib/internal/modules/esm/load.js | 22 ++- lib/internal/modules/esm/loader.js | 61 +++++---- lib/internal/modules/esm/translators.js | 125 +++++++++--------- test/es-module/test-esm-type-flag-errors.mjs | 49 ++++--- .../module-hooks/sync-and-async/app.js | 2 + .../sync-and-async/async-customize-loader.js | 10 ++ .../sync-and-async/async-customize.js | 3 + .../sync-and-async/async-forward-loader.js | 3 + .../sync-and-async/async-forward.js | 3 + .../sync-and-async/sync-customize.js | 14 ++ .../sync-and-async/sync-forward.js | 7 + .../test-module-hooks-load-async-and-sync.js | 32 +++++ 15 files changed, 258 insertions(+), 127 deletions(-) create mode 100644 test/fixtures/module-hooks/sync-and-async/app.js create mode 100644 test/fixtures/module-hooks/sync-and-async/async-customize-loader.js create mode 100644 test/fixtures/module-hooks/sync-and-async/async-customize.js create mode 100644 test/fixtures/module-hooks/sync-and-async/async-forward-loader.js create mode 100644 test/fixtures/module-hooks/sync-and-async/async-forward.js create mode 100644 test/fixtures/module-hooks/sync-and-async/sync-customize.js create mode 100644 test/fixtures/module-hooks/sync-and-async/sync-forward.js create mode 100644 test/module-hooks/test-module-hooks-load-async-and-sync.js diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index d40b074a11efa9..0f2beeea18a63c 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -177,6 +177,7 @@ const { registerHooks, resolveHooks, resolveWithHooks, + validateLoadStrict, } = require('internal/modules/customization_hooks'); const { stripTypeScriptModuleTypes } = require('internal/modules/typescript'); const packageJsonReader = require('internal/modules/package_json_reader'); @@ -1150,7 +1151,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, - getCjsConditionsArray(), getDefaultLoad(url, id)); + getCjsConditionsArray(), getDefaultLoad(url, id), validateLoadStrict); if (loadResult.format && loadResult.format !== 'builtin') { return undefined; // Format has been overridden, return undefined for the caller to continue loading. } @@ -1759,10 +1760,9 @@ function loadSource(mod, filename, formatFromNode) { mod[kURL] = convertCJSFilenameToURL(filename); } + const defaultLoad = getDefaultLoad(mod[kURL], filename); const loadResult = loadWithHooks(mod[kURL], mod[kFormat], /* importAttributes */ undefined, - getCjsConditionsArray(), - getDefaultLoad(mod[kURL], filename)); - + getCjsConditionsArray(), defaultLoad, validateLoadStrict); // Reset the module properties with load hook results. if (loadResult.format !== undefined) { mod[kFormat] = loadResult.format; diff --git a/lib/internal/modules/customization_hooks.js b/lib/internal/modules/customization_hooks.js index f085b4c570d23a..dca15c7b693ae1 100644 --- a/lib/internal/modules/customization_hooks.js +++ b/lib/internal/modules/customization_hooks.js @@ -262,13 +262,25 @@ function validateResolve(specifier, context, result) { */ /** - * Validate the result returned by a chain of resolve hook. + * Validate the result returned by a chain of load hook. * @param {string} url URL passed into the hooks. * @param {ModuleLoadContext} context Context passed into the hooks. * @param {ModuleLoadResult} result Result produced by load hooks. * @returns {ModuleLoadResult} */ -function validateLoad(url, context, result) { +function validateLoadStrict(url, context, result) { + validateSourceStrict(url, context, result); + validateFormat(url, context, result); + return result; +} + +function validateLoadSloppy(url, context, result) { + validateSourcePermissive(url, context, result); + validateFormat(url, context, result); + return result; +} + +function validateSourceStrict(url, context, result) { const { source, format } = result; // To align with module.register(), the load hooks are still invoked for // the builtins even though the default load step only provides null as source, @@ -276,7 +288,8 @@ function validateLoad(url, context, result) { if (!StringPrototypeStartsWith(url, 'node:') && typeof result.source !== 'string' && !isAnyArrayBuffer(source) && - !isArrayBufferView(source)) { + !isArrayBufferView(source) && + format !== 'addon') { throw new ERR_INVALID_RETURN_PROPERTY_VALUE( 'a string, an ArrayBuffer, or a TypedArray', 'load', @@ -284,7 +297,21 @@ function validateLoad(url, context, result) { source, ); } +} +function validateSourcePermissive(url, context, result) { + const { source, format } = result; + if (format === 'commonjs' && source == null) { + // Accommodate the quirk in defaultLoad used by asynchronous loader hooks + // which sets source to null for commonjs. + // See: https://github.com/nodejs/node/issues/57327#issuecomment-2701382020 + return; + } + validateSourceStrict(url, context, result); +} + +function validateFormat(url, context, result) { + const { format } = result; if (typeof format !== 'string' && format !== undefined) { throw new ERR_INVALID_RETURN_PROPERTY_VALUE( 'a string', @@ -293,12 +320,6 @@ function validateLoad(url, context, result) { format, ); } - - return { - __proto__: null, - format, - source, - }; } class ModuleResolveContext { @@ -338,9 +359,10 @@ let decoder; * @param {ImportAttributes|undefined} importAttributes * @param {string[]} conditions * @param {(url: string, context: ModuleLoadContext) => ModuleLoadResult} defaultLoad + * @param {(url: string, context: ModuleLoadContext, result: ModuleLoadResult) => ModuleLoadResult} validateLoad * @returns {ModuleLoadResult} */ -function loadWithHooks(url, originalFormat, importAttributes, conditions, defaultLoad) { +function loadWithHooks(url, originalFormat, importAttributes, conditions, defaultLoad, validateLoad) { debug('loadWithHooks', url, originalFormat); const context = new ModuleLoadContext(originalFormat, importAttributes, conditions); if (loadHooks.length === 0) { @@ -403,4 +425,6 @@ module.exports = { registerHooks, resolveHooks, resolveWithHooks, + validateLoadStrict, + validateLoadSloppy, }; diff --git a/lib/internal/modules/esm/hooks.js b/lib/internal/modules/esm/hooks.js index 4571922ed5a0e9..d4991ad0b6c7d4 100644 --- a/lib/internal/modules/esm/hooks.js +++ b/lib/internal/modules/esm/hooks.js @@ -61,7 +61,7 @@ const { SHARED_MEMORY_BYTE_LENGTH, WORKER_TO_MAIN_THREAD_NOTIFICATION, } = require('internal/modules/esm/shared_constants'); -let debug = require('internal/util/debuglog').debuglog('esm', (fn) => { +let debug = require('internal/util/debuglog').debuglog('async_loader_worker', (fn) => { debug = fn; }); let importMetaInitializer; diff --git a/lib/internal/modules/esm/load.js b/lib/internal/modules/esm/load.js index 307c3598055147..2fc344765d8161 100644 --- a/lib/internal/modules/esm/load.js +++ b/lib/internal/modules/esm/load.js @@ -145,15 +145,26 @@ function defaultLoadSync(url, context = kEmptyObject) { throwIfUnsupportedURLScheme(urlInstance, false); + let shouldBeReloadedByCJSLoader = false; if (urlInstance.protocol === 'node:') { source = null; - } else if (source == null) { - ({ responseURL, source } = getSourceSync(urlInstance, context)); - context.source = source; - } + format ??= 'builtin'; + } else if (format === 'addon') { + // Skip loading addon file content. It must be loaded with dlopen from file system. + source = null; + } else { + if (source == null) { + ({ responseURL, source } = getSourceSync(urlInstance, context)); + context = { __proto__: context, source }; + } - format ??= defaultGetFormat(urlInstance, context); + // 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); return { @@ -161,6 +172,7 @@ 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 b8f3aaf0a3bca9..1f2e7eeb4b6a16 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -53,8 +53,9 @@ const { resolveWithHooks, loadHooks, loadWithHooks, + validateLoadSloppy, } = require('internal/modules/customization_hooks'); -let defaultResolve, defaultLoad, defaultLoadSync, importMetaInitializer; +let defaultResolve, defaultLoadSync, importMetaInitializer; const { tracingChannel } = require('diagnostics_channel'); const onImport = tracingChannel('module.import'); @@ -144,6 +145,10 @@ let hooksProxy; * @typedef {ArrayBuffer|TypedArray|string} ModuleSource */ +/** + * @typedef {{ format: ModuleFormat, source: ModuleSource, translatorKey: string }} TranslateContext + */ + /** * This class covers the base machinery of module loading. To add custom * behavior you can pass a customizations object and this object will be @@ -492,18 +497,19 @@ class ModuleLoader { const loadResult = this.#loadSync(url, { format, importAttributes }); + const formatFromLoad = loadResult.format; // Use the synchronous commonjs translator which can deal with cycles. - const finalFormat = - loadResult.format === 'commonjs' || - loadResult.format === 'commonjs-typescript' ? 'commonjs-sync' : loadResult.format; + const translatorKey = (formatFromLoad === 'commonjs' || formatFromLoad === 'commonjs-typescript') ? + 'commonjs-sync' : formatFromLoad; - if (finalFormat === 'wasm') { + if (translatorKey === 'wasm') { assert.fail('WASM is currently unsupported by require(esm)'); } const { source } = loadResult; const isMain = (parentURL === undefined); - const wrap = this.#translate(url, finalFormat, source, parentURL); + const translateContext = { format: formatFromLoad, source, translatorKey, __proto__: null }; + const wrap = this.#translate(url, translateContext, parentURL); assert(wrap instanceof ModuleWrap, `Translator used for require(${url}) should not be async`); if (process.env.WATCH_REPORT_DEPENDENCIES && process.send) { @@ -512,7 +518,7 @@ class ModuleLoader { const cjsModule = wrap[imported_cjs_symbol]; if (cjsModule) { - assert(finalFormat === 'commonjs-sync'); + assert(translatorKey === 'commonjs-sync'); // Check if the ESM initiating import CJS is being required by the same CJS module. if (cjsModule?.[kIsExecuting]) { const parentFilename = urlToFilename(parentURL); @@ -536,22 +542,22 @@ class ModuleLoader { * Translate a loaded module source into a ModuleWrap. This is run synchronously, * but the translator may return the ModuleWrap in a Promise. * @param {string} url URL of the module to be translated. - * @param {string} format Format of the module to be translated. This is used to find - * matching translators. - * @param {ModuleSource} source Source of the module to be translated. - * @param {string|undefined} parentURL URL of the parent module. Undefined if it's the entry point. + * @param {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. * @returns {ModuleWrap} */ - #translate(url, format, source, parentURL) { + #translate(url, translateContext, parentURL) { + const { translatorKey, format } = translateContext; this.validateLoadResult(url, format); - const translator = getTranslators().get(format); + const translator = getTranslators().get(translatorKey); if (!translator) { - throw new ERR_UNKNOWN_MODULE_FORMAT(format, url); + throw new ERR_UNKNOWN_MODULE_FORMAT(translatorKey, url); } - const result = FunctionPrototypeCall(translator, this, url, source, parentURL === undefined); - assert(result instanceof ModuleWrap); + const result = FunctionPrototypeCall(translator, this, url, translateContext, parentURL); + assert(result instanceof ModuleWrap, `The ${format} module returned is not a ModuleWrap`); return result; } @@ -564,7 +570,8 @@ class ModuleLoader { * @returns {ModuleWrap} */ loadAndTranslateForRequireInImportedCJS(url, loadContext, parentURL) { - const { format: formatFromLoad, source } = this.#loadSync(url, loadContext); + const loadResult = this.#loadSync(url, loadContext); + const formatFromLoad = loadResult.format; if (formatFromLoad === 'wasm') { // require(wasm) is not supported. throw new ERR_UNKNOWN_MODULE_FORMAT(formatFromLoad, url); @@ -576,15 +583,16 @@ class ModuleLoader { } } - let finalFormat = formatFromLoad; + let translatorKey = formatFromLoad; if (formatFromLoad === 'commonjs') { - finalFormat = 'require-commonjs'; + translatorKey = 'require-commonjs'; } if (formatFromLoad === 'commonjs-typescript') { - finalFormat = 'require-commonjs-typescript'; + translatorKey = 'require-commonjs-typescript'; } - const wrap = this.#translate(url, finalFormat, source, parentURL); + const translateContext = { ...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; } @@ -599,8 +607,9 @@ class ModuleLoader { */ loadAndTranslate(url, loadContext, parentURL) { const maybePromise = this.load(url, loadContext); - const afterLoad = ({ format, source }) => { - return this.#translate(url, format, source, parentURL); + const afterLoad = (loadResult) => { + const translateContext = { ...loadResult, translatorKey: loadResult.format, __proto__: null }; + return this.#translate(url, translateContext, parentURL); }; if (isPromise(maybePromise)) { return maybePromise.then(afterLoad); @@ -818,8 +827,8 @@ class ModuleLoader { return this.#customizations.load(url, context); } - defaultLoad ??= require('internal/modules/esm/load').defaultLoad; - return defaultLoad(url, context); + defaultLoadSync ??= require('internal/modules/esm/load').defaultLoadSync; + return defaultLoadSync(url, context); } /** @@ -854,7 +863,7 @@ class ModuleLoader { // TODO(joyeecheung): construct the ModuleLoadContext in the loaders directly instead // of converting them from plain objects in the hooks. return loadWithHooks(url, context.format, context.importAttributes, this.#defaultConditions, - this.#loadAndMaybeBlockOnLoaderThread.bind(this)); + this.#loadAndMaybeBlockOnLoaderThread.bind(this), validateLoadSloppy); } return this.#loadAndMaybeBlockOnLoaderThread(url, context); } diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index 738be93a14b608..2a7221b6d941e2 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -99,10 +99,12 @@ function errPath(url) { } // Strategy for loading a standard JavaScript module. -translators.set('module', function moduleStrategy(url, source, isMain) { +translators.set('module', function moduleStrategy(url, translateContext, parentURL) { + let { source } = translateContext; + const isMain = (parentURL === undefined); assertBufferSource(source, true, 'load'); source = stringify(source); - debug(`Translating StandardModule ${url}`); + debug(`Translating StandardModule ${url}`, translateContext); const { compileSourceTextModule } = require('internal/modules/esm/utils'); const context = isMain ? { isMain } : undefined; const module = compileSourceTextModule(url, source, this, context); @@ -198,20 +200,23 @@ const cjsCache = new SafeMap(); /** * Creates a ModuleWrap object for a CommonJS module. * @param {string} url - The URL of the module. - * @param {string} source - The source code of the module. - * @param {boolean} isMain - Whether the module is the main module. - * @param {string} format - Format of the module. - * @param {typeof loadCJSModule} [loadCJS=loadCJSModule] - The function to load the CommonJS module. + * @param {{ format: ModuleFormat, source: ModuleSource }} 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, source, isMain, format, loadCJS = loadCJSModule) { - debug(`Translating CJSModule ${url}`); +function createCJSModuleWrap(url, translateContext, parentURL, loadCJS = loadCJSModule) { + 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 ?? getSource(new URL(url)).source); - const { exportNames, module } = cjsPreparseModuleExports(filename, source, format); + const { exportNames, module } = cjsPreparseModuleExports(filename, source, sourceFormat); cjsCache.set(url, module); const namesWithDefault = exportNames.has('default') ? [...exportNames] : ['default', ...exportNames]; @@ -255,11 +260,12 @@ function createCJSModuleWrap(url, source, isMain, format, loadCJS = loadCJSModul /** * Creates a ModuleWrap object for a CommonJS module without source texts. * @param {string} url - The URL of the module. - * @param {boolean} isMain - Whether the module is the main module. + * @param {string|undefined} parentURL - URL of the parent module, if any. * @returns {ModuleWrap} The ModuleWrap object for the CommonJS module. */ -function createCJSNoSourceModuleWrap(url, isMain) { +function createCJSNoSourceModuleWrap(url, parentURL) { debug(`Translating CJSModule without source ${url}`); + const isMain = (parentURL === undefined); const filename = urlToFilename(url); @@ -293,54 +299,60 @@ function createCJSNoSourceModuleWrap(url, isMain) { }, module); } -translators.set('commonjs-sync', function requireCommonJS(url, source, isMain) { +translators.set('commonjs-sync', function requireCommonJS(url, translateContext, parentURL) { initCJSParseSync(); - return createCJSModuleWrap(url, source, isMain, 'commonjs', (module, source, url, filename, isMain) => { - assert(module === CJSModule._cache[filename]); - wrapModuleLoad(filename, null, isMain); - }); + return createCJSModuleWrap(url, translateContext, parentURL, loadCJSModuleWithModuleLoad); }); // Handle CommonJS modules referenced by `require` calls. // This translator function must be sync, as `require` is sync. -translators.set('require-commonjs', (url, source, isMain) => { +translators.set('require-commonjs', (url, translateContext, parentURL) => { initCJSParseSync(); assert(cjsParse); - return createCJSModuleWrap(url, source, isMain, 'commonjs'); + return createCJSModuleWrap(url, translateContext, parentURL); }); // Handle CommonJS modules referenced by `require` calls. // This translator function must be sync, as `require` is sync. -translators.set('require-commonjs-typescript', (url, source, isMain) => { +translators.set('require-commonjs-typescript', (url, translateContext, parentURL) => { assert(cjsParse); - const code = stripTypeScriptModuleTypes(stringify(source), url); - return createCJSModuleWrap(url, code, isMain, 'commonjs-typescript'); + 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) { + assert(module === CJSModule._cache[filename]); + wrapModuleLoad(filename, undefined, isMain); +} + // 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, source, isMain) { +translators.set('commonjs', function commonjsStrategy(url, translateContext, parentURL) { if (!cjsParse) { initCJSParseSync(); } // For backward-compatibility, it's possible to return a nullish value for - // CJS source associated with a file: URL. In this case, the source is - // obtained by calling the monkey-patchable CJS loader. - const cjsLoader = source == null ? (module, source, url, filename, isMain) => { - assert(module === CJSModule._cache[filename]); - wrapModuleLoad(filename, undefined, isMain); - } : loadCJSModule; + // 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. - source ??= readFileSync(new URL(url), 'utf8'); + translateContext.source ??= readFileSync(new URL(url), 'utf8'); } catch { // Continue regardless of error. } - return createCJSModuleWrap(url, source, isMain, 'commonjs', cjsLoader); + return createCJSModuleWrap(url, translateContext, parentURL, cjsLoader); }); /** @@ -365,22 +377,6 @@ function cjsEmplaceModuleCacheEntry(filename, parent) { return cjsMod; } -/** - * Emplace a CJS module cache entry for the given URL. - * @param {string} url The module URL - * @param {CJSModule} parent The parent CJS module - * @returns {CJSModule|undefined} the cached CJS module entry, undefined if url cannot be used to identify a CJS entry. - */ -exports.cjsEmplaceModuleCacheEntryForURL = function cjsEmplaceModuleCacheEntryForURL(url, parent) { - const filename = urlToFilename(url); - if (!filename) { - return; - } - const cjsModule = cjsEmplaceModuleCacheEntry(filename, parent); - cjsCache.set(url, cjsModule); - return cjsModule; -}; - /** * Pre-parses a CommonJS module's exports and re-exports. * @param {string} filename - The filename of the module. @@ -445,8 +441,8 @@ function cjsPreparseModuleExports(filename, source, format) { // Strategy for loading a node builtin CommonJS module that isn't // through normal resolution -translators.set('builtin', function builtinStrategy(url) { - debug(`Translating BuiltinModule ${url}`); +translators.set('builtin', function builtinStrategy(url, translateContext) { + debug(`Translating BuiltinModule ${url}`, translateContext); // Slice 'node:' scheme const id = StringPrototypeSlice(url, 5); const module = loadBuiltinModule(id, url); @@ -459,7 +455,8 @@ translators.set('builtin', function builtinStrategy(url) { }); // Strategy for loading a JSON file -translators.set('json', function jsonStrategy(url, source) { +translators.set('json', function jsonStrategy(url, translateContext) { + let { source } = translateContext; assertBufferSource(source, true, 'load'); debug(`Loading JSONModule ${url}`); const pathname = StringPrototypeStartsWith(url, 'file:') ? @@ -527,10 +524,11 @@ translators.set('json', function jsonStrategy(url, source) { * >} [[Instance]] slot proxy for WebAssembly Module Record */ const wasmInstances = new SafeWeakMap(); -translators.set('wasm', function(url, source) { +translators.set('wasm', function(url, translateContext) { + const { source } = translateContext; assertBufferSource(source, false, 'load'); - debug(`Translating WASMModule ${url}`); + debug(`Translating WASMModule ${url}`, translateContext); let compiled; try { @@ -615,9 +613,10 @@ translators.set('wasm', function(url, source) { }); // Strategy for loading a addon -translators.set('addon', function translateAddon(url, source, isMain) { +translators.set('addon', function translateAddon(url, translateContext, parentURL) { emitExperimentalWarning('Importing addons'); + const { source } = translateContext; // The addon must be loaded from file system with dlopen. Assert // the source is null. if (source !== null) { @@ -628,23 +627,25 @@ translators.set('addon', function translateAddon(url, source, isMain) { source); } - debug(`Translating addon ${url}`); + debug(`Translating addon ${url}`, translateContext); - return createCJSNoSourceModuleWrap(url, isMain); + return createCJSNoSourceModuleWrap(url, parentURL); }); // Strategy for loading a commonjs TypeScript module -translators.set('commonjs-typescript', function(url, source, isMain) { +translators.set('commonjs-typescript', function(url, translateContext, parentURL) { + const { source } = translateContext; assertBufferSource(source, true, 'load'); - const code = stripTypeScriptModuleTypes(stringify(source), url); - debug(`Translating TypeScript ${url}`); - return FunctionPrototypeCall(translators.get('commonjs'), this, url, code, isMain); + debug(`Translating TypeScript ${url}`, translateContext); + translateContext.source = stripTypeScriptModuleTypes(stringify(source), url); + return FunctionPrototypeCall(translators.get('commonjs'), this, url, translateContext, parentURL); }); // Strategy for loading an esm TypeScript module -translators.set('module-typescript', function(url, source, isMain) { +translators.set('module-typescript', function(url, translateContext, parentURL) { + const { source } = translateContext; assertBufferSource(source, true, 'load'); - const code = stripTypeScriptModuleTypes(stringify(source), url); - debug(`Translating TypeScript ${url}`); - return FunctionPrototypeCall(translators.get('module'), this, url, code, isMain); + debug(`Translating TypeScript ${url}`, translateContext); + translateContext.source = stripTypeScriptModuleTypes(stringify(source), url); + return FunctionPrototypeCall(translators.get('module'), this, url, translateContext, parentURL); }); diff --git a/test/es-module/test-esm-type-flag-errors.mjs b/test/es-module/test-esm-type-flag-errors.mjs index 2f7a1db35d2423..fbef806b9079c8 100644 --- a/test/es-module/test-esm-type-flag-errors.mjs +++ b/test/es-module/test-esm-type-flag-errors.mjs @@ -36,14 +36,16 @@ describe('--experimental-default-type=module', { concurrency: !process.env.TEST_ const result = await spawnPromisified(process.execPath, [ '--experimental-default-type=module', fixtures.path('es-modules/package-type-commonjs/echo-require-cache.js'), - ]); - - deepStrictEqual(result, { - code: 0, - stderr: '', - stdout: 'undefined\n', - signal: null, + ], { + env: { + ...process.env, + NODE_DEBUG: 'esm', + } }); + match(result.stderr, /Translating CJSModule file.+echo-require-cache\.js/); + match(result.stdout, /Object: null prototype/); + strictEqual(result.code, 0); + strictEqual(result.signal, null); }); it('should affect .cjs files that are imported', async () => { @@ -51,25 +53,34 @@ describe('--experimental-default-type=module', { concurrency: !process.env.TEST_ '--experimental-default-type=module', '-e', `import ${JSON.stringify(fixtures.fileURL('es-module-require-cache/echo.cjs'))}`, - ]); - - deepStrictEqual(result, { - code: 0, - stderr: '', - stdout: 'undefined\n', - signal: null, + ], { + env: { + ...process.env, + NODE_DEBUG: 'esm', + } }); + + match(result.stderr, /Translating CJSModule file.+echo\.cjs/); + match(result.stdout, /Object: null prototype/); + strictEqual(result.code, 0); + strictEqual(result.signal, null); }); it('should affect entry point .cjs files (with no hooks)', async () => { - const { stderr, stdout, code } = await spawnPromisified(process.execPath, [ + const result = await spawnPromisified(process.execPath, [ '--experimental-default-type=module', fixtures.path('es-module-require-cache/echo.cjs'), - ]); + ], { + env: { + ...process.env, + NODE_DEBUG: 'esm', + } + }); - strictEqual(stderr, ''); - match(stdout, /^undefined\n$/); - strictEqual(code, 0); + match(result.stderr, /Translating CJSModule file.+echo\.cjs/); + match(result.stdout, /Object: null prototype/); + strictEqual(result.code, 0); + strictEqual(result.signal, null); }); it('should affect entry point .cjs files (when any hooks is registered)', async () => { diff --git a/test/fixtures/module-hooks/sync-and-async/app.js b/test/fixtures/module-hooks/sync-and-async/app.js new file mode 100644 index 00000000000000..c14477b70f7183 --- /dev/null +++ b/test/fixtures/module-hooks/sync-and-async/app.js @@ -0,0 +1,2 @@ +console.log('Hello world'); + diff --git a/test/fixtures/module-hooks/sync-and-async/async-customize-loader.js b/test/fixtures/module-hooks/sync-and-async/async-customize-loader.js new file mode 100644 index 00000000000000..ac961f0b97a3ab --- /dev/null +++ b/test/fixtures/module-hooks/sync-and-async/async-customize-loader.js @@ -0,0 +1,10 @@ +export async function load(url, context, nextLoad) { + if (url.endsWith('app.js')) { + return { + shortCircuit: true, + format: 'module', + source: 'console.log("customized by async hook");', + }; + } + return nextLoad(url, context); +} diff --git a/test/fixtures/module-hooks/sync-and-async/async-customize.js b/test/fixtures/module-hooks/sync-and-async/async-customize.js new file mode 100644 index 00000000000000..78bf7fc5564bd8 --- /dev/null +++ b/test/fixtures/module-hooks/sync-and-async/async-customize.js @@ -0,0 +1,3 @@ +import { register } from 'node:module'; + +register(new URL('async-customize-loader.js', import.meta.url)); diff --git a/test/fixtures/module-hooks/sync-and-async/async-forward-loader.js b/test/fixtures/module-hooks/sync-and-async/async-forward-loader.js new file mode 100644 index 00000000000000..4a1ced7e8dbfbf --- /dev/null +++ b/test/fixtures/module-hooks/sync-and-async/async-forward-loader.js @@ -0,0 +1,3 @@ +export async function load(url, context, nextLoad) { + return nextLoad(url, context); +} diff --git a/test/fixtures/module-hooks/sync-and-async/async-forward.js b/test/fixtures/module-hooks/sync-and-async/async-forward.js new file mode 100644 index 00000000000000..fd10aa6df8987c --- /dev/null +++ b/test/fixtures/module-hooks/sync-and-async/async-forward.js @@ -0,0 +1,3 @@ +import { register } from 'node:module'; + +register(new URL('async-forward-loader.js', import.meta.url)); diff --git a/test/fixtures/module-hooks/sync-and-async/sync-customize.js b/test/fixtures/module-hooks/sync-and-async/sync-customize.js new file mode 100644 index 00000000000000..90d63db1c7cc2e --- /dev/null +++ b/test/fixtures/module-hooks/sync-and-async/sync-customize.js @@ -0,0 +1,14 @@ +import { registerHooks } from 'node:module'; + +registerHooks({ + load(url, context, nextLoad) { + if (url.endsWith('app.js')) { + return { + shortCircuit: true, + format: 'module', + source: 'console.log("customized by sync hook")', + }; + } + return nextLoad(url, context); + }, +}); diff --git a/test/fixtures/module-hooks/sync-and-async/sync-forward.js b/test/fixtures/module-hooks/sync-and-async/sync-forward.js new file mode 100644 index 00000000000000..2688a240dc88f8 --- /dev/null +++ b/test/fixtures/module-hooks/sync-and-async/sync-forward.js @@ -0,0 +1,7 @@ +import { registerHooks } from 'node:module'; + +registerHooks({ + load(url, context, nextLoad) { + return nextLoad(url, context); + }, +}); diff --git a/test/module-hooks/test-module-hooks-load-async-and-sync.js b/test/module-hooks/test-module-hooks-load-async-and-sync.js new file mode 100644 index 00000000000000..75f4987942f93d --- /dev/null +++ b/test/module-hooks/test-module-hooks-load-async-and-sync.js @@ -0,0 +1,32 @@ +'use strict'; +// This tests that sync and async hooks can be mixed. + +require('../common'); +const { spawnSyncAndAssert } = require('../common/child_process'); +const fixtures = require('../common/fixtures'); + +const app = fixtures.path('module-hooks', 'sync-and-async', 'app.js'); + +const testCases = [ + // When mixing sync and async hooks, the sync ones always run first. + { preload: ['sync-customize', 'async-customize'], stdout: 'customized by sync hook' }, + { preload: ['async-customize', 'sync-customize'], stdout: 'customized by sync hook' }, + // It should still work when neither hook does any customization. + { preload: ['sync-forward', 'async-forward'], stdout: 'Hello world' }, + { preload: ['async-forward', 'sync-forward'], stdout: 'Hello world' }, + // It should work when only one hook is customizing. + { preload: ['sync-customize', 'async-forward'], stdout: 'customized by sync hook' }, + { preload: ['async-customize', 'sync-forward'], stdout: 'customized by async hook' }, +]; + + +for (const { preload, stdout } of testCases) { + const importArgs = []; + for (const p of preload) { + importArgs.push('--import', fixtures.fileURL(`module-hooks/sync-and-async/${p}.js`)); + } + spawnSyncAndAssert(process.execPath, [...importArgs, app], { + stdout, + trim: true, + }); +} From cf1e37d17b2903e286de81f98e03cc042ebceedf Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 22 Dec 2025 15:35:45 +0100 Subject: [PATCH 5/7] module: fix sync resolve hooks for require with node: prefixes Previously, when require()-ing builtins with the node: prefix, the sync resolve hooks were not properly invoked, and load hooks could not override the builtin's format. This fixes the handling and enables redirecting prefixed built-ins to on-disk files and overriding them with other module types via hooks. PR-URL: https://github.com/nodejs/node/pull/61088 Fixes: https://github.com/nodejs/node/issues/60005 Reviewed-By: Antoine du Hamel Reviewed-By: Marco Ippolito --- lib/internal/modules/cjs/loader.js | 122 ++++++++++++------ .../fixtures/module-hooks/redirected-zlib.mjs | 1 + ...le-hooks-load-builtin-override-commonjs.js | 37 ++++++ ...module-hooks-load-builtin-override-json.js | 37 ++++++ ...dule-hooks-load-builtin-override-module.js | 41 ++++++ ...lve-builtin-on-disk-require-with-prefix.js | 33 +++++ ...solve-load-builtin-override-both-prefix.js | 36 ++++++ ...ooks-resolve-load-builtin-override-both.js | 35 +++++ ...ks-resolve-load-builtin-redirect-prefix.js | 29 +++++ ...ule-hooks-resolve-load-builtin-redirect.js | 28 ++++ 10 files changed, 360 insertions(+), 39 deletions(-) create mode 100644 test/fixtures/module-hooks/redirected-zlib.mjs create mode 100644 test/module-hooks/test-module-hooks-load-builtin-override-commonjs.js create mode 100644 test/module-hooks/test-module-hooks-load-builtin-override-json.js create mode 100644 test/module-hooks/test-module-hooks-load-builtin-override-module.js create mode 100644 test/module-hooks/test-module-hooks-resolve-builtin-on-disk-require-with-prefix.js create mode 100644 test/module-hooks/test-module-hooks-resolve-load-builtin-override-both-prefix.js create mode 100644 test/module-hooks/test-module-hooks-resolve-load-builtin-override-both.js create mode 100644 test/module-hooks/test-module-hooks-resolve-load-builtin-redirect-prefix.js create mode 100644 test/module-hooks/test-module-hooks-resolve-load-builtin-redirect.js diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 0f2beeea18a63c..3cd822a1515974 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -1086,7 +1086,9 @@ function resolveForCJSWithHooks(specifier, parent, isMain) { filename = convertURLToCJSFilename(url); } - return { __proto__: null, url, format, filename, parentURL }; + const result = { __proto__: null, url, format, filename, parentURL }; + debug('resolveForCJSWithHooks', specifier, parent?.id, '->', result); + return result; } /** @@ -1143,24 +1145,29 @@ function getDefaultLoad(url, filename) { * @param {string} id The module ID (without the node: prefix) * @param {string} url The module URL (with the node: prefix) * @param {string} format Format from resolution. - * @returns {any} If there are no load hooks or the load hooks do not override the format of the - * builtin, load and return the exports of the builtin. Otherwise, return undefined. + * @returns {{builtinExports: any, resultFromHook: undefined|ModuleLoadResult}} If there are no load + * hooks or the load hooks do not override the format of the builtin, load and return the exports + * of the builtin module. Otherwise, return the loadResult for the caller to continue loading. */ function loadBuiltinWithHooks(id, url, format) { + let resultFromHook; if (loadHooks.length) { url ??= `node:${id}`; + debug('loadBuiltinWithHooks ', loadHooks.length, id, url, format); // TODO(joyeecheung): do we really want to invoke the load hook for the builtins? - const loadResult = loadWithHooks(url, format || 'builtin', /* importAttributes */ undefined, - getCjsConditionsArray(), getDefaultLoad(url, id), validateLoadStrict); - if (loadResult.format && loadResult.format !== 'builtin') { - return undefined; // Format has been overridden, return undefined for the caller to continue loading. + resultFromHook = loadWithHooks(url, format || 'builtin', /* importAttributes */ undefined, + getCjsConditionsArray(), getDefaultLoad(url, id), validateLoadStrict); + if (resultFromHook.format && resultFromHook.format !== 'builtin') { + debug('loadBuiltinWithHooks overriding module', id, url, resultFromHook); + // Format has been overridden, return result for the caller to continue loading. + return { builtinExports: undefined, resultFromHook }; } } // No hooks or the hooks have not overridden the format. Load it as a builtin module and return the // exports. const mod = loadBuiltinModule(id); - return mod.exports; + return { builtinExports: mod.exports, resultFromHook: undefined }; } /** @@ -1197,47 +1204,64 @@ Module._load = function(request, parent, isMain) { } } - const { url, format, filename } = resolveForCJSWithHooks(request, parent, isMain); + const resolveResult = resolveForCJSWithHooks(request, parent, isMain); + let { format } = resolveResult; + const { url, filename } = resolveResult; + let resultFromLoadHook; // For backwards compatibility, if the request itself starts with node:, load it before checking // Module._cache. Otherwise, load it after the check. - if (StringPrototypeStartsWith(request, 'node:')) { - const result = loadBuiltinWithHooks(filename, url, format); - if (result) { - return result; + // TODO(joyeecheung): a more sensible handling is probably, if there are hooks, always go through the hooks + // first before checking the cache. Otherwise, check the cache first, then proceed to default loading. + if (request === url && StringPrototypeStartsWith(request, 'node:')) { + const normalized = BuiltinModule.normalizeRequirableId(request); + if (normalized) { // It's a builtin module. + const { resultFromHook, builtinExports } = loadBuiltinWithHooks(normalized, url, format); + if (builtinExports) { + return builtinExports; + } + // The format of the builtin has been overridden by user hooks. Continue loading. + resultFromLoadHook = resultFromHook; + format = resultFromLoadHook.format; } - // The format of the builtin has been overridden by user hooks. Continue loading. } - const cachedModule = Module._cache[filename]; - if (cachedModule !== undefined) { - updateChildren(parent, cachedModule, true); - if (cachedModule.loaded) { - return cachedModule.exports; - } - // If it's not cached by the ESM loader, the loading request - // comes from required CJS, and we can consider it a circular - // dependency when it's cached. - if (!cachedModule[kIsCachedByESMLoader]) { - return getExportsForCircularRequire(cachedModule); - } - // If it's cached by the ESM loader as a way to indirectly pass - // the module in to avoid creating it twice, the loading request - // came from imported CJS. In that case use the kModuleCircularVisited - // to determine if it's loading or not. - if (cachedModule[kModuleCircularVisited]) { - return getExportsForCircularRequire(cachedModule); + // If load hooks overrides the format for a built-in, bypass the cache. + let cachedModule; + if (resultFromLoadHook === undefined) { + cachedModule = Module._cache[filename]; + debug('Module._load checking cache for', filename, !!cachedModule); + if (cachedModule !== undefined) { + updateChildren(parent, cachedModule, true); + if (cachedModule.loaded) { + return cachedModule.exports; + } + // If it's not cached by the ESM loader, the loading request + // comes from required CJS, and we can consider it a circular + // dependency when it's cached. + if (!cachedModule[kIsCachedByESMLoader]) { + return getExportsForCircularRequire(cachedModule); + } + // If it's cached by the ESM loader as a way to indirectly pass + // the module in to avoid creating it twice, the loading request + // came from imported CJS. In that case use the kModuleCircularVisited + // to determine if it's loading or not. + if (cachedModule[kModuleCircularVisited]) { + return getExportsForCircularRequire(cachedModule); + } + // This is an ESM loader created cache entry, mark it as visited and fallthrough to loading the module. + cachedModule[kModuleCircularVisited] = true; } - // This is an ESM loader created cache entry, mark it as visited and fallthrough to loading the module. - cachedModule[kModuleCircularVisited] = true; } - if (BuiltinModule.canBeRequiredWithoutScheme(filename)) { - const result = loadBuiltinWithHooks(filename, url, format); - if (result) { - return result; + if (resultFromLoadHook === undefined && BuiltinModule.canBeRequiredWithoutScheme(filename)) { + const { resultFromHook, builtinExports } = loadBuiltinWithHooks(filename, url, format); + if (builtinExports) { + return builtinExports; } // The format of the builtin has been overridden by user hooks. Continue loading. + resultFromLoadHook = resultFromHook; + format = resultFromLoadHook.format; } // Don't call updateChildren(), Module constructor already does. @@ -1252,6 +1276,9 @@ Module._load = function(request, parent, isMain) { } else { module[kIsMainSymbol] = false; } + if (resultFromLoadHook !== undefined) { + module[kModuleSource] = resultFromLoadHook.source; + } reportModuleToWatchMode(filename); Module._cache[filename] = module; @@ -1436,6 +1463,17 @@ function createEsmNotFoundErr(request, path) { return err; } +function getExtensionForFormat(format) { + switch (format) { + case 'addon': + return '.node'; + case 'json': + return '.json'; + default: + return '.js'; + } +} + /** * Given a file name, pass it to the proper extension handler. * @param {string} filename The `require` specifier @@ -1447,7 +1485,13 @@ Module.prototype.load = function(filename) { this.filename ??= filename; this.paths ??= Module._nodeModulePaths(path.dirname(filename)); - const extension = findLongestRegisteredExtension(filename); + // If the format is already overridden by hooks, convert that back to extension. + let extension; + if (this[kFormat] !== undefined) { + extension = getExtensionForFormat(this[kFormat]); + } else { + extension = findLongestRegisteredExtension(filename); + } Module._extensions[extension](this, filename); this.loaded = true; diff --git a/test/fixtures/module-hooks/redirected-zlib.mjs b/test/fixtures/module-hooks/redirected-zlib.mjs new file mode 100644 index 00000000000000..bdeb009362b686 --- /dev/null +++ b/test/fixtures/module-hooks/redirected-zlib.mjs @@ -0,0 +1 @@ +export const url = import.meta.url; diff --git a/test/module-hooks/test-module-hooks-load-builtin-override-commonjs.js b/test/module-hooks/test-module-hooks-load-builtin-override-commonjs.js new file mode 100644 index 00000000000000..3b592e76891522 --- /dev/null +++ b/test/module-hooks/test-module-hooks-load-builtin-override-commonjs.js @@ -0,0 +1,37 @@ +'use strict'; + +// This tests that load hooks can override the format of builtin modules +// to 'commonjs' format. +const common = require('../common'); +const assert = require('assert'); +const { registerHooks } = require('module'); + +// Pick a builtin that's unlikely to be loaded already - like zlib. +assert(!process.moduleLoadList.includes('NativeModule zlib')); + +const hook = registerHooks({ + load: common.mustCall(function load(url, context, nextLoad) { + // Only intercept zlib builtin + if (url === 'node:zlib') { + // Return a different format to override the builtin + return { + source: 'exports.custom_zlib = "overridden by load hook";', + format: 'commonjs', + shortCircuit: true, + }; + } + return nextLoad(url, context); + }, 2), // Called twice: once for 'zlib', once for 'node:zlib' +}); + +// Test: Load hook overrides builtin format to commonjs +const zlib = require('zlib'); +assert.strictEqual(zlib.custom_zlib, 'overridden by load hook'); +assert.strictEqual(typeof zlib.createGzip, 'undefined'); // Original zlib API should not be available + +// Test with node: prefix +const zlib2 = require('node:zlib'); +assert.strictEqual(zlib2.custom_zlib, 'overridden by load hook'); +assert.strictEqual(typeof zlib2.createGzip, 'undefined'); + +hook.deregister(); diff --git a/test/module-hooks/test-module-hooks-load-builtin-override-json.js b/test/module-hooks/test-module-hooks-load-builtin-override-json.js new file mode 100644 index 00000000000000..af23982ba7ade4 --- /dev/null +++ b/test/module-hooks/test-module-hooks-load-builtin-override-json.js @@ -0,0 +1,37 @@ +'use strict'; + +// This tests that load hooks can override the format of builtin modules +// to 'json' format. +const common = require('../common'); +const assert = require('assert'); +const { registerHooks } = require('module'); + +// Pick a builtin that's unlikely to be loaded already - like zlib. +assert(!process.moduleLoadList.includes('NativeModule zlib')); + +const hook = registerHooks({ + load: common.mustCall(function load(url, context, nextLoad) { + // Only intercept zlib builtin + if (url === 'node:zlib') { + // Return JSON format to override the builtin + return { + source: JSON.stringify({ custom_zlib: 'JSON overridden zlib' }), + format: 'json', + shortCircuit: true, + }; + } + return nextLoad(url, context); + }, 2), // Called twice: once for 'zlib', once for 'node:zlib' +}); + +// Test: Load hook overrides builtin format to json +const zlib = require('zlib'); +assert.strictEqual(zlib.custom_zlib, 'JSON overridden zlib'); +assert.strictEqual(typeof zlib.createGzip, 'undefined'); // Original zlib API should not be available + +// Test with node: prefix +const zlib2 = require('node:zlib'); +assert.strictEqual(zlib2.custom_zlib, 'JSON overridden zlib'); +assert.strictEqual(typeof zlib2.createGzip, 'undefined'); + +hook.deregister(); diff --git a/test/module-hooks/test-module-hooks-load-builtin-override-module.js b/test/module-hooks/test-module-hooks-load-builtin-override-module.js new file mode 100644 index 00000000000000..890990553e3667 --- /dev/null +++ b/test/module-hooks/test-module-hooks-load-builtin-override-module.js @@ -0,0 +1,41 @@ +'use strict'; + +// This tests that load hooks can override the format of builtin modules +// to 'module', and require() can load them. +const common = require('../common'); +const assert = require('assert'); +const { registerHooks } = require('module'); + +// Pick a builtin that's unlikely to be loaded already - like zlib. +assert(!process.moduleLoadList.includes('NativeModule zlib')); + +const hook = registerHooks({ + load: common.mustCall(function load(url, context, nextLoad) { + // Only intercept zlib builtin + if (url === 'node:zlib') { + // Return ES module format to override the builtin + // Note: For require() to work with ESM, we need to export 'module.exports' + return { + source: `const exports = { custom_zlib: "ESM overridden zlib" }; + export default exports; + export { exports as 'module.exports' };`, + format: 'module', + shortCircuit: true, + }; + } + return nextLoad(url, context); + }, 2), // Called twice: once for 'zlib', once for 'node:zlib' +}); + +// Test: Load hook overrides builtin format to module. +// With the 'module.exports' export, require() should work +const zlib = require('zlib'); +assert.strictEqual(zlib.custom_zlib, 'ESM overridden zlib'); +assert.strictEqual(typeof zlib.createGzip, 'undefined'); // Original zlib API should not be available + +// Test with node: prefix +const zlib2 = require('node:zlib'); +assert.strictEqual(zlib2.custom_zlib, 'ESM overridden zlib'); +assert.strictEqual(typeof zlib2.createGzip, 'undefined'); + +hook.deregister(); diff --git a/test/module-hooks/test-module-hooks-resolve-builtin-on-disk-require-with-prefix.js b/test/module-hooks/test-module-hooks-resolve-builtin-on-disk-require-with-prefix.js new file mode 100644 index 00000000000000..97afbbcbef8501 --- /dev/null +++ b/test/module-hooks/test-module-hooks-resolve-builtin-on-disk-require-with-prefix.js @@ -0,0 +1,33 @@ +'use strict'; + +// This tests that builtins can be redirected to a local file when they are prefixed +// with `node:`. +require('../common'); + +const assert = require('assert'); +const { registerHooks } = require('module'); +const fixtures = require('../common/fixtures'); + +// This tests that builtins can be redirected to a local file. +// Pick a builtin that's unlikely to be loaded already - like zlib. +assert(!process.moduleLoadList.includes('NativeModule zlib')); + +const hook = registerHooks({ + resolve(specifier, context, nextLoad) { + specifier = specifier.replaceAll('node:', ''); + return { + url: fixtures.fileURL('module-hooks', `redirected-${specifier}.js`).href, + shortCircuit: true, + }; + }, +}); + +// Check assert, which is already loaded. +// eslint-disable-next-line node-core/must-call-assert +assert.strictEqual(require('node:assert').exports_for_test, 'redirected assert'); +// Check zlib, which is not yet loaded. +assert.strictEqual(require('node:zlib').exports_for_test, 'redirected zlib'); +// Check fs, which is redirected to an ESM +assert.strictEqual(require('node:fs').exports_for_test, 'redirected fs'); + +hook.deregister(); diff --git a/test/module-hooks/test-module-hooks-resolve-load-builtin-override-both-prefix.js b/test/module-hooks/test-module-hooks-resolve-load-builtin-override-both-prefix.js new file mode 100644 index 00000000000000..769c9d218d8c3a --- /dev/null +++ b/test/module-hooks/test-module-hooks-resolve-load-builtin-override-both-prefix.js @@ -0,0 +1,36 @@ +'use strict'; + +// This tests the interaction between resolve and load hooks for builtins with the +// `node:` prefix. +const common = require('../common'); +const assert = require('assert'); +const { registerHooks } = require('module'); +const fixtures = require('../common/fixtures'); + +// Pick a builtin that's unlikely to be loaded already - like zlib. +assert(!process.moduleLoadList.includes('NativeModule zlib')); + +const redirectedURL = fixtures.fileURL('module-hooks/redirected-zlib.js').href; + +registerHooks({ + resolve: common.mustCall(function resolve(specifier, context, nextResolve) { + assert.strictEqual(specifier, 'node:zlib'); + return { + url: redirectedURL, + format: 'module', + shortCircuit: true, + }; + }), + + load: common.mustCall(function load(url, context, nextLoad) { + assert.strictEqual(url, redirectedURL); + return { + source: 'export const loadURL = import.meta.url;', + format: 'module', + shortCircuit: true, + }; + }), +}); + +const zlib = require('node:zlib'); +assert.strictEqual(zlib.loadURL, redirectedURL); diff --git a/test/module-hooks/test-module-hooks-resolve-load-builtin-override-both.js b/test/module-hooks/test-module-hooks-resolve-load-builtin-override-both.js new file mode 100644 index 00000000000000..68942a8541ac49 --- /dev/null +++ b/test/module-hooks/test-module-hooks-resolve-load-builtin-override-both.js @@ -0,0 +1,35 @@ +'use strict'; + +// This tests the interaction between resolve and load hooks for builtins. +const common = require('../common'); +const assert = require('assert'); +const { registerHooks } = require('module'); +const fixtures = require('../common/fixtures'); + +// Pick a builtin that's unlikely to be loaded already - like zlib. +assert(!process.moduleLoadList.includes('NativeModule zlib')); + +const redirectedURL = fixtures.fileURL('module-hooks/redirected-zlib.js').href; + +registerHooks({ + resolve: common.mustCall(function resolve(specifier, context, nextResolve) { + assert.strictEqual(specifier, 'zlib'); + return { + url: redirectedURL, + format: 'module', + shortCircuit: true, + }; + }), + + load: common.mustCall(function load(url, context, nextLoad) { + assert.strictEqual(url, redirectedURL); + return { + source: 'export const loadURL = import.meta.url;', + format: 'module', + shortCircuit: true, + }; + }), +}); + +const zlib = require('zlib'); +assert.strictEqual(zlib.loadURL, redirectedURL); diff --git a/test/module-hooks/test-module-hooks-resolve-load-builtin-redirect-prefix.js b/test/module-hooks/test-module-hooks-resolve-load-builtin-redirect-prefix.js new file mode 100644 index 00000000000000..7320ca33b5f96f --- /dev/null +++ b/test/module-hooks/test-module-hooks-resolve-load-builtin-redirect-prefix.js @@ -0,0 +1,29 @@ +'use strict'; + +// This tests the interaction between resolve and load hooks for builtins with the +// `node:` prefix. +const common = require('../common'); +const assert = require('assert'); +const { registerHooks } = require('module'); + +// Pick a builtin that's unlikely to be loaded already - like zlib or dns. +assert(!process.moduleLoadList.includes('NativeModule zlib')); +assert(!process.moduleLoadList.includes('NativeModule dns')); + +registerHooks({ + resolve: common.mustCall(function resolve(specifier, context, nextResolve) { + assert.strictEqual(specifier, 'node:dns'); + return { + url: 'node:zlib', + shortCircuit: true, + }; + }), + + load: common.mustCall(function load(url, context, nextLoad) { + assert.strictEqual(url, 'node:zlib'); + return nextLoad(url, context); + }), +}); + +const zlib = require('node:dns'); +assert.strictEqual(typeof zlib.createGzip, 'function'); diff --git a/test/module-hooks/test-module-hooks-resolve-load-builtin-redirect.js b/test/module-hooks/test-module-hooks-resolve-load-builtin-redirect.js new file mode 100644 index 00000000000000..f50fe42a4fd285 --- /dev/null +++ b/test/module-hooks/test-module-hooks-resolve-load-builtin-redirect.js @@ -0,0 +1,28 @@ +'use strict'; + +// This tests the interaction between resolve and load hooks for builtins. +const common = require('../common'); +const assert = require('assert'); +const { registerHooks } = require('module'); + +// Pick a builtin that's unlikely to be loaded already - like zlib or dns. +assert(!process.moduleLoadList.includes('NativeModule zlib')); +assert(!process.moduleLoadList.includes('NativeModule dns')); + +registerHooks({ + resolve: common.mustCall(function resolve(specifier, context, nextResolve) { + assert.strictEqual(specifier, 'dns'); + return { + url: 'node:zlib', + shortCircuit: true, + }; + }), + + load: common.mustCall(function load(url, context, nextLoad) { + assert.strictEqual(url, 'node:zlib'); + return nextLoad(url, context); + }), +}); + +const zlib = require('dns'); +assert.strictEqual(typeof zlib.createGzip, 'function'); From 6df0cdf234c43ce1670eab34c65aab630ec2e2ec Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 26 Jan 2026 12:20:53 +0100 Subject: [PATCH 6/7] module: do not wrap module._load when tracing is not enabled This prevents clobbering the stack traces with another internal frame and removes the unnecessary hoops from step-debugging. PR-URL: https://github.com/nodejs/node/pull/61479 Reviewed-By: Matteo Collina Reviewed-By: Marco Ippolito Reviewed-By: Luigi Pinca Reviewed-By: Stephen Belanger --- lib/internal/modules/cjs/loader.js | 16 ++++++---- test/fixtures/console/console.snapshot | 1 - test/fixtures/errors/force_colors.snapshot | 1 - ...promise_unhandled_warn_with_error.snapshot | 1 - .../unhandled_promise_trace_warnings.snapshot | 2 -- .../source_map_assert_source_line.snapshot | 1 - test/message/assert_throws_stack.out | 1 - test/message/internal_assert.out | 1 - test/message/internal_assert_fail.out | 1 - test/message/util-inspect-error-cause.out | 30 +++++++------------ test/message/util_inspect_error.out | 5 ---- test/pseudo-tty/console_colors.out | 4 --- test/pseudo-tty/test-fatal-error.out | 1 - test/pseudo-tty/test-start-trace-sigint.out | 1 - test/pseudo-tty/test-trace-sigint.out | 1 - 15 files changed, 21 insertions(+), 46 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 3cd822a1515974..d467e5afc07fee 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -237,15 +237,21 @@ let statCache = null; function wrapModuleLoad(request, parent, isMain) { const logLabel = `[${parent?.id || ''}] [${request}]`; const traceLabel = `require('${request}')`; + const channel = onRequire(); startTimer(logLabel, traceLabel); try { - return onRequire().traceSync(Module._load, { - __proto__: null, - parentFilename: parent?.filename, - id: request, - }, Module, request, parent, isMain); + if (channel.hasSubscribers) { + return onRequire().traceSync(Module._load, { + __proto__: null, + parentFilename: parent?.filename, + id: request, + }, Module, request, parent, isMain); + } + // No subscribers, skip the wrapping to avoid clobbering stack traces + // and debugging steps. + return Module._load(request, parent, isMain); } finally { endTimer(logLabel, traceLabel); } diff --git a/test/fixtures/console/console.snapshot b/test/fixtures/console/console.snapshot index 41e7d16fb993a6..4f1cb254811b6d 100644 --- a/test/fixtures/console/console.snapshot +++ b/test/fixtures/console/console.snapshot @@ -7,4 +7,3 @@ Trace: foo at * at * at * - at * diff --git a/test/fixtures/errors/force_colors.snapshot b/test/fixtures/errors/force_colors.snapshot index 93ac005e833ce6..a93eea815b3152 100644 --- a/test/fixtures/errors/force_colors.snapshot +++ b/test/fixtures/errors/force_colors.snapshot @@ -11,6 +11,5 @@ Error: Should include grayed stack trace  at *  at *  at * - at * Node.js * diff --git a/test/fixtures/errors/promise_unhandled_warn_with_error.snapshot b/test/fixtures/errors/promise_unhandled_warn_with_error.snapshot index 4b3ed8640dc221..2a2ec57dc1cfcd 100644 --- a/test/fixtures/errors/promise_unhandled_warn_with_error.snapshot +++ b/test/fixtures/errors/promise_unhandled_warn_with_error.snapshot @@ -7,6 +7,5 @@ at * at * at * - at * (Use `* --trace-warnings ...` to show where the warning was created) (node:*) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https:*nodejs.org*api*cli.html#cli_unhandled_rejections_mode). (rejection id: 1) diff --git a/test/fixtures/errors/unhandled_promise_trace_warnings.snapshot b/test/fixtures/errors/unhandled_promise_trace_warnings.snapshot index 246b82a8d5accb..94c709b17788c4 100644 --- a/test/fixtures/errors/unhandled_promise_trace_warnings.snapshot +++ b/test/fixtures/errors/unhandled_promise_trace_warnings.snapshot @@ -11,7 +11,6 @@ at * at * at * - at * (node:*) Error: This was rejected at * at * @@ -21,7 +20,6 @@ at * at * at * - at * (node:*) PromiseRejectionHandledWarning: Promise rejection was handled asynchronously (rejection id: 1) at * at * diff --git a/test/fixtures/source-map/output/source_map_assert_source_line.snapshot b/test/fixtures/source-map/output/source_map_assert_source_line.snapshot index 62e611f330f97f..d42bc01cf758cf 100644 --- a/test/fixtures/source-map/output/source_map_assert_source_line.snapshot +++ b/test/fixtures/source-map/output/source_map_assert_source_line.snapshot @@ -10,7 +10,6 @@ AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value: * * * - * generatedMessage: true, code: 'ERR_ASSERTION', actual: false, diff --git a/test/message/assert_throws_stack.out b/test/message/assert_throws_stack.out index 1ecda64889e07f..24c0bfd41af371 100644 --- a/test/message/assert_throws_stack.out +++ b/test/message/assert_throws_stack.out @@ -17,7 +17,6 @@ AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal: at * at * at * - at * at * { generatedMessage: true, code: 'ERR_ASSERTION', diff --git a/test/message/internal_assert.out b/test/message/internal_assert.out index 87aaab00370d00..e4f67f650f938e 100644 --- a/test/message/internal_assert.out +++ b/test/message/internal_assert.out @@ -13,7 +13,6 @@ Please open an issue with this stack trace at https://github.com/nodejs/node/iss at * at * at * - at * at * { code: 'ERR_INTERNAL_ASSERTION' } diff --git a/test/message/internal_assert_fail.out b/test/message/internal_assert_fail.out index 9fc86673262dba..6a5916401bb7f6 100644 --- a/test/message/internal_assert_fail.out +++ b/test/message/internal_assert_fail.out @@ -14,7 +14,6 @@ Please open an issue with this stack trace at https://github.com/nodejs/node/iss at * at * at * - at * at * { code: 'ERR_INTERNAL_ASSERTION' } diff --git a/test/message/util-inspect-error-cause.out b/test/message/util-inspect-error-cause.out index 6e2caf7f05102c..344ace1bc94074 100644 --- a/test/message/util-inspect-error-cause.out +++ b/test/message/util-inspect-error-cause.out @@ -6,7 +6,6 @@ Error: Number error cause at * at * at * - at * at * { [cause]: 42 } @@ -18,7 +17,6 @@ Error: Object cause at * at * at * - at * at * { [cause]: { message: 'Unique', @@ -35,7 +33,6 @@ Error: undefined cause at * at * at * - at * at * { [cause]: undefined } @@ -47,7 +44,6 @@ Error: cause that throws at * at * at * - at * at * { [cause]: [Getter] } @@ -57,7 +53,7 @@ RangeError: New Stack Frames [cause]: FoobarError: Individual message at * *[90m at *[39m - *[90m ... 6 lines matching cause stack trace ...*[39m + *[90m ... 5 lines matching cause stack trace ...*[39m *[90m at *[39m { status: *[32m'Feeling good'*[39m, extraProperties: *[32m'Yes!'*[39m, @@ -70,18 +66,17 @@ RangeError: New Stack Frames *[90m at *[39m *[90m at *[39m *[90m at *[39m - *[90m at *[39m } } Error: Stack causes at * *[90m at *[39m -*[90m ... 6 lines matching cause stack trace ...*[39m +*[90m ... 5 lines matching cause stack trace ...*[39m *[90m at *[39m { [cause]: FoobarError: Individual message at * *[90m at *[39m - *[90m ... 6 lines matching cause stack trace ...*[39m + *[90m ... 5 lines matching cause stack trace ...*[39m *[90m at *[39m { status: *[32m'Feeling good'*[39m, extraProperties: *[32m'Yes!'*[39m, @@ -94,7 +89,6 @@ Error: Stack causes *[90m at *[39m *[90m at *[39m *[90m at *[39m - *[90m at *[39m } } RangeError: New Stack Frames @@ -103,12 +97,12 @@ RangeError: New Stack Frames [cause]: Error: Stack causes at * *[90m at *[39m - *[90m ... 6 lines matching cause stack trace ...*[39m + *[90m ... 5 lines matching cause stack trace ...*[39m *[90m at *[39m { [cause]: FoobarError: Individual message at * *[90m at *[39m - *[90m ... 6 lines matching cause stack trace ...*[39m + *[90m ... 5 lines matching cause stack trace ...*[39m *[90m at *[39m { status: *[32m'Feeling good'*[39m, extraProperties: *[32m'Yes!'*[39m, @@ -121,7 +115,6 @@ RangeError: New Stack Frames *[90m at *[39m *[90m at *[39m *[90m at *[39m - *[90m at *[39m } } } @@ -131,7 +124,7 @@ RangeError: New Stack Frames [cause]: FoobarError: Individual message at * at * - ... 6 lines matching cause stack trace ... + ... 5 lines matching cause stack trace ... at * { status: 'Feeling good', extraProperties: 'Yes!', @@ -144,18 +137,17 @@ RangeError: New Stack Frames at * at * at * - at * } } Error: Stack causes at * at * - ... 6 lines matching cause stack trace ... + ... 5 lines matching cause stack trace ... at * { [cause]: FoobarError: Individual message at * at * - ... 6 lines matching cause stack trace ... + ... 5 lines matching cause stack trace ... at * status: 'Feeling good', extraProperties: 'Yes!', @@ -168,7 +160,6 @@ Error: Stack causes at * at * at * - at * } } RangeError: New Stack Frames @@ -177,12 +168,12 @@ RangeError: New Stack Frames [cause]: Error: Stack causes at * at * - ... 6 lines matching cause stack trace ... + ... 5 lines matching cause stack trace ... at * { [cause]: FoobarError: Individual message at * at * - ... 6 lines matching cause stack trace ... + ... 5 lines matching cause stack trace ... at * { status: 'Feeling good', extraProperties: 'Yes!', @@ -195,7 +186,6 @@ RangeError: New Stack Frames at * at * at * - at * } } } diff --git a/test/message/util_inspect_error.out b/test/message/util_inspect_error.out index 0af1853addeb44..31b65eb2e2bf3c 100644 --- a/test/message/util_inspect_error.out +++ b/test/message/util_inspect_error.out @@ -9,7 +9,6 @@ at * at * at * - at * nested: { err: Error: foo @@ -22,7 +21,6 @@ at * at * at * - at * { err: Error: foo bar @@ -34,7 +32,6 @@ at * at * at * - at * nested: { err: Error: foo bar @@ -46,7 +43,6 @@ at * at * at * - at * } } { Error: foo @@ -59,5 +55,4 @@ bar at * at * at * - at * foo: 'bar' } diff --git a/test/pseudo-tty/console_colors.out b/test/pseudo-tty/console_colors.out index 2aaa21fbb7f045..7d191fa63554b4 100644 --- a/test/pseudo-tty/console_colors.out +++ b/test/pseudo-tty/console_colors.out @@ -13,7 +13,6 @@ foobar [90m at *[39m [90m at *[39m [90m at *[39m -[90m at *[39m Error: Should not ever get here. at Object. [90m(*node_modules*[4m*node_modules*[24m*bar.js:*:*[90m)[39m @@ -23,7 +22,6 @@ Error: Should not ever get here. [90m at *[39m [90m at *[39m [90m at *[39m -[90m at *[39m [90m at *[39m at Object. [90m(*console_colors.js:*:*[90m)[39m [90m at *[39m @@ -33,7 +31,6 @@ Error: Should not ever get here. [90m at *[39m [90m at *[39m [90m at *[39m -[90m at *[39m Error at evalmachine.:*:* @@ -47,4 +44,3 @@ Error [90m at *[39m [90m at *[39m [90m at *[39m -[90m at *[39m diff --git a/test/pseudo-tty/test-fatal-error.out b/test/pseudo-tty/test-fatal-error.out index 7809e049fe0773..9fb8df4251a49b 100644 --- a/test/pseudo-tty/test-fatal-error.out +++ b/test/pseudo-tty/test-fatal-error.out @@ -10,7 +10,6 @@ TypeError: foobar [90m at *(node:internal*loader:*:*)[39m [90m at *[39m [90m at *[39m -[90m at *[39m [90m at *[39m { bla: [33mtrue[39m } diff --git a/test/pseudo-tty/test-start-trace-sigint.out b/test/pseudo-tty/test-start-trace-sigint.out index e5e4911f844080..02cd34e786516b 100644 --- a/test/pseudo-tty/test-start-trace-sigint.out +++ b/test/pseudo-tty/test-start-trace-sigint.out @@ -8,4 +8,3 @@ KEYBOARD_INTERRUPT: Script execution was interrupted by `SIGINT` at * at * at * - at * diff --git a/test/pseudo-tty/test-trace-sigint.out b/test/pseudo-tty/test-trace-sigint.out index 6efae6f612f05d..ae6063b74242e4 100644 --- a/test/pseudo-tty/test-trace-sigint.out +++ b/test/pseudo-tty/test-trace-sigint.out @@ -8,4 +8,3 @@ KEYBOARD_INTERRUPT: Script execution was interrupted by `SIGINT` at * at * at * - at * From 7d1b2170a40bde9d186ac4e412e3b27629633055 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 3 Feb 2026 00:04:23 +0100 Subject: [PATCH 7/7] module: do not invoke resolve hooks twice for imported cjs Previously the resolve hook can be invoked twice from the synthetic module evaluation step of imported CJS in the extra module._load() call that's invoked on the resolved full path. Add an option to avoid it, since the resolution and loading has already been done before. PR-URL: https://github.com/nodejs/node/pull/61529 Fixes: https://github.com/nodejs/node/issues/57125 Refs: https://github.com/nodejs/node/issues/55808 Refs: https://github.com/nodejs/node/issues/56241 Reviewed-By: Jacob Smith --- lib/internal/modules/cjs/loader.js | 19 ++++++++++-------- lib/internal/modules/esm/translators.js | 13 +++++++++--- test/fixtures/value.cjs | 1 + .../test-module-hooks-load-import-cjs.js | 20 +++++++++++++++++++ .../test-module-hooks-resolve-import-cjs.js | 18 +++++++++++++++++ 5 files changed, 60 insertions(+), 11 deletions(-) create mode 100644 test/fixtures/value.cjs create mode 100644 test/module-hooks/test-module-hooks-load-import-cjs.js create mode 100644 test/module-hooks/test-module-hooks-resolve-import-cjs.js diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index d467e5afc07fee..e70b465db82c66 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -234,7 +234,7 @@ let statCache = null; * * See more {@link Module._load} */ -function wrapModuleLoad(request, parent, isMain) { +function wrapModuleLoad(request, parent, isMain, options) { const logLabel = `[${parent?.id || ''}] [${request}]`; const traceLabel = `require('${request}')`; const channel = onRequire(); @@ -247,11 +247,11 @@ function wrapModuleLoad(request, parent, isMain) { __proto__: null, parentFilename: parent?.filename, id: request, - }, Module, request, parent, isMain); + }, Module, request, parent, isMain, options); } // No subscribers, skip the wrapping to avoid clobbering stack traces // and debugging steps. - return Module._load(request, parent, isMain); + return Module._load(request, parent, isMain, options); } finally { endTimer(logLabel, traceLabel); } @@ -1015,9 +1015,10 @@ function getExportsForCircularRequire(module) { * @param {string} specifier * @param {Module|undefined} parent * @param {boolean} isMain + * @param {boolean} shouldSkipModuleHooks * @returns {{url?: string, format?: string, parentURL?: string, filename: string}} */ -function resolveForCJSWithHooks(specifier, parent, isMain) { +function resolveForCJSWithHooks(specifier, parent, isMain, shouldSkipModuleHooks) { let defaultResolvedURL; let defaultResolvedFilename; let format; @@ -1040,7 +1041,7 @@ function resolveForCJSWithHooks(specifier, parent, isMain) { } // Fast path: no hooks, just return simple results. - if (!resolveHooks.length) { + if (!resolveHooks.length || shouldSkipModuleHooks) { const filename = defaultResolveImpl(specifier, parent, isMain); return { __proto__: null, url: defaultResolvedURL, filename, format }; } @@ -1093,7 +1094,7 @@ function resolveForCJSWithHooks(specifier, parent, isMain) { } const result = { __proto__: null, url, format, filename, parentURL }; - debug('resolveForCJSWithHooks', specifier, parent?.id, '->', result); + debug('resolveForCJSWithHooks', specifier, parent?.id, isMain, shouldSkipModuleHooks, '->', result); return result; } @@ -1186,8 +1187,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} options Additional options for loading the module + * @returns {object} */ -Module._load = function(request, parent, isMain) { +Module._load = function(request, parent, isMain, options = kEmptyObject) { let relResolveCacheIdentifier; if (parent) { debug('Module._load REQUEST %s parent: %s', request, parent.id); @@ -1210,7 +1213,7 @@ Module._load = function(request, parent, isMain) { } } - const resolveResult = resolveForCJSWithHooks(request, parent, isMain); + const resolveResult = resolveForCJSWithHooks(request, parent, isMain, options.shouldSkipModuleHooks); let { format } = resolveResult; const { url, filename } = resolveResult; diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index 2a7221b6d941e2..a8cb010f470afb 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -111,6 +111,7 @@ translators.set('module', function moduleStrategy(url, translateContext, parentU return module; }); +const kShouldSkipModuleHooks = { __proto__: null, shouldSkipModuleHooks: true }; /** * 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. @@ -144,7 +145,9 @@ function loadCJSModule(module, source, url, filename, isMain) { importAttributes = { __proto__: null, type: 'json' }; break; case '.node': - return wrapModuleLoad(specifier, module); + // 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. + return wrapModuleLoad(specifier, module, false, kShouldSkipModuleHooks); default: // fall through } @@ -282,7 +285,9 @@ function createCJSNoSourceModuleWrap(url, parentURL) { debug(`Loading CJSModule ${url}`); if (!module.loaded) { - wrapModuleLoad(filename, null, isMain); + // 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, null, isMain, kShouldSkipModuleHooks); } /** @type {import('./loader').ModuleExports} */ @@ -325,7 +330,9 @@ translators.set('require-commonjs-typescript', (url, translateContext, parentURL // This goes through Module._load to accommodate monkey-patchers. function loadCJSModuleWithModuleLoad(module, source, url, filename, isMain) { assert(module === CJSModule._cache[filename]); - wrapModuleLoad(filename, undefined, isMain); + // 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); } // Handle CommonJS modules referenced by `import` statements or expressions, diff --git a/test/fixtures/value.cjs b/test/fixtures/value.cjs new file mode 100644 index 00000000000000..f16abdc55b3f4c --- /dev/null +++ b/test/fixtures/value.cjs @@ -0,0 +1 @@ +exports.value = 42; diff --git a/test/module-hooks/test-module-hooks-load-import-cjs.js b/test/module-hooks/test-module-hooks-load-import-cjs.js new file mode 100644 index 00000000000000..d42d534b0abdff --- /dev/null +++ b/test/module-hooks/test-module-hooks-load-import-cjs.js @@ -0,0 +1,20 @@ +'use strict'; +// Test that load hook in imported CJS only gets invoked once. + +const common = require('../common'); +const assert = require('assert'); +const { registerHooks } = require('module'); +const path = require('path'); +const { pathToFileURL } = require('url'); + +const hook = registerHooks({ + load: common.mustCall(function(url, context, nextLoad) { + assert.strictEqual(url, pathToFileURL(path.resolve(__dirname, '../fixtures/value.cjs')).href); + return nextLoad(url, context); + }, 1), +}); + +import('../fixtures/value.cjs').then(common.mustCall((result) => { + assert.strictEqual(result.value, 42); + hook.deregister(); +})); diff --git a/test/module-hooks/test-module-hooks-resolve-import-cjs.js b/test/module-hooks/test-module-hooks-resolve-import-cjs.js new file mode 100644 index 00000000000000..a44059606311fe --- /dev/null +++ b/test/module-hooks/test-module-hooks-resolve-import-cjs.js @@ -0,0 +1,18 @@ +'use strict'; +// Test that resolve hook in imported CJS only gets invoked once. + +const common = require('../common'); +const assert = require('assert'); +const { registerHooks } = require('module'); + +const hook = registerHooks({ + resolve: common.mustCall(function(specifier, context, nextResolve) { + assert.strictEqual(specifier, '../fixtures/value.cjs'); + return nextResolve(specifier, context); + }, 1), +}); + +import('../fixtures/value.cjs').then(common.mustCall((result) => { + assert.strictEqual(result.value, 42); + hook.deregister(); +}));