Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit c2ea22f

Browse filesBrowse files
JakobJingleheimerdanielleadams
authored andcommitted
esm: fix loader hooks accepting too many arguments
PR-URL: #44109 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
1 parent 2178f17 commit c2ea22f
Copy full SHA for c2ea22f

File tree

Expand file treeCollapse file tree

4 files changed

+64
-15
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

4 files changed

+64
-15
lines changed
Open diff view settings
Collapse file

‎lib/internal/modules/esm/loader.js‎

Copy file name to clipboardExpand all lines: lib/internal/modules/esm/loader.js
+7-15Lines changed: 7 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ const {
1515
ObjectDefineProperty,
1616
ObjectSetPrototypeOf,
1717
PromiseAll,
18-
ReflectApply,
1918
RegExpPrototypeExec,
2019
SafeArrayIterator,
2120
SafeWeakMap,
@@ -148,29 +147,22 @@ function nextHookFactory(chain, meta, { validateArgs, validateOutput }) {
148147
}
149148

150149
return ObjectDefineProperty(
151-
async (...args) => {
150+
async (arg0 = undefined, context) => {
152151
// Update only when hook is invoked to avoid fingering the wrong filePath
153152
meta.hookErrIdentifier = `${hookFilePath} '${hookName}'`;
154153

155-
validateArgs(`${meta.hookErrIdentifier} hook's ${nextHookName}()`, args);
154+
validateArgs(`${meta.hookErrIdentifier} hook's ${nextHookName}()`, arg0, context);
156155

157156
const outputErrIdentifier = `${chain[generatedHookIndex].url} '${hookName}' hook's ${nextHookName}()`;
158157

159158
// Set when next<HookName> is actually called, not just generated.
160159
if (generatedHookIndex === 0) { meta.chainFinished = true; }
161160

162-
// `context` is an optional argument that only needs to be passed when changed
163-
switch (args.length) {
164-
case 1: // It was omitted, so supply the cached value
165-
ArrayPrototypePush(args, meta.context);
166-
break;
167-
case 2: // Overrides were supplied, so update cached value
168-
ObjectAssign(meta.context, args[1]);
169-
break;
161+
if (context) { // `context` has already been validated, so no fancy check needed.
162+
ObjectAssign(meta.context, context);
170163
}
171164

172-
ArrayPrototypePush(args, nextNextHook);
173-
const output = await ReflectApply(hook, undefined, args);
165+
const output = await hook(arg0, meta.context, nextNextHook);
174166

175167
validateOutput(outputErrIdentifier, output);
176168

@@ -575,7 +567,7 @@ class ESMLoader {
575567
shortCircuited: false,
576568
};
577569

578-
const validateArgs = (hookErrIdentifier, { 0: nextUrl, 1: ctx }) => {
570+
const validateArgs = (hookErrIdentifier, nextUrl, ctx) => {
579571
if (typeof nextUrl !== 'string') {
580572
// non-strings can be coerced to a url string
581573
// validateString() throws a less-specific error
@@ -829,7 +821,7 @@ class ESMLoader {
829821
shortCircuited: false,
830822
};
831823

832-
const validateArgs = (hookErrIdentifier, { 0: suppliedSpecifier, 1: ctx }) => {
824+
const validateArgs = (hookErrIdentifier, suppliedSpecifier, ctx) => {
833825
validateString(
834826
suppliedSpecifier,
835827
`${hookErrIdentifier} specifier`,
Collapse file

‎test/es-module/test-esm-loader-chaining.mjs‎

Copy file name to clipboardExpand all lines: test/es-module/test-esm-loader-chaining.mjs
+22Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,28 @@ describe('ESM: loader chaining', { concurrency: true }, () => {
101101
assert.strictEqual(code, 0);
102102
});
103103

104+
it('should accept only the correct arguments', async () => {
105+
const { stdout } = await spawnPromisified(
106+
execPath,
107+
[
108+
'--loader',
109+
fixtures.fileURL('es-module-loaders', 'loader-log-args.mjs'),
110+
'--loader',
111+
fixtures.fileURL('es-module-loaders', 'loader-with-too-many-args.mjs'),
112+
...commonArgs,
113+
],
114+
{ encoding: 'utf8' },
115+
);
116+
117+
assert.match(stdout, /^resolve arg count: 3$/m);
118+
assert.match(stdout, /specifier: 'node:fs'/);
119+
assert.match(stdout, /next: \[AsyncFunction: nextResolve\]/);
120+
121+
assert.match(stdout, /^load arg count: 3$/m);
122+
assert.match(stdout, /url: 'node:fs'/);
123+
assert.match(stdout, /next: \[AsyncFunction: nextLoad\]/);
124+
});
125+
104126
it('should result in proper output from multiple changes in resolve hooks', async () => {
105127
const { code, stderr, stdout } = await spawnPromisified(
106128
execPath,
Collapse file
+28Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
export async function resolve(...args) {
2+
console.log(`resolve arg count: ${args.length}`);
3+
console.log({
4+
specifier: args[0],
5+
context: args[1],
6+
next: args[2],
7+
});
8+
9+
return {
10+
shortCircuit: true,
11+
url: args[0],
12+
};
13+
}
14+
15+
export async function load(...args) {
16+
console.log(`load arg count: ${args.length}`);
17+
console.log({
18+
url: args[0],
19+
context: args[1],
20+
next: args[2],
21+
});
22+
23+
return {
24+
format: 'module',
25+
source: '',
26+
shortCircuit: true,
27+
};
28+
}
Collapse file
+7Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
export async function resolve(specifier, context, next) {
2+
return next(specifier, context, 'resolve-extra-arg');
3+
}
4+
5+
export async function load(url, context, next) {
6+
return next(url, context, 'load-extra-arg');
7+
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.