Repository: nodejs/node
Pull request: 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.

Fixes: https://github.com/nodejs/node/issues/57125
Refs: https://github.com/nodejs/node/issues/55808
Refs: https://github.com/nodejs/node/issues/56241

<!--
Before submitting a pull request, please read:

- the CONTRIBUTING guide at https://github.com/nodejs/node/blob/HEAD/CONTRIBUTING.md
- the commit message formatting guidelines at
  https://github.com/nodejs/node/blob/HEAD/doc/contributing/pull-requests.md#commit-message-guidelines

For code changes:
1. Include tests for any bug fixes or new features.
2. Update documentation if relevant.
3. Ensure that `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes.

If you believe this PR should be highlighted in the Node.js CHANGELOG
please add the `notable-change` label.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.
-->


Changed files:
filename: lib/internal/modules/cjs/loader.js
patch: @@ -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;
 

filename: lib/internal/modules/esm/translators.js
patch: @@ -100,6 +100,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.
@@ -132,7 +133,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
       }
@@ -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.
 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,

filename: test/fixtures/value.cjs
patch: @@ -0,0 +1 @@
+exports.value = 42;

filename: test/module-hooks/test-module-hooks-load-import-cjs.js
patch: @@ -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();
+}));

filename: test/module-hooks/test-module-hooks-resolve-import-cjs.js
patch: @@ -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();
+}));