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: #61529Fixes: #57125
Refs: #55808
Refs: #56241
Reviewed-By: Jacob Smith <jacob@frende.me>

Author: joyeecheung

lib/internal/modules/cjs/loader.js

@@ -235,7 +235,7 @@ let statCache = null;
  * See more {@link Module._load}
  * @returns {object}
  */
-functionwrapModuleLoad(request,parent,isMain){
+functionwrapModuleLoad(request,parent,isMain,options){
 constlogLabel=`[${parent?.id||''}] [${request}]`;
 consttraceLabel=`require('${request}')`;
 constchannel=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.
-returnModule._load(request,parent,isMain);
+returnModule._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}}
  */
-functionresolveForCJSWithHooks(specifier,parent,isMain){
+functionresolveForCJSWithHooks(specifier,parent,isMain,shouldSkipModuleHooks){
 letdefaultResolvedURL;
 letdefaultResolvedFilename;
 letformat;
@@ -1065,7 +1066,7 @@ function resolveForCJSWithHooks(specifier, parent, isMain) {
 }
 
 // Fast path: no hooks, just return simple results.
-if(!resolveHooks.length){
+if(!resolveHooks.length||shouldSkipModuleHooks){
 constfilename=defaultResolveImpl(specifier,parent,isMain);
 return{__proto__: null,url: defaultResolvedURL, filename, format };
 }
@@ -1118,7 +1119,7 @@ function resolveForCJSWithHooks(specifier, parent, isMain) {
 }
 
 constresult={__proto__: null, url, format, filename, parentURL };
-debug('resolveForCJSWithHooks',specifier,parent?.id,'->',result);
+debug('resolveForCJSWithHooks',specifier,parent?.id,isMain,shouldSkipModuleHooks,'->',result);
 returnresult;
 }
 
@@ -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){
 letrelResolveCacheIdentifier;
 if(parent){
 debug('Module._load REQUEST %s parent: %s',request,parent.id);
@@ -1236,7 +1238,7 @@ Module._load = function(request, parent, isMain) {
 }
 }
 
-constresolveResult=resolveForCJSWithHooks(request,parent,isMain);
+constresolveResult=resolveForCJSWithHooks(request,parent,isMain,options.shouldSkipModuleHooks);
 let{ format }=resolveResult;
 const{ url, filename }=resolveResult;
 

lib/internal/modules/esm/translators.js

@@ -100,6 +100,7 @@ translators.set('module', function moduleStrategy(url, translateContext, parentU
 });
 
 const{requestTypes: { kRequireInImportedCJS }}=require('internal/modules/esm/utils');
+constkShouldSkipModuleHooks={__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.
@@ -132,7 +133,9 @@ function loadCJSModule(module, source, url, filename, isMain) {
 importAttributes={__proto__: null,type: 'json'};
 break;
 case'.node':
-returnwrapModuleLoad(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.
+returnwrapModuleLoad(specifier,module,false,kShouldSkipModuleHooks);
 default:
 // fall through
 }
@@ -280,7 +283,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} */
@@ -317,7 +322,9 @@ translators.set('require-commonjs-typescript', (url, translateContext, parentURL
 // This goes through Module._load to accommodate monkey-patchers.
 functionloadCJSModuleWithModuleLoad(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,

test/fixtures/value.cjs

@@ -0,0 +1 @@
+exports.value=42;

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.
+
+constcommon=require('../common');
+constassert=require('assert');
+const{ registerHooks }=require('module');
+constpath=require('path');
+const{ pathToFileURL }=require('url');
+
+consthook=registerHooks({
+load: common.mustCall(function(url,context,nextLoad){
+assert.strictEqual(url,pathToFileURL(path.resolve(__dirname,'../fixtures/value.cjs')).href);
+returnnextLoad(url,context);
+},1),
+});
+
+import('../fixtures/value.cjs').then(common.mustCall((result)=>{
+assert.strictEqual(result.value,42);
+hook.deregister();
+}));

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.
+
+constcommon=require('../common');
+constassert=require('assert');
+const{ registerHooks }=require('module');
+
+consthook=registerHooks({
+resolve: common.mustCall(function(specifier,context,nextResolve){
+assert.strictEqual(specifier,'../fixtures/value.cjs');
+returnnextResolve(specifier,context);
+},1),
+});
+
+import('../fixtures/value.cjs').then(common.mustCall((result)=>{
+assert.strictEqual(result.value,42);
+hook.deregister();
+}));