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

PR-URL: nodejs#43303 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
aduh95 · Jul 19, 2022 · 4c726b0 · 4c726b0
1 parent 1bd62be
commit 4c726b0
Show file tree

Hide file tree

Showing 6 changed files with 198 additions and 109 deletions.
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
@@ -1360,7 +1360,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
@@ -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,
@@ -89,6 +94,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
@@ -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 {
@@ -735,55 +795,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(
@@ -793,10 +832,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
@@ -1115,7 +1115,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);