esm: restore next<HookName>'s context as optional arg

PR-URL: #43553 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs · Jul 20, 2022 · 1cda175 · 1cda175
1 parent 2d31695
commit 1cda175
Show file tree

Hide file tree

Showing 6 changed files with 107 additions and 38 deletions.
diff --git a/doc/api/esm.md b/doc/api/esm.md
@@ -806,7 +806,7 @@ export async function resolve(specifier, context, nextResolve) {
 
   // Defer to the next hook in the chain, which would be the
   // Node.js default resolve if this is the last user-specified loader.
-  return nextResolve(specifier, context);
+  return nextResolve(specifier);
 }
 ```
 
@@ -901,7 +901,7 @@ export async function load(url, context, nextLoad) {
   }
 
   // Defer to the next hook in the chain.
-  return nextLoad(url, context);
+  return nextLoad(url);
 }
 ```
 
@@ -1017,7 +1017,7 @@ export function resolve(specifier, context, nextResolve) {
   }
 
   // Let Node.js handle all other specifiers.
-  return nextResolve(specifier, context);
+  return nextResolve(specifier);
 }
 
 export function load(url, context, nextLoad) {
@@ -1040,7 +1040,7 @@ export function load(url, context, nextLoad) {
   }
 
   // Let Node.js handle all other URLs.
-  return nextLoad(url, context);
+  return nextLoad(url);
 }
 ```
 
@@ -1093,7 +1093,7 @@ export async function resolve(specifier, context, nextResolve) {
   }
 
   // Let Node.js handle all other specifiers.
-  return nextResolve(specifier, context);
+  return nextResolve(specifier);
 }
 
 export async function load(url, context, nextLoad) {
@@ -1134,7 +1134,7 @@ export async function load(url, context, nextLoad) {
   }
 
   // Let Node.js handle all other URLs.
-  return nextLoad(url, context);
+  return nextLoad(url);
 }
 
 async function getPackageType(url) {

diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js
@@ -114,7 +114,7 @@ let emittedSpecifierResolutionWarning = false;
  *  validation within MUST throw.
  * @returns {function next<HookName>(...hookArgs)} The next hook in the chain.
  */
-function nextHookFactory(chain, meta, validate) {
+function nextHookFactory(chain, meta, { validateArgs, validateOutput }) {
   // First, prepare the current
   const { hookName } = meta;
   const {
@@ -137,7 +137,7 @@ function nextHookFactory(chain, meta, validate) {
     // factory generates the next link in the chain.
     meta.hookIndex--;
 
-    nextNextHook = nextHookFactory(chain, meta, validate);
+    nextNextHook = nextHookFactory(chain, meta, { validateArgs, validateOutput });
   } else {
     // eslint-disable-next-line func-name-matching
     nextNextHook = function chainAdvancedTooFar() {
@@ -152,14 +152,28 @@ function nextHookFactory(chain, meta, validate) {
       // Update only when hook is invoked to avoid fingering the wrong filePath
       meta.hookErrIdentifier = `${hookFilePath} '${hookName}'`;
 
-      validate(`${meta.hookErrIdentifier} hook's ${nextHookName}()`, args);
+      validateArgs(`${meta.hookErrIdentifier} hook's ${nextHookName}()`, args);
+
+      const outputErrIdentifier = `${chain[generatedHookIndex].url} '${hookName}' hook's ${nextHookName}()`;
 
       // Set when next<HookName> is actually called, not just generated.
       if (generatedHookIndex === 0) { meta.chainFinished = true; }
 
+      // `context` is an optional argument that only needs to be passed when changed
+      switch (args.length) {
+        case 1: // It was omitted, so supply the cached value
+          ArrayPrototypePush(args, meta.context);
+          break;
+        case 2: // Overrides were supplied, so update cached value
+          ObjectAssign(meta.context, args[1]);
+          break;
+      }
+
       ArrayPrototypePush(args, nextNextHook);
       const output = await ReflectApply(hook, undefined, args);
 
+      validateOutput(outputErrIdentifier, output);
+
       if (output?.shortCircuit === true) { meta.shortCircuited = true; }
       return output;
 
@@ -554,13 +568,14 @@ class ESMLoader {
     const chain = this.#loaders;
     const meta = {
       chainFinished: null,
+      context,
       hookErrIdentifier: '',
       hookIndex: chain.length - 1,
       hookName: 'load',
       shortCircuited: false,
     };
 
-    const validate = (hookErrIdentifier, { 0: nextUrl, 1: ctx }) => {
+    const validateArgs = (hookErrIdentifier, { 0: nextUrl, 1: ctx }) => {
       if (typeof nextUrl !== 'string') {
         // non-strings can be coerced to a url string
         // validateString() throws a less-specific error
@@ -584,21 +599,24 @@ class ESMLoader {
         }
       }
 
-      validateObject(ctx, `${hookErrIdentifier} context`);
+      if (ctx) validateObject(ctx, `${hookErrIdentifier} context`);
+    };
+    const validateOutput = (hookErrIdentifier, output) => {
+      if (typeof output !== 'object' || output === null) { // [2]
+        throw new ERR_INVALID_RETURN_VALUE(
+          'an object',
+          hookErrIdentifier,
+          output,
+        );
+      }
     };
 
-    const nextLoad = nextHookFactory(chain, meta, validate);
+    const nextLoad = nextHookFactory(chain, meta, { validateArgs, validateOutput });
 
     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(
-        'an object',
-        hookErrIdentifier,
-        loaded,
-      );
-    }
+    validateOutput(hookErrIdentifier, loaded);
 
     if (loaded?.shortCircuit === true) { meta.shortCircuited = true; }
 
@@ -796,41 +814,44 @@ class ESMLoader {
       );
     }
     const chain = this.#resolvers;
+    const context = {
+      conditions: DEFAULT_CONDITIONS,
+      importAssertions,
+      parentURL,
+    };
     const meta = {
       chainFinished: null,
+      context,
       hookErrIdentifier: '',
       hookIndex: chain.length - 1,
       hookName: 'resolve',
       shortCircuited: false,
     };
 
-    const context = {
-      conditions: DEFAULT_CONDITIONS,
-      importAssertions,
-      parentURL,
-    };
-    const validate = (hookErrIdentifier, { 0: suppliedSpecifier, 1: ctx }) => {
-
+    const validateArgs = (hookErrIdentifier, { 0: suppliedSpecifier, 1: ctx }) => {
       validateString(
         suppliedSpecifier,
         `${hookErrIdentifier} specifier`,
       ); // non-strings can be coerced to a url string
 
-      validateObject(ctx, `${hookErrIdentifier} context`);
+      if (ctx) validateObject(ctx, `${hookErrIdentifier} context`);
+    };
+    const validateOutput = (hookErrIdentifier, output) => {
+      if (typeof output !== 'object' || output === null) { // [2]
+        throw new ERR_INVALID_RETURN_VALUE(
+          'an object',
+          hookErrIdentifier,
+          output,
+        );
+      }
     };
 
-    const nextResolve = nextHookFactory(chain, meta, validate);
+    const nextResolve = nextHookFactory(chain, meta, { validateArgs, validateOutput });
 
     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(
-        'an object',
-        hookErrIdentifier,
-        resolution,
-      );
-    }
+    validateOutput(hookErrIdentifier, resolution);
 
     if (resolution?.shortCircuit === true) { meta.shortCircuited = true; }
 

diff --git a/test/es-module/test-esm-loader-chaining.mjs b/test/es-module/test-esm-loader-chaining.mjs
@@ -229,7 +229,7 @@ const commonArgs = [
   assert.strictEqual(status, 0);
 }
 
-{ // Verify chain does break and throws appropriately
+{ // Verify resolve chain does break and throws appropriately
   const { status, stderr, stdout } = spawnSync(
     process.execPath,
     [
@@ -273,7 +273,7 @@ const commonArgs = [
   assert.strictEqual(status, 1);
 }
 
-{ // Verify chain does break and throws appropriately
+{ // Verify load chain does break and throws appropriately
   const { status, stderr, stdout } = spawnSync(
     process.execPath,
     [
@@ -314,6 +314,27 @@ const commonArgs = [
   assert.match(stderr, /'resolve' hook's nextResolve\(\) specifier/);
 }
 
+{ // Verify error thrown when resolve hook is invalid
+  const { status, stderr } = spawnSync(
+    process.execPath,
+    [
+      '--loader',
+      fixtures.fileURL('es-module-loaders', 'loader-resolve-passthru.mjs'),
+      '--loader',
+      fixtures.fileURL('es-module-loaders', 'loader-resolve-null-return.mjs'),
+      ...commonArgs,
+    ],
+    { encoding: 'utf8' },
+  );
+
+  assert.strictEqual(status, 1);
+  assert.match(stderr, /ERR_INVALID_RETURN_VALUE/);
+  assert.match(stderr, /loader-resolve-null-return\.mjs/);
+  assert.match(stderr, /'resolve' hook's nextResolve\(\)/);
+  assert.match(stderr, /an object/);
+  assert.match(stderr, /got null/);
+}
+
 { // Verify error thrown when invalid `context` argument passed to `nextResolve`
   const { status, stderr } = spawnSync(
     process.execPath,
@@ -333,6 +354,27 @@ const commonArgs = [
   assert.strictEqual(status, 1);
 }
 
+{ // Verify error thrown when load hook is invalid
+  const { status, stderr } = spawnSync(
+    process.execPath,
+    [
+      '--loader',
+      fixtures.fileURL('es-module-loaders', 'loader-load-passthru.mjs'),
+      '--loader',
+      fixtures.fileURL('es-module-loaders', 'loader-load-null-return.mjs'),
+      ...commonArgs,
+    ],
+    { encoding: 'utf8' },
+  );
+
+  assert.strictEqual(status, 1);
+  assert.match(stderr, /ERR_INVALID_RETURN_VALUE/);
+  assert.match(stderr, /loader-load-null-return\.mjs/);
+  assert.match(stderr, /'load' hook's nextLoad\(\)/);
+  assert.match(stderr, /an object/);
+  assert.match(stderr, /got null/);
+}
+
 { // Verify error thrown when invalid `url` argument passed to `nextLoad`
   const { status, stderr } = spawnSync(
     process.execPath,

diff --git a/test/fixtures/es-module-loaders/loader-load-null-return.mjs b/test/fixtures/es-module-loaders/loader-load-null-return.mjs
@@ -0,0 +1,3 @@
+export async function load(specifier, context, next) {
+  return null;
+}
diff --git a/test/fixtures/es-module-loaders/loader-resolve-42.mjs b/test/fixtures/es-module-loaders/loader-resolve-42.mjs
@@ -2,5 +2,5 @@ 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);
+  return next('file:///42.mjs');
 }
diff --git a/test/fixtures/es-module-loaders/loader-resolve-null-return.mjs b/test/fixtures/es-module-loaders/loader-resolve-null-return.mjs
@@ -0,0 +1,3 @@
+export async function resolve(specifier, context, next) {
+  return null;
+}