From 335a943635f35f5967e749d3c0e9e1d1ca7fa329 Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Sun, 5 Jun 2022 19:33:46 +0200 Subject: [PATCH] esm: fix chain advances when loader calls next multiple times PR-URL: https://github.com/nodejs/node/pull/43303 Reviewed-By: Geoffrey Booth Reviewed-By: Antoine du Hamel --- lib/internal/errors.js | 2 +- lib/internal/modules/esm/loader.js | 191 +++++++++++------- lib/internal/modules/esm/resolve.js | 2 +- test/es-module/test-esm-loader-chaining.mjs | 101 ++++++--- .../es-module-loaders/loader-resolve-42.mjs | 2 + .../loader-resolve-multiple-next-calls.mjs | 9 + 6 files changed, 198 insertions(+), 109 deletions(-) create mode 100644 test/fixtures/es-module-loaders/loader-resolve-multiple-next-calls.mjs diff --git a/lib/internal/errors.js b/lib/internal/errors.js index cf54bbe6fb35e1..306dcbad998134 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1372,7 +1372,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 index c16f9122f1d0e6..f81ffa8e31cf22 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -12,17 +12,22 @@ const { FunctionPrototypeCall, ObjectAssign, ObjectCreate, + ObjectDefineProperty, ObjectSetPrototypeOf, PromiseAll, + ReflectApply, RegExpPrototypeExec, SafeArrayIterator, SafeWeakMap, + StringPrototypeSlice, + StringPrototypeToUpperCase, globalThis, } = primordials; const { MessageChannel } = require('internal/worker/io'); const { ERR_LOADER_CHAIN_INCOMPLETE, + ERR_INTERNAL_ASSERTION, ERR_INVALID_ARG_TYPE, ERR_INVALID_ARG_VALUE, ERR_INVALID_RETURN_PROPERTY_VALUE, @@ -89,6 +94,81 @@ const { getOptionValue } = require('internal/options'); let emittedSpecifierResolutionWarning = false; +/** + * A utility function to iterate through a hook chain, track advancement in the + * chain, and generate and supply the `next` argument to the custom + * hook. + * @param {KeyedHook[]} chain The whole hook chain. + * @param {object} meta Properties that change as the current hook advances + * along the chain. + * @param {boolean} meta.chainFinished Whether the end of the chain has been + * reached AND invoked. + * @param {string} meta.hookErrIdentifier A user-facing identifier to help + * pinpoint where an error occurred. Ex "file:///foo.mjs 'resolve'". + * @param {number} meta.hookIndex A non-negative integer tracking the current + * position in the hook chain. + * @param {string} meta.hookName The kind of hook the chain is (ex 'resolve') + * @param {boolean} meta.shortCircuited Whether a hook signaled a short-circuit. + * @param {(hookErrIdentifier, hookArgs) => void} validate A wrapper function + * containing all validation of a custom loader hook's intermediary output. Any + * validation within MUST throw. + * @returns {function next(...hookArgs)} The next hook in the chain. + */ +function nextHookFactory(chain, meta, validate) { + // First, prepare the current + const { hookName } = meta; + const { + fn: hook, + url: hookFilePath, + } = chain[meta.hookIndex]; + + // ex 'nextResolve' + 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 ERR_INTERNAL_ASSERTION( + `ESM custom loader '${hookName}' advanced beyond the end of the chain.` + ); + }; + } + + return ObjectDefineProperty( + async (...args) => { + // Update only when hook is invoked to avoid fingering the wrong filePath + meta.hookErrIdentifier = `${hookFilePath} '${hookName}'`; + + validate(`${meta.hookErrIdentifier} hook's ${nextHookName}()`, args); + + // Set when next is actually called, not just generated. + if (generatedHookIndex === 0) { meta.chainFinished = true; } + + ArrayPrototypePush(args, nextNextHook); + const output = await ReflectApply(hook, undefined, args); + + if (output?.shortCircuit === true) { meta.shortCircuited = true; } + return output; + + }, + 'name', + { __proto__: null, 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 @@ -471,32 +551,21 @@ 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 meta = { + chainFinished: null, + hookErrIdentifier: '', + hookIndex: chain.length - 1, + hookName: 'load', + shortCircuited: false, + }; + const validate = (hookErrIdentifier, { 0: nextUrl, 1: ctx }) => { if (typeof nextUrl !== 'string') { // non-strings can be coerced to a url string // validateString() throws a less-specific error throw new ERR_INVALID_ARG_TYPE( - `${hookErrIdentifier} nextLoad(url)`, + `${hookErrIdentifier} url`, 'a url string', nextUrl, ); @@ -508,29 +577,20 @@ class ESMLoader { new URL(nextUrl); } catch { throw new ERR_INVALID_ARG_VALUE( - `${hookErrIdentifier} nextLoad(url)`, + `${hookErrIdentifier} 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} 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( @@ -540,10 +600,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 { @@ -736,55 +796,34 @@ 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 meta = { + chainFinished: null, + hookErrIdentifier: '', + hookIndex: chain.length - 1, + 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, { 0: suppliedSpecifier, 1: ctx }) => { validateString( suppliedSpecifier, - `${hookErrIdentifier} nextResolve(specifier)`, + `${hookErrIdentifier} 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} 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( @@ -794,10 +833,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 index 8767109d7c4177..79ea42233b66a0 100644 --- 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); diff --git a/test/es-module/test-esm-loader-chaining.mjs b/test/es-module/test-esm-loader-chaining.mjs index 1055c44a156a77..977c20dbbbab37 100644 --- a/test/es-module/test-esm-loader-chaining.mjs +++ b/test/es-module/test-esm-loader-chaining.mjs @@ -25,8 +25,8 @@ const commonArgs = [ ); assert.strictEqual(stderr, ''); - assert.strictEqual(status, 0); assert.match(stdout, /number/); // node:fs is an object + assert.strictEqual(status, 0); } { // Verify loaded source is properly different when only load changes something @@ -47,8 +47,8 @@ const commonArgs = [ assert.strictEqual(stderr, ''); assert.match(stdout, /load passthru/); assert.match(stdout, /resolve passthru/); - assert.strictEqual(status, 0); assert.match(stdout, /foo/); + assert.strictEqual(status, 0); } { // Verify multiple changes from hooks result in proper output @@ -70,8 +70,8 @@ const commonArgs = [ assert.strictEqual(stderr, ''); assert.match(stdout, /resolve 42/); // It did go thru resolve-42 - assert.strictEqual(status, 0); assert.match(stdout, /foo/); // LIFO, so resolve-foo won + assert.strictEqual(status, 0); } { // Verify modifying context within resolve chain is respected @@ -117,8 +117,52 @@ const commonArgs = [ assert.strictEqual(stderr, ''); assert.match(stdout, /resolve foo/); // It did go thru resolve-foo - assert.strictEqual(status, 0); assert.match(stdout, /42/); // LIFO, so resolve-42 won + assert.strictEqual(status, 0); +} + +{ // Verify multiple calls to next within same loader receive correct "next" fn + const { status, stderr, stdout } = spawnSync( + process.execPath, + [ + '--loader', + fixtures.fileURL('es-module-loaders', 'loader-resolve-shortcircuit.mjs'), + '--loader', + fixtures.fileURL('es-module-loaders', 'loader-resolve-foo.mjs'), + '--loader', + fixtures.fileURL('es-module-loaders', 'loader-resolve-multiple-next-calls.mjs'), + '--loader', + fixtures.fileURL('es-module-loaders', 'loader-load-foo-or-42.mjs'), + ...commonArgs, + ], + { encoding: 'utf8' }, + ); + + const countFoos = stdout.match(/resolve foo/g)?.length; + + assert.strictEqual(stderr, ''); + assert.strictEqual(countFoos, 2); + assert.strictEqual(status, 0); +} + +{ // Verify next function's `name` is correct + const { status, stderr, stdout } = 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-load-foo-or-42.mjs'), + ...commonArgs, + ], + { encoding: 'utf8' }, + ); + + assert.strictEqual(stderr, ''); + assert.match(stdout, /next: nextResolve/); + assert.strictEqual(status, 0); } { // Verify error thrown for incomplete resolve chain, citing errant loader & hook @@ -137,10 +181,10 @@ const commonArgs = [ ); assert.match(stdout, /resolve passthru/); - assert.strictEqual(status, 1); assert.match(stderr, /ERR_LOADER_CHAIN_INCOMPLETE/); assert.match(stderr, /loader-resolve-incomplete\.mjs/); - assert.match(stderr, /"resolve"/); + assert.match(stderr, /'resolve'/); + assert.strictEqual(status, 1); } { // Verify error NOT thrown when nested resolve hook signaled a short circuit @@ -201,10 +245,10 @@ const commonArgs = [ ); assert.doesNotMatch(stdout, /resolve passthru/); - assert.strictEqual(status, 1); assert.match(stderr, /ERR_LOADER_CHAIN_INCOMPLETE/); assert.match(stderr, /loader-resolve-incomplete\.mjs/); - assert.match(stderr, /"resolve"/); + assert.match(stderr, /'resolve'/); + assert.strictEqual(status, 1); } { // Verify error thrown for incomplete load chain, citing errant loader & hook @@ -223,10 +267,10 @@ const commonArgs = [ ); assert.match(stdout, /load passthru/); - assert.strictEqual(status, 1); assert.match(stderr, /ERR_LOADER_CHAIN_INCOMPLETE/); assert.match(stderr, /loader-load-incomplete\.mjs/); - assert.match(stderr, /"load"/); + assert.match(stderr, /'load'/); + assert.strictEqual(status, 1); } { // Verify chain does break and throws appropriately @@ -245,13 +289,13 @@ const commonArgs = [ ); assert.doesNotMatch(stdout, /load passthru/); - assert.strictEqual(status, 1); assert.match(stderr, /ERR_LOADER_CHAIN_INCOMPLETE/); assert.match(stderr, /loader-load-incomplete\.mjs/); - assert.match(stderr, /"load"/); + assert.match(stderr, /'load'/); + assert.strictEqual(status, 1); } -{ // Verify error thrown when invalid `specifier` argument passed to `resolve…next` +{ // Verify error thrown when invalid `specifier` argument passed to `nextResolve` const { status, stderr } = spawnSync( process.execPath, [ @@ -267,11 +311,10 @@ const commonArgs = [ assert.strictEqual(status, 1); assert.match(stderr, /ERR_INVALID_ARG_TYPE/); assert.match(stderr, /loader-resolve-bad-next-specifier\.mjs/); - assert.match(stderr, /"resolve"/); - assert.match(stderr, /nextResolve\(specifier\)/); + assert.match(stderr, /'resolve' hook's nextResolve\(\) specifier/); } -{ // Verify error thrown when invalid `context` argument passed to `resolve…next` +{ // Verify error thrown when invalid `context` argument passed to `nextResolve` const { status, stderr } = spawnSync( process.execPath, [ @@ -284,14 +327,13 @@ const commonArgs = [ { encoding: 'utf8' }, ); - assert.strictEqual(status, 1); assert.match(stderr, /ERR_INVALID_ARG_TYPE/); assert.match(stderr, /loader-resolve-bad-next-context\.mjs/); - assert.match(stderr, /"resolve"/); - assert.match(stderr, /nextResolve\(, context\)/); + assert.match(stderr, /'resolve' hook's nextResolve\(\) context/); + assert.strictEqual(status, 1); } -{ // Verify error thrown when invalid `url` argument passed to `load…next` +{ // Verify error thrown when invalid `url` argument passed to `nextLoad` const { status, stderr } = spawnSync( process.execPath, [ @@ -304,14 +346,13 @@ const commonArgs = [ { encoding: 'utf8' }, ); - assert.strictEqual(status, 1); assert.match(stderr, /ERR_INVALID_ARG_TYPE/); assert.match(stderr, /loader-load-bad-next-url\.mjs/); - assert.match(stderr, /"load"/); - assert.match(stderr, /nextLoad\(url\)/); + assert.match(stderr, /'load' hook's nextLoad\(\) url/); + assert.strictEqual(status, 1); } -{ // Verify error thrown when invalid `url` argument passed to `load…next` +{ // Verify error thrown when invalid `url` argument passed to `nextLoad` const { status, stderr } = spawnSync( process.execPath, [ @@ -324,14 +365,13 @@ const commonArgs = [ { encoding: 'utf8' }, ); - assert.strictEqual(status, 1); assert.match(stderr, /ERR_INVALID_ARG_VALUE/); assert.match(stderr, /loader-load-impersonating-next-url\.mjs/); - assert.match(stderr, /"load"/); - assert.match(stderr, /nextLoad\(url\)/); + assert.match(stderr, /'load' hook's nextLoad\(\) url/); + assert.strictEqual(status, 1); } -{ // Verify error thrown when invalid `context` argument passed to `load…next` +{ // Verify error thrown when invalid `context` argument passed to `nextLoad` const { status, stderr } = spawnSync( process.execPath, [ @@ -344,9 +384,8 @@ const commonArgs = [ { encoding: 'utf8' }, ); - assert.strictEqual(status, 1); assert.match(stderr, /ERR_INVALID_ARG_TYPE/); assert.match(stderr, /loader-load-bad-next-context\.mjs/); - assert.match(stderr, /"load"/); - assert.match(stderr, /nextLoad\(, context\)/); + assert.match(stderr, /'load' hook's nextLoad\(\) context/); + assert.strictEqual(status, 1); } diff --git a/test/fixtures/es-module-loaders/loader-resolve-42.mjs b/test/fixtures/es-module-loaders/loader-resolve-42.mjs index f4dffd70fc94ad..5c90bceed2b07e 100644 --- a/test/fixtures/es-module-loaders/loader-resolve-42.mjs +++ b/test/fixtures/es-module-loaders/loader-resolve-42.mjs @@ -1,4 +1,6 @@ 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); } diff --git a/test/fixtures/es-module-loaders/loader-resolve-multiple-next-calls.mjs b/test/fixtures/es-module-loaders/loader-resolve-multiple-next-calls.mjs new file mode 100644 index 00000000000000..88d333c2404a3c --- /dev/null +++ b/test/fixtures/es-module-loaders/loader-resolve-multiple-next-calls.mjs @@ -0,0 +1,9 @@ +export async function resolve(specifier, context, next) { + const { url: first } = await next(specifier, context); + const { url: second } = await next(specifier, context); + + return { + format: 'module', + url: first, + }; +}