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..223fd68a79fd00 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' || 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; } @@ -797,41 +815,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 index 977c20dbbbab37..f1ea13495ca5c4 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, /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 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-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'); } 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; +}