Skip to content

Commit 3cf8f28

Browse files
esm: fix chain advances when loader calls next<HookName> multiple times
1 parent 8f39f51 commit 3cf8f28

File tree

5 files changed

+154
-103
lines changed

5 files changed

+154
-103
lines changed

lib/internal/errors.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1361,7 +1361,7 @@ E('ERR_IPC_ONE_PIPE', 'Child process can have only one IPC pipe', Error);
13611361
E('ERR_IPC_SYNC_FORK', 'IPC cannot be used with synchronous forks', Error);
13621362
E(
13631363
'ERR_LOADER_CHAIN_INCOMPLETE',
1364-
'The "%s" hook from %s did not call the next hook in its chain and did not' +
1364+
'"%s" did not call the next hook in its chain and did not' +
13651365
' explicitly signal a short circuit. If this is intentional, include' +
13661366
' `shortCircuit: true` in the hook\'s return.',
13671367
Error

lib/internal/modules/esm/loader.js

Lines changed: 93 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,16 @@ const {
1212
FunctionPrototypeCall,
1313
ObjectAssign,
1414
ObjectCreate,
15+
ObjectDefineProperty,
1516
ObjectSetPrototypeOf,
1617
PromiseAll,
18+
RangeError,
1719
RegExpPrototypeExec,
1820
SafeArrayIterator,
1921
SafeWeakMap,
22+
StringPrototypeSlice,
2023
StringPrototypeStartsWith,
24+
StringPrototypeToUpperCase,
2125
globalThis,
2226
} = primordials;
2327
const { MessageChannel } = require('internal/worker/io');
@@ -96,6 +100,59 @@ const {
96100

97101
let emittedSpecifierResolutionWarning = false;
98102

103+
function nextHookFactory(chain, meta, validate) {
104+
// First, prepare the current
105+
const { hookName } = meta;
106+
const {
107+
fn: hook,
108+
url: hookFilePath,
109+
} = chain[meta.hookIndex];
110+
111+
const nextHookName = `next${
112+
StringPrototypeToUpperCase(hookName[0]) +
113+
StringPrototypeSlice(hookName, 1)
114+
}`;
115+
116+
// When hookIndex is 0, it's reached the default, which does not call next()
117+
// so feed it a noop that blows up if called, so the problem is obvious.
118+
const generatedHookIndex = meta.hookIndex;
119+
let nextNextHook;
120+
if (meta.hookIndex > 0) {
121+
// Now, prepare the next: decrement the pointer so the next call to the
122+
// factory generates the next link in the chain.
123+
meta.hookIndex--;
124+
125+
nextNextHook = nextHookFactory(chain, meta, validate);
126+
} else {
127+
// eslint-disable-next-line func-name-matching
128+
nextNextHook = function chainAdvancedTooFar() {
129+
throw new RangeError(
130+
`ESM custom loader '${hookName}' advanced beyond the end of the chain; this is a bug in Node.js itself.`
131+
);
132+
};
133+
}
134+
135+
return ObjectDefineProperty(
136+
async (...args) => {
137+
// Update only when hook is invoked to avoid fingering the wrong filePath
138+
meta.hookErrIdentifier = `${hookFilePath} '${hookName}'`;
139+
140+
validate(meta.hookErrIdentifier, ...args);
141+
142+
// Set when next<HookName> is actually called, not just generated.
143+
if (generatedHookIndex === 0) { meta.chainFinished = true; }
144+
145+
const output = await hook(...args, nextNextHook);
146+
147+
if (output?.shortCircuit === true) { meta.shortCircuited = true; }
148+
149+
return output;
150+
},
151+
'name',
152+
{ value: nextHookName },
153+
);
154+
}
155+
99156
/**
100157
* An ESMLoader instance is used as the main entry point for loading ES modules.
101158
* Currently, this is a singleton -- there is only one used for loading
@@ -528,27 +585,17 @@ class ESMLoader {
528585
* @returns {{ format: ModuleFormat, source: ModuleSource }}
529586
*/
530587
async load(url, context = {}) {
531-
const loaders = this.#loaders;
532-
let hookIndex = loaders.length - 1;
533-
let {
534-
fn: loader,
535-
url: loaderFilePath,
536-
} = loaders[hookIndex];
537-
let chainFinished = hookIndex === 0;
538-
let shortCircuited = false;
539-
540-
const nextLoad = async (nextUrl, ctx = context) => {
541-
--hookIndex; // `nextLoad` has been called, so decrement our pointer.
542-
543-
({
544-
fn: loader,
545-
url: loaderFilePath,
546-
} = loaders[hookIndex]);
547-
548-
if (hookIndex === 0) { chainFinished = true; }
549-
550-
const hookErrIdentifier = `${loaderFilePath} "load"`;
588+
const chain = this.#loaders;
589+
const hookIndex = chain.length - 1;
590+
const meta = {
591+
chainFinished: hookIndex === 0,
592+
hookErrIdentifier: '',
593+
hookIndex,
594+
hookName: 'load',
595+
shortCircuited: false,
596+
};
551597

598+
const validate = (hookErrIdentifier, nextUrl, ctx) => {
552599
if (typeof nextUrl !== 'string') {
553600
// non-strings can be coerced to a url string
554601
// validateString() throws a less-specific error
@@ -565,29 +612,20 @@ class ESMLoader {
565612
new URL(nextUrl);
566613
} catch {
567614
throw new ERR_INVALID_ARG_VALUE(
568-
`${hookErrIdentifier} nextLoad(url)`,
615+
`${hookErrIdentifier}s nextLoad(url)`,
569616
nextUrl,
570617
'should be a url string',
571618
);
572619
}
573620
}
574621

575-
validateObject(ctx, `${hookErrIdentifier} nextLoad(, context)`);
576-
577-
const output = await loader(nextUrl, ctx, nextLoad);
578-
579-
if (output?.shortCircuit === true) { shortCircuited = true; }
580-
581-
return output;
622+
validateObject(ctx, `${hookErrIdentifier}s nextLoad(, context)`);
582623
};
583624

584-
const loaded = await loader(
585-
url,
586-
context,
587-
nextLoad,
588-
);
625+
const nextLoad = nextHookFactory(chain, meta, validate);
589626

590-
const hookErrIdentifier = `${loaderFilePath} load`;
627+
const loaded = await nextLoad(url, context);
628+
const { hookErrIdentifier } = meta; // Retrieve the value after all settled
591629

592630
if (typeof loaded !== 'object') { // [2]
593631
throw new ERR_INVALID_RETURN_VALUE(
@@ -597,10 +635,10 @@ class ESMLoader {
597635
);
598636
}
599637

600-
if (loaded?.shortCircuit === true) { shortCircuited = true; }
638+
if (loaded?.shortCircuit === true) { meta.shortCircuited = true; }
601639

602-
if (!chainFinished && !shortCircuited) {
603-
throw new ERR_LOADER_CHAIN_INCOMPLETE('load', loaderFilePath);
640+
if (!meta.chainFinished && !meta.shortCircuited) {
641+
throw new ERR_LOADER_CHAIN_INCOMPLETE(hookErrIdentifier);
604642
}
605643

606644
const {
@@ -769,55 +807,35 @@ class ESMLoader {
769807
parentURL,
770808
);
771809
}
772-
const resolvers = this.#resolvers;
773-
774-
let hookIndex = resolvers.length - 1;
775-
let {
776-
fn: resolver,
777-
url: resolverFilePath,
778-
} = resolvers[hookIndex];
779-
let chainFinished = hookIndex === 0;
780-
let shortCircuited = false;
810+
const chain = this.#resolvers;
811+
const hookIndex = chain.length - 1;
812+
const meta = {
813+
chainFinished: hookIndex === 0,
814+
hookErrIdentifier: '',
815+
hookIndex,
816+
hookName: 'resolve',
817+
shortCircuited: false,
818+
};
781819

782820
const context = {
783821
conditions: DEFAULT_CONDITIONS,
784822
importAssertions,
785823
parentURL,
786824
};
787825

788-
const nextResolve = async (suppliedSpecifier, ctx = context) => {
789-
--hookIndex; // `nextResolve` has been called, so decrement our pointer.
790-
791-
({
792-
fn: resolver,
793-
url: resolverFilePath,
794-
} = resolvers[hookIndex]);
795-
796-
if (hookIndex === 0) { chainFinished = true; }
797-
798-
const hookErrIdentifier = `${resolverFilePath} "resolve"`;
799-
826+
const validate = (hookErrIdentifier, suppliedSpecifier, ctx) => {
800827
validateString(
801828
suppliedSpecifier,
802-
`${hookErrIdentifier} nextResolve(specifier)`,
829+
`${hookErrIdentifier}s nextResolve(specifier)`,
803830
); // non-strings can be coerced to a url string
804831

805-
validateObject(ctx, `${hookErrIdentifier} nextResolve(, context)`);
806-
807-
const output = await resolver(suppliedSpecifier, ctx, nextResolve);
808-
809-
if (output?.shortCircuit === true) { shortCircuited = true; }
810-
811-
return output;
832+
validateObject(ctx, `${hookErrIdentifier}s nextResolve(, context)`);
812833
};
813834

814-
const resolution = await resolver(
815-
originalSpecifier,
816-
context,
817-
nextResolve,
818-
);
835+
const nextResolve = nextHookFactory(chain, meta, validate);
819836

820-
const hookErrIdentifier = `${resolverFilePath} resolve`;
837+
const resolution = await nextResolve(originalSpecifier, context);
838+
const { hookErrIdentifier } = meta; // Retrieve the value after all settled
821839

822840
if (typeof resolution !== 'object') { // [2]
823841
throw new ERR_INVALID_RETURN_VALUE(
@@ -827,10 +845,10 @@ class ESMLoader {
827845
);
828846
}
829847

830-
if (resolution?.shortCircuit === true) { shortCircuited = true; }
848+
if (resolution?.shortCircuit === true) { meta.shortCircuited = true; }
831849

832-
if (!chainFinished && !shortCircuited) {
833-
throw new ERR_LOADER_CHAIN_INCOMPLETE('resolve', resolverFilePath);
850+
if (!meta.chainFinished && !meta.shortCircuited) {
851+
throw new ERR_LOADER_CHAIN_INCOMPLETE(hookErrIdentifier);
834852
}
835853

836854
const {

lib/internal/modules/esm/resolve.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1081,7 +1081,7 @@ function throwIfUnsupportedURLScheme(parsed, experimentalNetworkImports) {
10811081
}
10821082
}
10831083

1084-
async function defaultResolve(specifier, context = {}, defaultResolveUnused) {
1084+
async function defaultResolve(specifier, context = {}) {
10851085
let { parentURL, conditions } = context;
10861086
if (parentURL && policy?.manifest) {
10871087
const redirects = policy.manifest.getDependencyMapper(parentURL);

0 commit comments

Comments
 (0)