FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

esm: fix loader hooks accepting too many arguments · nodejs/node@c2ea22f · GitHub

/ node Public

Commit c2ea22f

Browse files
JakobJingleheimer
authored and
danielleadams
committed
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

4 files changed

Lines changed: 64 additions & 15 deletions

File tree

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

Lines 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`,

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

Lines 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,

‎test/fixtures/es-module-loaders/loader-log-args.mjs‎

Lines 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+
}

‎test/fixtures/es-module-loaders/loader-with-too-many-args.mjs‎

Lines 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)
Back | FazBrowse Home | New Git URL