From 581d8a876004fbfc3d6c15485231a947d3095410 Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Fri, 24 Jun 2022 00:06:08 +0200 Subject: [PATCH 1/3] esm: restore `next`'s `context` as optional arg --- doc/api/esm.md | 12 +-- lib/internal/modules/esm/loader.js | 83 ++++++++++++------- .../es-module-loaders/loader-resolve-42.mjs | 2 +- 3 files changed, 61 insertions(+), 36 deletions(-) diff --git a/doc/api/esm.md b/doc/api/esm.md index d01b9c09cdfdb0..0fa6a37d89b6a1 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -815,7 +815,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); } ``` @@ -910,7 +910,7 @@ export async function load(url, context, nextLoad) { } // Defer to the next hook in the chain. - return nextLoad(url, context); + return nextLoad(url); } ``` @@ -1026,7 +1026,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) { @@ -1049,7 +1049,7 @@ export function load(url, context, nextLoad) { } // Let Node.js handle all other URLs. - return nextLoad(url, context); + return nextLoad(url); } ``` @@ -1102,7 +1102,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) { @@ -1143,7 +1143,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 index f81ffa8e31cf22..10f53ba6851f5d 100644 --- 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(...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 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') { // [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; } @@ -797,41 +815,48 @@ 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' || // [2] + output === null || + (output.url == null && typeof output.then === 'function') + ) { + 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/fixtures/es-module-loaders/loader-resolve-42.mjs b/test/fixtures/es-module-loaders/loader-resolve-42.mjs index 5c90bceed2b07e..eaca111998ae23 100644 --- 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:', next.name); // This log is deliberate - return next('file:///42.mjs', context); + return next('file:///42.mjs'); } From d51085260652096f4259612de5aba517c6aae954 Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Fri, 24 Jun 2022 11:38:54 +0200 Subject: [PATCH 2/3] fixup: add test case for `null` hook returns --- lib/internal/modules/esm/loader.js | 14 +++--- test/es-module/test-esm-loader-chaining.mjs | 46 ++++++++++++++++++- .../loader-load-null-return.mjs | 3 ++ .../loader-resolve-null-return.mjs | 3 ++ 4 files changed, 56 insertions(+), 10 deletions(-) create mode 100644 test/fixtures/es-module-loaders/loader-load-null-return.mjs create mode 100644 test/fixtures/es-module-loaders/loader-resolve-null-return.mjs diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 10f53ba6851f5d..d4f2bc7f5b5b01 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -94,6 +94,8 @@ const { getOptionValue } = require('internal/options'); let emittedSpecifierResolutionWarning = false; +const nullTypeForErr = { constructor: { name: 'null' } }; + /** * A utility function to iterate through a hook chain, track advancement in the * chain, and generate and supply the `next` argument to the custom @@ -602,11 +604,11 @@ class ESMLoader { if (ctx) validateObject(ctx, `${hookErrIdentifier} context`); }; const validateOutput = (hookErrIdentifier, output) => { - if (typeof output !== 'object') { // [2] + if (typeof output !== 'object' || output === null) { // [2] throw new ERR_INVALID_RETURN_VALUE( 'an object', hookErrIdentifier, - output, + output === null ? nullTypeForErr : output, ); } }; @@ -838,15 +840,11 @@ class ESMLoader { if (ctx) validateObject(ctx, `${hookErrIdentifier} context`); }; const validateOutput = (hookErrIdentifier, output) => { - if ( - typeof output !== 'object' || // [2] - output === null || - (output.url == null && typeof output.then === 'function') - ) { + if (typeof output !== 'object' || output === null) { // [2] throw new ERR_INVALID_RETURN_VALUE( 'an object', hookErrIdentifier, - output, + output === null ? nullTypeForErr : output, ); } }; diff --git a/test/es-module/test-esm-loader-chaining.mjs b/test/es-module/test-esm-loader-chaining.mjs index 977c20dbbbab37..5b8bc65eeeec33 100644 --- 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, /instance of 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, /instance of 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 new file mode 100644 index 00000000000000..252eec4ebc28a9 --- /dev/null +++ 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-null-return.mjs b/test/fixtures/es-module-loaders/loader-resolve-null-return.mjs new file mode 100644 index 00000000000000..16c9826a8ba51e --- /dev/null +++ b/test/fixtures/es-module-loaders/loader-resolve-null-return.mjs @@ -0,0 +1,3 @@ +export async function resolve(specifier, context, next) { + return null; +} From cb738d69e7c463dd603eaa73e3e23af19d888d48 Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Fri, 24 Jun 2022 12:13:37 +0200 Subject: [PATCH 3/3] fixup: switch back to regular `value` arg to error --- lib/internal/modules/esm/loader.js | 6 ++---- test/es-module/test-esm-loader-chaining.mjs | 4 ++-- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index d4f2bc7f5b5b01..223fd68a79fd00 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -94,8 +94,6 @@ const { getOptionValue } = require('internal/options'); let emittedSpecifierResolutionWarning = false; -const nullTypeForErr = { constructor: { name: 'null' } }; - /** * A utility function to iterate through a hook chain, track advancement in the * chain, and generate and supply the `next` argument to the custom @@ -608,7 +606,7 @@ class ESMLoader { throw new ERR_INVALID_RETURN_VALUE( 'an object', hookErrIdentifier, - output === null ? nullTypeForErr : output, + output, ); } }; @@ -844,7 +842,7 @@ class ESMLoader { throw new ERR_INVALID_RETURN_VALUE( 'an object', hookErrIdentifier, - output === null ? nullTypeForErr : output, + output, ); } }; diff --git a/test/es-module/test-esm-loader-chaining.mjs b/test/es-module/test-esm-loader-chaining.mjs index 5b8bc65eeeec33..f1ea13495ca5c4 100644 --- a/test/es-module/test-esm-loader-chaining.mjs +++ b/test/es-module/test-esm-loader-chaining.mjs @@ -332,7 +332,7 @@ const commonArgs = [ assert.match(stderr, /loader-resolve-null-return\.mjs/); assert.match(stderr, /'resolve' hook's nextResolve\(\)/); assert.match(stderr, /an object/); - assert.match(stderr, /instance of null/); + assert.match(stderr, /got null/); } { // Verify error thrown when invalid `context` argument passed to `nextResolve` @@ -372,7 +372,7 @@ const commonArgs = [ assert.match(stderr, /loader-load-null-return\.mjs/); assert.match(stderr, /'load' hook's nextLoad\(\)/); assert.match(stderr, /an object/); - assert.match(stderr, /instance of null/); + assert.match(stderr, /got null/); } { // Verify error thrown when invalid `url` argument passed to `nextLoad`