Skip to content

Commit

Permalink
esm: restore next<HookName>'s context as optional arg
Browse files Browse the repository at this point in the history
PR-URL: #43553
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
JakobJingleheimer authored and targos committed Jul 12, 2022
1 parent 2251242 commit 953fefe
Show file tree
Hide file tree
Showing 6 changed files with 107 additions and 38 deletions.
12 changes: 6 additions & 6 deletions doc/api/esm.md
Original file line number Diff line number Diff line change
Expand Up @@ -815,7 +815,7 @@ export async function resolve(specifier, context, nextResolve) {

// Defer to the next hook in the chain, which would be the
// Node.js default resolve if this is the last user-specified loader.
return nextResolve(specifier, context);
return nextResolve(specifier);
}
```
Expand Down Expand Up @@ -910,7 +910,7 @@ export async function load(url, context, nextLoad) {
}

// Defer to the next hook in the chain.
return nextLoad(url, context);
return nextLoad(url);
}
```
Expand Down Expand Up @@ -1026,7 +1026,7 @@ export function resolve(specifier, context, nextResolve) {
}
// Let Node.js handle all other specifiers.
return nextResolve(specifier, context);
return nextResolve(specifier);
}
export function load(url, context, nextLoad) {
Expand All @@ -1049,7 +1049,7 @@ export function load(url, context, nextLoad) {
}
// Let Node.js handle all other URLs.
return nextLoad(url, context);
return nextLoad(url);
}
```

Expand Down Expand Up @@ -1102,7 +1102,7 @@ export async function resolve(specifier, context, nextResolve) {
}
// Let Node.js handle all other specifiers.
return nextResolve(specifier, context);
return nextResolve(specifier);
}
export async function load(url, context, nextLoad) {
Expand Down Expand Up @@ -1143,7 +1143,7 @@ export async function load(url, context, nextLoad) {
}
// Let Node.js handle all other URLs.
return nextLoad(url, context);
return nextLoad(url);
}
async function getPackageType(url) {
Expand Down
79 changes: 50 additions & 29 deletions lib/internal/modules/esm/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ let emittedSpecifierResolutionWarning = false;
* validation within MUST throw.
* @returns {function next<HookName>(...hookArgs)} The next hook in the chain.
*/
function nextHookFactory(chain, meta, validate) {
function nextHookFactory(chain, meta, { validateArgs, validateOutput }) {
// First, prepare the current
const { hookName } = meta;
const {
Expand All @@ -137,7 +137,7 @@ function nextHookFactory(chain, meta, validate) {
// factory generates the next link in the chain.
meta.hookIndex--;

nextNextHook = nextHookFactory(chain, meta, validate);
nextNextHook = nextHookFactory(chain, meta, { validateArgs, validateOutput });
} else {
// eslint-disable-next-line func-name-matching
nextNextHook = function chainAdvancedTooFar() {
Expand All @@ -152,14 +152,28 @@ function nextHookFactory(chain, meta, validate) {
// Update only when hook is invoked to avoid fingering the wrong filePath
meta.hookErrIdentifier = `${hookFilePath} '${hookName}'`;

validate(`${meta.hookErrIdentifier} hook's ${nextHookName}()`, args);
validateArgs(`${meta.hookErrIdentifier} hook's ${nextHookName}()`, args);

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

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

// `context` is an optional argument that only needs to be passed when changed
switch (args.length) {
case 1: // It was omitted, so supply the cached value
ArrayPrototypePush(args, meta.context);
break;
case 2: // Overrides were supplied, so update cached value
ObjectAssign(meta.context, args[1]);
break;
}

ArrayPrototypePush(args, nextNextHook);
const output = await ReflectApply(hook, undefined, args);

validateOutput(outputErrIdentifier, output);

if (output?.shortCircuit === true) { meta.shortCircuited = true; }
return output;

Expand Down Expand Up @@ -554,13 +568,14 @@ class ESMLoader {
const chain = this.#loaders;
const meta = {
chainFinished: null,
context,
hookErrIdentifier: '',
hookIndex: chain.length - 1,
hookName: 'load',
shortCircuited: false,
};

const validate = (hookErrIdentifier, { 0: nextUrl, 1: ctx }) => {
const validateArgs = (hookErrIdentifier, { 0: nextUrl, 1: ctx }) => {
if (typeof nextUrl !== 'string') {
// non-strings can be coerced to a url string
// validateString() throws a less-specific error
Expand All @@ -584,21 +599,24 @@ class ESMLoader {
}
}

validateObject(ctx, `${hookErrIdentifier} context`);
if (ctx) validateObject(ctx, `${hookErrIdentifier} context`);
};
const validateOutput = (hookErrIdentifier, output) => {
if (typeof output !== 'object' || output === null) { // [2]
throw new ERR_INVALID_RETURN_VALUE(
'an object',
hookErrIdentifier,
output,
);
}
};

const nextLoad = nextHookFactory(chain, meta, validate);
const nextLoad = nextHookFactory(chain, meta, { validateArgs, validateOutput });

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(
'an object',
hookErrIdentifier,
loaded,
);
}
validateOutput(hookErrIdentifier, loaded);

if (loaded?.shortCircuit === true) { meta.shortCircuited = true; }

Expand Down Expand Up @@ -797,41 +815,44 @@ class ESMLoader {
);
}
const chain = this.#resolvers;
const context = {
conditions: DEFAULT_CONDITIONS,
importAssertions,
parentURL,
};
const meta = {
chainFinished: null,
context,
hookErrIdentifier: '',
hookIndex: chain.length - 1,
hookName: 'resolve',
shortCircuited: false,
};

const context = {
conditions: DEFAULT_CONDITIONS,
importAssertions,
parentURL,
};
const validate = (hookErrIdentifier, { 0: suppliedSpecifier, 1: ctx }) => {

const validateArgs = (hookErrIdentifier, { 0: suppliedSpecifier, 1: ctx }) => {
validateString(
suppliedSpecifier,
`${hookErrIdentifier} specifier`,
); // non-strings can be coerced to a url string

validateObject(ctx, `${hookErrIdentifier} context`);
if (ctx) validateObject(ctx, `${hookErrIdentifier} context`);
};
const validateOutput = (hookErrIdentifier, output) => {
if (typeof output !== 'object' || output === null) { // [2]
throw new ERR_INVALID_RETURN_VALUE(
'an object',
hookErrIdentifier,
output,
);
}
};

const nextResolve = nextHookFactory(chain, meta, validate);
const nextResolve = nextHookFactory(chain, meta, { validateArgs, validateOutput });

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(
'an object',
hookErrIdentifier,
resolution,
);
}
validateOutput(hookErrIdentifier, resolution);

if (resolution?.shortCircuit === true) { meta.shortCircuited = true; }

Expand Down
46 changes: 44 additions & 2 deletions test/es-module/test-esm-loader-chaining.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ const commonArgs = [
assert.strictEqual(status, 0);
}

{ // Verify chain does break and throws appropriately
{ // Verify resolve chain does break and throws appropriately
const { status, stderr, stdout } = spawnSync(
process.execPath,
[
Expand Down Expand Up @@ -273,7 +273,7 @@ const commonArgs = [
assert.strictEqual(status, 1);
}

{ // Verify chain does break and throws appropriately
{ // Verify load chain does break and throws appropriately
const { status, stderr, stdout } = spawnSync(
process.execPath,
[
Expand Down Expand Up @@ -314,6 +314,27 @@ const commonArgs = [
assert.match(stderr, /'resolve' hook's nextResolve\(\) specifier/);
}

{ // Verify error thrown when resolve hook is invalid
const { status, stderr } = spawnSync(
process.execPath,
[
'--loader',
fixtures.fileURL('es-module-loaders', 'loader-resolve-passthru.mjs'),
'--loader',
fixtures.fileURL('es-module-loaders', 'loader-resolve-null-return.mjs'),
...commonArgs,
],
{ encoding: 'utf8' },
);

assert.strictEqual(status, 1);
assert.match(stderr, /ERR_INVALID_RETURN_VALUE/);
assert.match(stderr, /loader-resolve-null-return\.mjs/);
assert.match(stderr, /'resolve' hook's nextResolve\(\)/);
assert.match(stderr, /an object/);
assert.match(stderr, /got null/);
}

{ // Verify error thrown when invalid `context` argument passed to `nextResolve`
const { status, stderr } = spawnSync(
process.execPath,
Expand All @@ -333,6 +354,27 @@ const commonArgs = [
assert.strictEqual(status, 1);
}

{ // Verify error thrown when load hook is invalid
const { status, stderr } = spawnSync(
process.execPath,
[
'--loader',
fixtures.fileURL('es-module-loaders', 'loader-load-passthru.mjs'),
'--loader',
fixtures.fileURL('es-module-loaders', 'loader-load-null-return.mjs'),
...commonArgs,
],
{ encoding: 'utf8' },
);

assert.strictEqual(status, 1);
assert.match(stderr, /ERR_INVALID_RETURN_VALUE/);
assert.match(stderr, /loader-load-null-return\.mjs/);
assert.match(stderr, /'load' hook's nextLoad\(\)/);
assert.match(stderr, /an object/);
assert.match(stderr, /got null/);
}

{ // Verify error thrown when invalid `url` argument passed to `nextLoad`
const { status, stderr } = spawnSync(
process.execPath,
Expand Down
3 changes: 3 additions & 0 deletions test/fixtures/es-module-loaders/loader-load-null-return.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export async function load(specifier, context, next) {
return null;
}
2 changes: 1 addition & 1 deletion test/fixtures/es-module-loaders/loader-resolve-42.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@ 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);
return next('file:///42.mjs');
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export async function resolve(specifier, context, next) {
return null;
}

0 comments on commit 953fefe

Please sign in to comment.