module: allow omitting context in synchronous next hooks

This aligns the behavior of synchronous hooks with asynchronous
hooks by allowing omission of the context parameter in the
invocation of next hooks. The contexts are merged along the
chain.

PR-URL: #57056
Fixes: #57030
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Author: joyeecheung

Diff for: lib/internal/modules/customization_hooks.js

@@ -4,6 +4,7 @@ const {
   ArrayPrototypeFindIndex,
   ArrayPrototypePush,
   ArrayPrototypeSplice,
+  ObjectAssign,
   ObjectFreeze,
   StringPrototypeStartsWith,
   Symbol,
@@ -162,19 +163,31 @@ function convertURLToCJSFilename(url) {
  * @param {'load'|'resolve'} name Name of the hook in ModuleHooks.
  * @param {Function} defaultStep The default step in the chain.
  * @param {Function} validate A function that validates and sanitize the result returned by the chain.
- * @returns {Function}
  */
-function buildHooks(hooks, name, defaultStep, validate) {
+function buildHooks(hooks, name, defaultStep, validate, mergedContext) {
   let lastRunIndex = hooks.length;
-  function wrapHook(index, userHook, next) {
-    return function wrappedHook(...args) {
+  /**
+   * Helper function to wrap around invocation of user hook or the default step
+   * in order to fill in missing arguments or check returned results.
+   * Due to the merging of the context, this must be a closure.
+   * @param {number} index Index in the chain. Default step is 0, last added hook is 1,
+   *                       and so on.
+   * @param {Function} userHookOrDefault Either the user hook or the default step to invoke.
+   * @param {Function|undefined} next The next wrapped step. If this is the default step, it's undefined.
+   * @returns {Function} Wrapped hook or default step.
+   */
+  function wrapHook(index, userHookOrDefault, next) {
+    return function nextStep(arg0, context) {
       lastRunIndex = index;
-      const hookResult = userHook(...args, next);
+      if (context && context !== mergedContext) {
+        ObjectAssign(mergedContext, context);
+      }
+      const hookResult = userHookOrDefault(arg0, mergedContext, next);
       if (lastRunIndex > 0 && lastRunIndex === index && !hookResult.shortCircuit) {
         throw new ERR_INVALID_RETURN_PROPERTY_VALUE('true', name, 'shortCircuit',
                                                     hookResult.shortCircuit);
       }
-      return validate(...args, hookResult);
+      return validate(arg0, mergedContext, hookResult);
     };
   }
   const chain = [wrapHook(0, defaultStep)];
@@ -330,7 +343,7 @@ function loadWithHooks(url, originalFormat, importAttributes, conditions, defaul
     return defaultLoad(url, context);
   }
 
-  const runner = buildHooks(loadHooks, 'load', defaultLoad, validateLoad);
+  const runner = buildHooks(loadHooks, 'load', defaultLoad, validateLoad, context);
 
   const result = runner(url, context);
   const { source, format } = result;
@@ -373,7 +386,7 @@ function resolveWithHooks(specifier, parentURL, importAttributes, conditions, de
     return defaultResolve(specifier, context);
   }
 
-  const runner = buildHooks(resolveHooks, 'resolve', defaultResolve, validateResolve);
+  const runner = buildHooks(resolveHooks, 'resolve', defaultResolve, validateResolve, context);
 
   return runner(specifier, context);
 }

Diff for: test/module-hooks/test-module-hooks-load-context-merged-esm.mjs

@@ -0,0 +1,31 @@
+// Test that the context parameter will be merged in multiple load hooks.
+
+import * as common from '../common/index.mjs';
+import assert from 'node:assert';
+import { registerHooks } from 'node:module';
+
+const hook1 = registerHooks({
+  load: common.mustCall(function(url, context, nextLoad) {
+    assert.strictEqual(context.testProp, 'custom');  // It should be merged from hook 2 and 3.
+    return nextLoad(url, context);
+  }, 1),
+});
+
+const hook2 = registerHooks({
+  load: common.mustCall(function(url, context, nextLoad) {
+    assert.strictEqual(context.testProp, 'custom');  // It should be merged from hook 3.
+    return nextLoad(url);  // Omit the context.
+  }, 1),
+});
+
+const hook3 = registerHooks({
+  load: common.mustCall(function(url, context, nextLoad) {
+    return nextLoad(url, { testProp: 'custom' });  // Add a custom property
+  }, 1),
+});
+
+await import('../fixtures/es-modules/message.mjs');
+
+hook3.deregister();
+hook2.deregister();
+hook1.deregister();

Diff for: test/module-hooks/test-module-hooks-load-context-merged.js

@@ -0,0 +1,33 @@
+'use strict';
+
+// Test that the context parameter will be merged in multiple load hooks.
+
+const common = require('../common');
+const assert = require('assert');
+const { registerHooks } = require('module');
+
+const hook1 = registerHooks({
+  load: common.mustCall(function(url, context, nextLoad) {
+    assert.strictEqual(context.testProp, 'custom');  // It should be merged from hook 2 and 3.
+    return nextLoad(url, context);
+  }, 1),
+});
+
+const hook2 = registerHooks({
+  load: common.mustCall(function(url, context, nextLoad) {
+    assert.strictEqual(context.testProp, 'custom');  // It should be merged from hook 3.
+    return nextLoad(url);  // Omit the context.
+  }, 1),
+});
+
+const hook3 = registerHooks({
+  load: common.mustCall(function(url, context, nextLoad) {
+    return nextLoad(url, { testProp: 'custom' });  // Add a custom property
+  }, 1),
+});
+
+require('../fixtures/empty.js');
+
+hook3.deregister();
+hook2.deregister();
+hook1.deregister();

Diff for: test/module-hooks/test-module-hooks-load-context-optional-esm.mjs

@@ -0,0 +1,14 @@
+// Test that the context parameter can be omitted in the nextLoad invocation.
+
+import * as common from '../common/index.mjs';
+import { registerHooks } from 'node:module';
+
+const hook = registerHooks({
+  load: common.mustCall(function(url, context, nextLoad) {
+    return nextLoad(url);
+  }, 1),
+});
+
+await import('../fixtures/es-modules/message.mjs');
+
+hook.deregister();

Diff for: test/module-hooks/test-module-hooks-load-context-optional.js

@@ -0,0 +1,16 @@
+'use strict';
+
+// Test that the context parameter can be omitted in the nextLoad invocation.
+
+const common = require('../common');
+const { registerHooks } = require('module');
+
+const hook = registerHooks({
+  load: common.mustCall(function(url, context, nextLoad) {
+    return nextLoad(url);
+  }, 1),
+});
+
+require('../fixtures/empty.js');
+
+hook.deregister();

Diff for: test/module-hooks/test-module-hooks-resolve-context-merged-esm.mjs

@@ -0,0 +1,32 @@
+// Test that the context parameter will be merged in multiple resolve hooks.
+
+import * as common from '../common/index.mjs';
+import assert from 'node:assert';
+import { registerHooks } from 'node:module';
+
+const hook1 = registerHooks({
+  resolve: common.mustCall(function(specifier, context, nextResolve) {
+    assert.strictEqual(context.testProp, 'custom');  // It should be merged from hook 2 and 3.
+    const result = nextResolve(specifier, context);
+    return result;
+  }, 1),
+});
+
+const hook2 = registerHooks({
+  resolve: common.mustCall(function(specifier, context, nextResolve) {
+    assert.strictEqual(context.testProp, 'custom');  // It should be merged from hook 3.
+    return nextResolve(specifier);  // Omit the context.
+  }, 1),
+});
+
+const hook3 = registerHooks({
+  resolve: common.mustCall(function(specifier, context, nextResolve) {
+    return nextResolve(specifier, { testProp: 'custom' });  // Add a custom property
+  }, 1),
+});
+
+await import('../fixtures/es-modules/message.mjs');
+
+hook3.deregister();
+hook2.deregister();
+hook1.deregister();

Diff for: test/module-hooks/test-module-hooks-resolve-context-merged.js

@@ -0,0 +1,34 @@
+'use strict';
+
+// Test that the context parameter will be merged in multiple resolve hooks.
+
+const common = require('../common');
+const assert = require('assert');
+const { registerHooks } = require('module');
+
+const hook1 = registerHooks({
+  resolve: common.mustCall(function(specifier, context, nextResolve) {
+    assert.strictEqual(context.testProp, 'custom');  // It should be merged from hook 2 and 3.
+    const result = nextResolve(specifier, context);
+    return result;
+  }, 1),
+});
+
+const hook2 = registerHooks({
+  resolve: common.mustCall(function(specifier, context, nextResolve) {
+    assert.strictEqual(context.testProp, 'custom');  // It should be merged from hook 3.
+    return nextResolve(specifier);  // Omit the context.
+  }, 1),
+});
+
+const hook3 = registerHooks({
+  resolve: common.mustCall(function(specifier, context, nextResolve) {
+    return nextResolve(specifier, { testProp: 'custom' });  // Add a custom property
+  }, 1),
+});
+
+require('../fixtures/empty.js');
+
+hook3.deregister();
+hook2.deregister();
+hook1.deregister();

Diff for: test/module-hooks/test-module-hooks-resolve-context-optional-esm.mjs

@@ -0,0 +1,14 @@
+// Test that the context parameter can be omitted in the nextResolve invocation.
+
+import * as common from '../common/index.mjs';
+import { registerHooks } from 'node:module';
+
+const hook = registerHooks({
+  resolve: common.mustCall(function(specifier, context, nextResolve) {
+    return nextResolve(specifier);
+  }, 1),
+});
+
+await import('../fixtures/es-modules/message.mjs');
+
+hook.deregister();

Diff for: test/module-hooks/test-module-hooks-resolve-context-optional.js

@@ -0,0 +1,16 @@
+'use strict';
+
+// Test that the context parameter can be omitted in the nextResolve invocation.
+
+const common = require('../common');
+const { registerHooks } = require('module');
+
+const hook = registerHooks({
+  resolve: common.mustCall(function(specifier, context, nextResolve) {
+    return nextResolve(specifier);
+  }, 1),
+});
+
+require('../fixtures/empty.js');
+
+hook.deregister();