From cc0b7cd030c22ebc5c86e78478107a18e1af9b0e Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 22 Jan 2026 19:40:09 +0100 Subject: [PATCH] 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. --- lib/internal/modules/cjs/loader.js | 18 +++++++++-------- 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, 59 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 0ea268bcec7ece..6a841a2f72f485 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -235,7 +235,7 @@ let statCache = null; * See more {@link Module._load} * @returns {object} */ -function wrapModuleLoad(request, parent, isMain) { +function wrapModuleLoad(request, parent, isMain, options) { const logLabel = `[${parent?.id || ''}] [${request}]`; const traceLabel = `require('${request}')`; const channel = onRequire(); @@ -248,11 +248,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); } @@ -1040,9 +1040,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; @@ -1065,7 +1066,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 }; } @@ -1118,7 +1119,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; } @@ -1211,9 +1212,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); @@ -1236,7 +1238,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 87f271adcd3e50..cd34c2f7570ec7 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -117,6 +117,7 @@ translators.set('module', function moduleStrategy(url, translateContext, parentU }); const { requestTypes: { kRequireInImportedCJS } } = require('internal/modules/esm/utils'); +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. @@ -149,7 +150,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 } @@ -297,7 +300,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} */ @@ -340,7 +345,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(); +}));