esm: fix chain advances when loader calls next<HookName> multiple times

PR-URL: https://github.com/nodejs/node/pull/43303
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

6 files changed, 198 insertions, 109 deletions

diff --git a/lib/internal/errors.js b/lib/internal/errors.js
index cf54bbe6fb3..306dcbad998 100644
--- a/lib/internal/errors.js
+++ b/lib/internal/errors.js
@@ -1372,7 +1372,7 @@ E('ERR_IPC_ONE_PIPE', 'Child process can have only one IPC pipe', Error);
 E('ERR_IPC_SYNC_FORK', 'IPC cannot be used with synchronous forks', Error);
 E(
   'ERR_LOADER_CHAIN_INCOMPLETE',
-  'The "%s" hook from %s did not call the next hook in its chain and did not' +
+  '"%s" did not call the next hook in its chain and did not' +
   ' explicitly signal a short circuit. If this is intentional, include' +
   ' `shortCircuit: true` in the hook\'s return.',
   Error
diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js
index c16f9122f1d..f81ffa8e31c 100644
--- a/lib/internal/modules/esm/loader.js
+++ b/lib/internal/modules/esm/loader.js
@@ -12,17 +12,22 @@ const {
   FunctionPrototypeCall,
   ObjectAssign,
   ObjectCreate,
+  ObjectDefineProperty,
   ObjectSetPrototypeOf,
   PromiseAll,
+  ReflectApply,
   RegExpPrototypeExec,
   SafeArrayIterator,
   SafeWeakMap,
+  StringPrototypeSlice,
+  StringPrototypeToUpperCase,
   globalThis,
 } = primordials;
 const { MessageChannel } = require('internal/worker/io');
 
 const {
   ERR_LOADER_CHAIN_INCOMPLETE,
+  ERR_INTERNAL_ASSERTION,
   ERR_INVALID_ARG_TYPE,
   ERR_INVALID_ARG_VALUE,
   ERR_INVALID_RETURN_PROPERTY_VALUE,
@@ -90,6 +95,81 @@ const { getOptionValue } = require('internal/options');
 let emittedSpecifierResolutionWarning = false;
 
 /**
+ * A utility function to iterate through a hook chain, track advancement in the
+ * chain, and generate and supply the `next<HookName>` argument to the custom
+ * hook.
+ * @param {KeyedHook[]} chain The whole hook chain.
+ * @param {object} meta Properties that change as the current hook advances
+ * along the chain.
+ * @param {boolean} meta.chainFinished Whether the end of the chain has been
+ * reached AND invoked.
+ * @param {string} meta.hookErrIdentifier A user-facing identifier to help
+ *  pinpoint where an error occurred. Ex "file:///foo.mjs 'resolve'".
+ * @param {number} meta.hookIndex A non-negative integer tracking the current
+ * position in the hook chain.
+ * @param {string} meta.hookName The kind of hook the chain is (ex 'resolve')
+ * @param {boolean} meta.shortCircuited Whether a hook signaled a short-circuit.
+ * @param {(hookErrIdentifier, hookArgs) => void} validate A wrapper function
+ *  containing all validation of a custom loader hook's intermediary output. Any
+ *  validation within MUST throw.
+ * @returns {function next<HookName>(...hookArgs)} The next hook in the chain.
+ */
+function nextHookFactory(chain, meta, validate) {
+  // First, prepare the current
+  const { hookName } = meta;
+  const {
+    fn: hook,
+    url: hookFilePath,
+  } = chain[meta.hookIndex];
+
+  // ex 'nextResolve'
+  const nextHookName = `next${
+    StringPrototypeToUpperCase(hookName[0]) +
+    StringPrototypeSlice(hookName, 1)
+  }`;
+
+  // When hookIndex is 0, it's reached the default, which does not call next()
+  // so feed it a noop that blows up if called, so the problem is obvious.
+  const generatedHookIndex = meta.hookIndex;
+  let nextNextHook;
+  if (meta.hookIndex > 0) {
+    // Now, prepare the next: decrement the pointer so the next call to the
+    // factory generates the next link in the chain.
+    meta.hookIndex--;
+
+    nextNextHook = nextHookFactory(chain, meta, validate);
+  } else {
+    // eslint-disable-next-line func-name-matching
+    nextNextHook = function chainAdvancedTooFar() {
+      throw new ERR_INTERNAL_ASSERTION(
+        `ESM custom loader '${hookName}' advanced beyond the end of the chain.`
+      );
+    };
+  }
+
+  return ObjectDefineProperty(
+    async (...args) => {
+      // Update only when hook is invoked to avoid fingering the wrong filePath
+      meta.hookErrIdentifier = `${hookFilePath} '${hookName}'`;
+
+      validate(`${meta.hookErrIdentifier} hook's ${nextHookName}()`, args);
+
+      // Set when next<HookName> is actually called, not just generated.
+      if (generatedHookIndex === 0) { meta.chainFinished = true; }
+
+      ArrayPrototypePush(args, nextNextHook);
+      const output = await ReflectApply(hook, undefined, args);
+
+      if (output?.shortCircuit === true) { meta.shortCircuited = true; }
+      return output;
+
+    },
+    'name',
+    { __proto__: null, value: nextHookName },
+  );
+}
+
+/**
  * An ESMLoader instance is used as the main entry point for loading ES modules.
  * Currently, this is a singleton -- there is only one used for loading
  * the main module and everything in its dependency graph.
@@ -471,32 +551,21 @@ class ESMLoader {
    * @returns {{ format: ModuleFormat, source: ModuleSource }}
    */
   async load(url, context = {}) {
-    const loaders = this.#loaders;
-    let hookIndex = loaders.length - 1;
-    let {
-      fn: loader,
-      url: loaderFilePath,
-    } = loaders[hookIndex];
-    let chainFinished = hookIndex === 0;
-    let shortCircuited = false;
-
-    const nextLoad = async (nextUrl, ctx = context) => {
-      --hookIndex; // `nextLoad` has been called, so decrement our pointer.
-
-      ({
-        fn: loader,
-        url: loaderFilePath,
-      } = loaders[hookIndex]);
-
-      if (hookIndex === 0) { chainFinished = true; }
-
-      const hookErrIdentifier = `${loaderFilePath} "load"`;
+    const chain = this.#loaders;
+    const meta = {
+      chainFinished: null,
+      hookErrIdentifier: '',
+      hookIndex: chain.length - 1,
+      hookName: 'load',
+      shortCircuited: false,
+    };
 
+    const validate = (hookErrIdentifier, { 0: nextUrl, 1: ctx }) => {
       if (typeof nextUrl !== 'string') {
         // non-strings can be coerced to a url string
         // validateString() throws a less-specific error
         throw new ERR_INVALID_ARG_TYPE(
-          `${hookErrIdentifier} nextLoad(url)`,
+          `${hookErrIdentifier} url`,
           'a url string',
           nextUrl,
         );
@@ -508,29 +577,20 @@ class ESMLoader {
           new URL(nextUrl);
         } catch {
           throw new ERR_INVALID_ARG_VALUE(
-            `${hookErrIdentifier} nextLoad(url)`,
+            `${hookErrIdentifier} url`,
             nextUrl,
             'should be a url string',
           );
         }
       }
 
-      validateObject(ctx, `${hookErrIdentifier} nextLoad(, context)`);
-
-      const output = await loader(nextUrl, ctx, nextLoad);
-
-      if (output?.shortCircuit === true) { shortCircuited = true; }
-
-      return output;
+      validateObject(ctx, `${hookErrIdentifier} context`);
     };
 
-    const loaded = await loader(
-      url,
-      context,
-      nextLoad,
-    );
+    const nextLoad = nextHookFactory(chain, meta, validate);
 
-    const hookErrIdentifier = `${loaderFilePath} load`;
+    const loaded = await nextLoad(url, context);
+    const { hookErrIdentifier } = meta; // Retrieve the value after all settled
 
     if (typeof loaded !== 'object') { // [2]
       throw new ERR_INVALID_RETURN_VALUE(
@@ -540,10 +600,10 @@ class ESMLoader {
       );
     }
 
-    if (loaded?.shortCircuit === true) { shortCircuited = true; }
+    if (loaded?.shortCircuit === true) { meta.shortCircuited = true; }
 
-    if (!chainFinished && !shortCircuited) {
-      throw new ERR_LOADER_CHAIN_INCOMPLETE('load', loaderFilePath);
+    if (!meta.chainFinished && !meta.shortCircuited) {
+      throw new ERR_LOADER_CHAIN_INCOMPLETE(hookErrIdentifier);
     }
 
     const {
@@ -736,55 +796,34 @@ class ESMLoader {
         parentURL,
       );
     }
-    const resolvers = this.#resolvers;
-
-    let hookIndex = resolvers.length - 1;
-    let {
-      fn: resolver,
-      url: resolverFilePath,
-    } = resolvers[hookIndex];
-    let chainFinished = hookIndex === 0;
-    let shortCircuited = false;
+    const chain = this.#resolvers;
+    const meta = {
+      chainFinished: null,
+      hookErrIdentifier: '',
+      hookIndex: chain.length - 1,
+      hookName: 'resolve',
+      shortCircuited: false,
+    };
 
     const context = {
       conditions: DEFAULT_CONDITIONS,
       importAssertions,
       parentURL,
     };
-
-    const nextResolve = async (suppliedSpecifier, ctx = context) => {
-      --hookIndex; // `nextResolve` has been called, so decrement our pointer.
-
-      ({
-        fn: resolver,
-        url: resolverFilePath,
-      } = resolvers[hookIndex]);
-
-      if (hookIndex === 0) { chainFinished = true; }
-
-      const hookErrIdentifier = `${resolverFilePath} "resolve"`;
+    const validate = (hookErrIdentifier, { 0: suppliedSpecifier, 1: ctx }) => {
 
       validateString(
         suppliedSpecifier,
-        `${hookErrIdentifier} nextResolve(specifier)`,
+        `${hookErrIdentifier} specifier`,
       ); // non-strings can be coerced to a url string
 
-      validateObject(ctx, `${hookErrIdentifier} nextResolve(, context)`);
-
-      const output = await resolver(suppliedSpecifier, ctx, nextResolve);
-
-      if (output?.shortCircuit === true) { shortCircuited = true; }
-
-      return output;
+      validateObject(ctx, `${hookErrIdentifier} context`);
     };
 
-    const resolution = await resolver(
-      originalSpecifier,
-      context,
-      nextResolve,
-    );
+    const nextResolve = nextHookFactory(chain, meta, validate);
 
-    const hookErrIdentifier = `${resolverFilePath} resolve`;
+    const resolution = await nextResolve(originalSpecifier, context);
+    const { hookErrIdentifier } = meta; // Retrieve the value after all settled
 
     if (typeof resolution !== 'object') { // [2]
       throw new ERR_INVALID_RETURN_VALUE(
@@ -794,10 +833,10 @@ class ESMLoader {
       );
     }
 
-    if (resolution?.shortCircuit === true) { shortCircuited = true; }
+    if (resolution?.shortCircuit === true) { meta.shortCircuited = true; }
 
-    if (!chainFinished && !shortCircuited) {
-      throw new ERR_LOADER_CHAIN_INCOMPLETE('resolve', resolverFilePath);
+    if (!meta.chainFinished && !meta.shortCircuited) {
+      throw new ERR_LOADER_CHAIN_INCOMPLETE(hookErrIdentifier);
     }
 
     const {
diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js
index 8767109d7c4..79ea42233b6 100644
--- a/lib/internal/modules/esm/resolve.js
+++ b/lib/internal/modules/esm/resolve.js
@@ -1081,7 +1081,7 @@ function throwIfUnsupportedURLScheme(parsed, experimentalNetworkImports) {
   }
 }
 
-async function defaultResolve(specifier, context = {}, defaultResolveUnused) {
+async function defaultResolve(specifier, context = {}) {
   let { parentURL, conditions } = context;
   if (parentURL && policy?.manifest) {
     const redirects = policy.manifest.getDependencyMapper(parentURL);
diff --git a/test/es-module/test-esm-loader-chaining.mjs b/test/es-module/test-esm-loader-chaining.mjs
index 1055c44a156..977c20dbbba 100644
--- a/test/es-module/test-esm-loader-chaining.mjs
+++ b/test/es-module/test-esm-loader-chaining.mjs
@@ -25,8 +25,8 @@ const commonArgs = [
   );
 
   assert.strictEqual(stderr, '');
-  assert.strictEqual(status, 0);
   assert.match(stdout, /number/); // node:fs is an object
+  assert.strictEqual(status, 0);
 }
 
 { // Verify loaded source is properly different when only load changes something
@@ -47,8 +47,8 @@ const commonArgs = [
   assert.strictEqual(stderr, '');
   assert.match(stdout, /load passthru/);
   assert.match(stdout, /resolve passthru/);
-  assert.strictEqual(status, 0);
   assert.match(stdout, /foo/);
+  assert.strictEqual(status, 0);
 }
 
 { // Verify multiple changes from hooks result in proper output
@@ -70,8 +70,8 @@ const commonArgs = [
 
   assert.strictEqual(stderr, '');
   assert.match(stdout, /resolve 42/); // It did go thru resolve-42
-  assert.strictEqual(status, 0);
   assert.match(stdout, /foo/); // LIFO, so resolve-foo won
+  assert.strictEqual(status, 0);
 }
 
 { // Verify modifying context within resolve chain is respected
@@ -117,8 +117,52 @@ const commonArgs = [
 
   assert.strictEqual(stderr, '');
   assert.match(stdout, /resolve foo/); // It did go thru resolve-foo
-  assert.strictEqual(status, 0);
   assert.match(stdout, /42/); // LIFO, so resolve-42 won
+  assert.strictEqual(status, 0);
+}
+
+{ // Verify multiple calls to next within same loader receive correct "next" fn
+  const { status, stderr, stdout } = spawnSync(
+    process.execPath,
+    [
+      '--loader',
+      fixtures.fileURL('es-module-loaders', 'loader-resolve-shortcircuit.mjs'),
+      '--loader',
+      fixtures.fileURL('es-module-loaders', 'loader-resolve-foo.mjs'),
+      '--loader',
+      fixtures.fileURL('es-module-loaders', 'loader-resolve-multiple-next-calls.mjs'),
+      '--loader',
+      fixtures.fileURL('es-module-loaders', 'loader-load-foo-or-42.mjs'),
+      ...commonArgs,
+    ],
+    { encoding: 'utf8' },
+  );
+
+  const countFoos = stdout.match(/resolve foo/g)?.length;
+
+  assert.strictEqual(stderr, '');
+  assert.strictEqual(countFoos, 2);
+  assert.strictEqual(status, 0);
+}
+
+{ // Verify next<HookName> function's `name` is correct
+  const { status, stderr, stdout } = spawnSync(
+    process.execPath,
+    [
+      '--loader',
+      fixtures.fileURL('es-module-loaders', 'loader-resolve-shortcircuit.mjs'),
+      '--loader',
+      fixtures.fileURL('es-module-loaders', 'loader-resolve-42.mjs'),
+      '--loader',
+      fixtures.fileURL('es-module-loaders', 'loader-load-foo-or-42.mjs'),
+      ...commonArgs,
+    ],
+    { encoding: 'utf8' },
+  );
+
+  assert.strictEqual(stderr, '');
+  assert.match(stdout, /next<HookName>: nextResolve/);
+  assert.strictEqual(status, 0);
 }
 
 { // Verify error thrown for incomplete resolve chain, citing errant loader & hook
@@ -137,10 +181,10 @@ const commonArgs = [
   );
 
   assert.match(stdout, /resolve passthru/);
-  assert.strictEqual(status, 1);
   assert.match(stderr, /ERR_LOADER_CHAIN_INCOMPLETE/);
   assert.match(stderr, /loader-resolve-incomplete\.mjs/);
-  assert.match(stderr, /"resolve"/);
+  assert.match(stderr, /'resolve'/);
+  assert.strictEqual(status, 1);
 }
 
 { // Verify error NOT thrown when nested resolve hook signaled a short circuit
@@ -201,10 +245,10 @@ const commonArgs = [
   );
 
   assert.doesNotMatch(stdout, /resolve passthru/);
-  assert.strictEqual(status, 1);
   assert.match(stderr, /ERR_LOADER_CHAIN_INCOMPLETE/);
   assert.match(stderr, /loader-resolve-incomplete\.mjs/);
-  assert.match(stderr, /"resolve"/);
+  assert.match(stderr, /'resolve'/);
+  assert.strictEqual(status, 1);
 }
 
 { // Verify error thrown for incomplete load chain, citing errant loader & hook
@@ -223,10 +267,10 @@ const commonArgs = [
   );
 
   assert.match(stdout, /load passthru/);
-  assert.strictEqual(status, 1);
   assert.match(stderr, /ERR_LOADER_CHAIN_INCOMPLETE/);
   assert.match(stderr, /loader-load-incomplete\.mjs/);
-  assert.match(stderr, /"load"/);
+  assert.match(stderr, /'load'/);
+  assert.strictEqual(status, 1);
 }
 
 { // Verify chain does break and throws appropriately
@@ -245,13 +289,13 @@ const commonArgs = [
   );
 
   assert.doesNotMatch(stdout, /load passthru/);
-  assert.strictEqual(status, 1);
   assert.match(stderr, /ERR_LOADER_CHAIN_INCOMPLETE/);
   assert.match(stderr, /loader-load-incomplete\.mjs/);
-  assert.match(stderr, /"load"/);
+  assert.match(stderr, /'load'/);
+  assert.strictEqual(status, 1);
 }
 
-{ // Verify error thrown when invalid `specifier` argument passed to `resolve…next`
+{ // Verify error thrown when invalid `specifier` argument passed to `nextResolve`
   const { status, stderr } = spawnSync(
     process.execPath,
     [
@@ -267,11 +311,10 @@ const commonArgs = [
   assert.strictEqual(status, 1);
   assert.match(stderr, /ERR_INVALID_ARG_TYPE/);
   assert.match(stderr, /loader-resolve-bad-next-specifier\.mjs/);
-  assert.match(stderr, /"resolve"/);
-  assert.match(stderr, /nextResolve\(specifier\)/);
+  assert.match(stderr, /'resolve' hook's nextResolve\(\) specifier/);
 }
 
-{ // Verify error thrown when invalid `context` argument passed to `resolve…next`
+{ // Verify error thrown when invalid `context` argument passed to `nextResolve`
   const { status, stderr } = spawnSync(
     process.execPath,
     [
@@ -284,14 +327,13 @@ const commonArgs = [
     { encoding: 'utf8' },
   );
 
-  assert.strictEqual(status, 1);
   assert.match(stderr, /ERR_INVALID_ARG_TYPE/);
   assert.match(stderr, /loader-resolve-bad-next-context\.mjs/);
-  assert.match(stderr, /"resolve"/);
-  assert.match(stderr, /nextResolve\(, context\)/);
+  assert.match(stderr, /'resolve' hook's nextResolve\(\) context/);
+  assert.strictEqual(status, 1);
 }
 
-{ // Verify error thrown when invalid `url` argument passed to `load…next`
+{ // Verify error thrown when invalid `url` argument passed to `nextLoad`
   const { status, stderr } = spawnSync(
     process.execPath,
     [
@@ -304,14 +346,13 @@ const commonArgs = [
     { encoding: 'utf8' },
   );
 
-  assert.strictEqual(status, 1);
   assert.match(stderr, /ERR_INVALID_ARG_TYPE/);
   assert.match(stderr, /loader-load-bad-next-url\.mjs/);
-  assert.match(stderr, /"load"/);
-  assert.match(stderr, /nextLoad\(url\)/);
+  assert.match(stderr, /'load' hook's nextLoad\(\) url/);
+  assert.strictEqual(status, 1);
 }
 
-{ // Verify error thrown when invalid `url` argument passed to `load…next`
+{ // Verify error thrown when invalid `url` argument passed to `nextLoad`
   const { status, stderr } = spawnSync(
     process.execPath,
     [
@@ -324,14 +365,13 @@ const commonArgs = [
     { encoding: 'utf8' },
   );
 
-  assert.strictEqual(status, 1);
   assert.match(stderr, /ERR_INVALID_ARG_VALUE/);
   assert.match(stderr, /loader-load-impersonating-next-url\.mjs/);
-  assert.match(stderr, /"load"/);
-  assert.match(stderr, /nextLoad\(url\)/);
+  assert.match(stderr, /'load' hook's nextLoad\(\) url/);
+  assert.strictEqual(status, 1);
 }
 
-{ // Verify error thrown when invalid `context` argument passed to `load…next`
+{ // Verify error thrown when invalid `context` argument passed to `nextLoad`
   const { status, stderr } = spawnSync(
     process.execPath,
     [
@@ -344,9 +384,8 @@ const commonArgs = [
     { encoding: 'utf8' },
   );
 
-  assert.strictEqual(status, 1);
   assert.match(stderr, /ERR_INVALID_ARG_TYPE/);
   assert.match(stderr, /loader-load-bad-next-context\.mjs/);
-  assert.match(stderr, /"load"/);
-  assert.match(stderr, /nextLoad\(, context\)/);
+  assert.match(stderr, /'load' hook's nextLoad\(\) context/);
+  assert.strictEqual(status, 1);
 }
diff --git a/test/fixtures/es-module-loaders/loader-resolve-42.mjs b/test/fixtures/es-module-loaders/loader-resolve-42.mjs
index f4dffd70fc9..5c90bceed2b 100644
--- a/test/fixtures/es-module-loaders/loader-resolve-42.mjs
+++ b/test/fixtures/es-module-loaders/loader-resolve-42.mjs
@@ -1,4 +1,6 @@
 export async function resolve(specifier, context, next) {
   console.log('resolve 42'); // This log is deliberate
+  console.log('next<HookName>:', next.name); // This log is deliberate
+
   return next('file:///42.mjs', context);
 }
diff --git a/test/fixtures/es-module-loaders/loader-resolve-multiple-next-calls.mjs b/test/fixtures/es-module-loaders/loader-resolve-multiple-next-calls.mjs
new file mode 100644
index 00000000000..88d333c2404
--- /dev/null
+++ b/test/fixtures/es-module-loaders/loader-resolve-multiple-next-calls.mjs
@@ -0,0 +1,9 @@
+export async function resolve(specifier, context, next) {
+  const { url: first } = await next(specifier, context);
+  const { url: second } = await next(specifier, context);
+
+  return {
+    format: 'module',
+    url: first,
+  };
+}