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

…imes
nodejs · Jun 2, 2022 · 3cf8f28 · 3cf8f28
1 parent 8f39f51
commit 3cf8f28
Show file tree

Hide file tree

Showing 5 changed files with 154 additions and 103 deletions.
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
@@ -1361,7 +1361,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,12 +12,16 @@ const {
   FunctionPrototypeCall,
   ObjectAssign,
   ObjectCreate,
+  ObjectDefineProperty,
   ObjectSetPrototypeOf,
   PromiseAll,
+  RangeError,
   RegExpPrototypeExec,
   SafeArrayIterator,
   SafeWeakMap,
+  StringPrototypeSlice,
   StringPrototypeStartsWith,
+  StringPrototypeToUpperCase,
   globalThis,
 } = primordials;
 const { MessageChannel } = require('internal/worker/io');
@@ -96,6 +100,59 @@ const {
 
 let emittedSpecifierResolutionWarning = false;
 
+function nextHookFactory(chain, meta, validate) {
+  // First, prepare the current
+  const { hookName } = meta;
+  const {
+    fn: hook,
+    url: hookFilePath,
+  } = chain[meta.hookIndex];
+
+  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 RangeError(
+        `ESM custom loader '${hookName}' advanced beyond the end of the chain; this is a bug in Node.js itself.`
+      );
+    };
+  }
+
+  return ObjectDefineProperty(
+    async (...args) => {
+      // Update only when hook is invoked to avoid fingering the wrong filePath
+      meta.hookErrIdentifier = `${hookFilePath} '${hookName}'`;
+
+      validate(meta.hookErrIdentifier, ...args);
+
+      // Set when next<HookName> is actually called, not just generated.
+      if (generatedHookIndex === 0) { meta.chainFinished = true; }
+
+      const output = await hook(...args, nextNextHook);
+
+      if (output?.shortCircuit === true) { meta.shortCircuited = true; }
+
+      return output;
+    },
+    'name',
+    { 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
@@ -528,27 +585,17 @@ 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 hookIndex = chain.length - 1;
+    const meta = {
+      chainFinished: hookIndex === 0,
+      hookErrIdentifier: '',
+      hookIndex,
+      hookName: 'load',
+      shortCircuited: false,
+    };
 
+    const validate = (hookErrIdentifier, nextUrl, ctx) => {
       if (typeof nextUrl !== 'string') {
         // non-strings can be coerced to a url string
         // validateString() throws a less-specific error
@@ -565,29 +612,20 @@ class ESMLoader {
           new URL(nextUrl);
         } catch {
           throw new ERR_INVALID_ARG_VALUE(
-            `${hookErrIdentifier} nextLoad(url)`,
+            `${hookErrIdentifier}s nextLoad(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}s nextLoad(, 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(
@@ -597,10 +635,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 {
@@ -769,55 +807,35 @@ 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 hookIndex = chain.length - 1;
+    const meta = {
+      chainFinished: hookIndex === 0,
+      hookErrIdentifier: '',
+      hookIndex,
+      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, suppliedSpecifier, ctx) => {
       validateString(
         suppliedSpecifier,
-        `${hookErrIdentifier} nextResolve(specifier)`,
+        `${hookErrIdentifier}s nextResolve(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}s nextResolve(, 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(
@@ -827,10 +845,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
@@ -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);