GeezerVison is showing github.com — esm: fix loader hooks accepting too many arguments · nodejs/node@c2ea22f
File treeExpand file treeCollapse file tree Open diff view settings fixtures/es-module-loaders
Expand file treeCollapse file tree Open diff view settings Collapse file Copy file name to clipboardExpand all lines: lib/internal/modules/esm/loader.js +7-15Lines changed: 7 additions & 15 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -15,7 +15,6 @@ const { | 15 | 15 | ObjectDefineProperty, | 16 | 16 | ObjectSetPrototypeOf, | 17 | 17 | PromiseAll, | 18 | | - ReflectApply, | 19 | 18 | RegExpPrototypeExec, | 20 | 19 | SafeArrayIterator, | 21 | 20 | SafeWeakMap, | @@ -148,29 +147,22 @@ function nextHookFactory(chain, meta, { validateArgs, validateOutput }) { | 148 | 147 | } | 149 | 148 |
| 150 | 149 | return ObjectDefineProperty( | 151 | | - async (...args) => { | | 150 | + async (arg0 = undefined, context) => { | 152 | 151 | // Update only when hook is invoked to avoid fingering the wrong filePath | 153 | 152 | meta.hookErrIdentifier = `${hookFilePath} '${hookName}'`; | 154 | 153 |
| 155 | | - validateArgs(`${meta.hookErrIdentifier} hook's ${nextHookName}()`, args); | | 154 | + validateArgs(`${meta.hookErrIdentifier} hook's ${nextHookName}()`, arg0, context); | 156 | 155 |
| 157 | 156 | const outputErrIdentifier = `${chain[generatedHookIndex].url} '${hookName}' hook's ${nextHookName}()`; | 158 | 157 |
| 159 | 158 | // Set when next<HookName> is actually called, not just generated. | 160 | 159 | if (generatedHookIndex === 0) { meta.chainFinished = true; } | 161 | 160 |
| 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); | 170 | 163 | } | 171 | 164 |
| 172 | | - ArrayPrototypePush(args, nextNextHook); | 173 | | - const output = await ReflectApply(hook, undefined, args); | | 165 | + const output = await hook(arg0, meta.context, nextNextHook); | 174 | 166 |
| 175 | 167 | validateOutput(outputErrIdentifier, output); | 176 | 168 |
| @@ -575,7 +567,7 @@ class ESMLoader { | 575 | 567 | shortCircuited: false, | 576 | 568 | }; | 577 | 569 |
| 578 | | - const validateArgs = (hookErrIdentifier, { 0: nextUrl, 1: ctx }) => { | | 570 | + const validateArgs = (hookErrIdentifier, nextUrl, ctx) => { | 579 | 571 | if (typeof nextUrl !== 'string') { | 580 | 572 | // non-strings can be coerced to a url string | 581 | 573 | // validateString() throws a less-specific error | @@ -829,7 +821,7 @@ class ESMLoader { | 829 | 821 | shortCircuited: false, | 830 | 822 | }; | 831 | 823 |
| 832 | | - const validateArgs = (hookErrIdentifier, { 0: suppliedSpecifier, 1: ctx }) => { | | 824 | + const validateArgs = (hookErrIdentifier, suppliedSpecifier, ctx) => { | 833 | 825 | validateString( | 834 | 826 | suppliedSpecifier, | 835 | 827 | `${hookErrIdentifier} specifier`, | |
Collapse file Copy file name to clipboardExpand all lines: test/es-module/test-esm-loader-chaining.mjs +22Lines changed: 22 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -101,6 +101,28 @@ describe('ESM: loader chaining', { concurrency: true }, () => { | 101 | 101 | assert.strictEqual(code, 0); | 102 | 102 | }); | 103 | 103 |
| | 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 | + | 104 | 126 | it('should result in proper output from multiple changes in resolve hooks', async () => { | 105 | 127 | const { code, stderr, stdout } = await spawnPromisified( | 106 | 128 | execPath, | |
Collapse file Copy file name to clipboard +28Lines changed: 28 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
| | 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 Copy file name to clipboard +7Lines changed: 7 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
| | 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 | +} |
|