nodejs
/
node
mirror of https://github.com/nodejs/node.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

esm: fix chain advances when loader calls next<HookName> multiple times 

PR-URL: https://github.com/nodejs/node/pull/43303
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
This commit is contained in:
Jacob Smith 2022-06-05 19:33:46 +02:00 committed by GitHub
parent 824c8b7e62
commit 765666a8d4
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 198 additions and 109 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
lib/internal/errors.js
View File

@ -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

191
lib/internal/modules/esm/loader.js
View File

@ -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<HookName>` 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<HookName>(...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<HookName> 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 {

2
lib/internal/modules/esm/resolve.js
View File

@ -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);

101
test/es-module/test-esm-loader-chaining.mjs
View File

@ -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<HookName> 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<HookName>: 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);
}

2
test/fixtures/es-module-loaders/loader-resolve-42.mjs vendored
View File

@ -1,4 +1,6 @@
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);
}

9
test/fixtures/es-module-loaders/loader-resolve-multiple-next-calls.mjs vendored Normal file
View File

@ -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,
};
}