fixup: support detecting when a resolve hook returns a promise

nodejs · Jun 12, 2022 · 8bf7965 · 8bf7965
1 parent ae8ef63
commit 8bf7965
Show file tree

Hide file tree

Showing 3 changed files with 30 additions and 15 deletions.
diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js
@@ -164,8 +164,22 @@ function nextHookFactory(chain, meta, validate) {
         if (output?.shortCircuit === true) { meta.shortCircuited = true; }
       }
 
-      if (output instanceof Promise) { // eslint-disable-line node-core/prefer-primordials
-        output?.then(checkShortCircuited);
+      const then = output?.then;
+      if (typeof then === 'function') {
+        if (!meta.isChainAsync) {
+          throw ERR_INVALID_RETURN_VALUE(
+            'an object',
+            // MUST use generatedHookIndex because the chain has already advanced,
+            // causing meta.hookIndex to advance
+            `${chain[generatedHookIndex].url} '${hookName}' hook's ${nextHookName}()`,
+            output,
+          );
+        }
+
+        ReflectApply(then, output, [
+          checkShortCircuited,
+          // TODO: handle error case
+        ]);
       } else {
         checkShortCircuited(output);
       }
@@ -568,6 +582,7 @@ class ESMLoader {
       hookErrIdentifier: '',
       hookIndex: chain.length - 1,
       hookName: 'load',
+      isChainAsync: true,
       shortCircuited: false,
     };
 
@@ -813,6 +828,7 @@ class ESMLoader {
       hookErrIdentifier: '',
       hookIndex: chain.length - 1,
       hookName: 'resolve',
+      isChainAsync: false,
       shortCircuited: false,
     };
     const context = {
@@ -822,14 +838,6 @@ class ESMLoader {
     };
 
     const validate = (hookErrIdentifier, output) => {
-      if (output instanceof Promise) { // eslint-disable-line node-core/prefer-primordials
-        throw ERR_INVALID_RETURN_VALUE(
-          'an object',
-          hookErrIdentifier,
-          output,
-        );
-      }
-
       const { 0: suppliedSpecifier, 1: ctx } = output;
 
       validateString(
@@ -847,7 +855,7 @@ class ESMLoader {
 
     if (
       typeof resolution !== 'object' || // [2]
-      resolution instanceof Promise // eslint-disable-line node-core/prefer-primordials
+      typeof resolution?.then === 'function'
     ) {
       throw new ERR_INVALID_RETURN_VALUE(
         'an object',

diff --git a/test/es-module/test-esm-loader-chaining.mjs b/test/es-module/test-esm-loader-chaining.mjs
@@ -166,11 +166,17 @@ const commonArgs = [
 }
 
 { // Verify error thrown for an async resolve hook
-  const { status, stderr, stdout } = spawnSync(
+  const { status, stderr } = 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-resolve-async-fn.mjs'),
+      '--loader',
+      fixtures.fileURL('es-module-loaders', 'loader-load-foo-or-42.mjs'),
       ...commonArgs,
     ],
     { encoding: 'utf8' },
@@ -180,7 +186,8 @@ const commonArgs = [
   assert.match(stderr, /Promise/);
   assert.match(stderr, /loader-resolve-async-fn\.mjs/);
   assert.match(stderr, /'resolve'/);
-  assert.strictEqual(stdout, '');
+  // Cannot expect stdout to be empty because detecting whether a hook has
+  // returned a promise requires the hook to be executed
   assert.strictEqual(status, 1);
 }
 

diff --git a/test/fixtures/es-module-loaders/loader-resolve-async-fn.mjs b/test/fixtures/es-module-loaders/loader-resolve-async-fn.mjs
@@ -1,3 +1,3 @@
-export async function resolve() {
-	return 'whatever';
+export async function resolve(specifier, context, next) {
+  return next(specifier, context);
 }