From 22d77773fd5e8b6feba74b6c293c3d4887f14ecb Mon Sep 17 00:00:00 2001 From: Wei Zhu Date: Wed, 16 Oct 2024 05:32:07 +1030 Subject: [PATCH] esm: fix inconsistency with `importAssertion` in `resolve` hook As the documentation states, the `context.importAssertion` should be still supported and emit a warning. This is true for the `load` hook, but not correct for context of the `resolve` hook. This commit fixes the inconsistency. PR-URL: https://github.com/nodejs/node/pull/55365 Reviewed-By: Antoine du Hamel Reviewed-By: Matteo Collina --- lib/internal/modules/esm/hooks.js | 2 +- test/es-module/test-esm-import-assertion-warning.mjs | 9 +++++++-- test/fixtures/es-module-loaders/hooks-input.mjs | 4 +++- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/lib/internal/modules/esm/hooks.js b/lib/internal/modules/esm/hooks.js index 5984c707f16c4f..4571922ed5a0e9 100644 --- a/lib/internal/modules/esm/hooks.js +++ b/lib/internal/modules/esm/hooks.js @@ -237,7 +237,7 @@ class Hooks { const nextResolve = nextHookFactory(chain[chain.length - 1], meta, { validateArgs, validateOutput }); - const resolution = await nextResolve(originalSpecifier, context); + const resolution = await nextResolve(originalSpecifier, defineImportAssertionAlias(context)); const { hookErrIdentifier } = meta; // Retrieve the value after all settled validateOutput(hookErrIdentifier, resolution); diff --git a/test/es-module/test-esm-import-assertion-warning.mjs b/test/es-module/test-esm-import-assertion-warning.mjs index a11b5164cebffc..2f9b7348a2d97a 100644 --- a/test/es-module/test-esm-import-assertion-warning.mjs +++ b/test/es-module/test-esm-import-assertion-warning.mjs @@ -7,6 +7,11 @@ await Promise.all([ `data:text/javascript,export ${encodeURIComponent(function resolve() { return { shortCircuit: true, url: 'data:application/json,1', importAssertions: { type: 'json' } }; })}`, + // Using importAssertions on the context object of the resolve hook should warn but still work. + `data:text/javascript,export ${encodeURIComponent(function resolve(s, c, n) { + const type = c.importAssertions.type; + return { shortCircuit: true, url: 'data:application/json,1', importAttributes: { type: type ?? 'json' } }; + })}`, // Setting importAssertions on the context object of the load hook should warn but still work. `data:text/javascript,export ${encodeURIComponent(function load(u, c, n) { c.importAssertions = { type: 'json' }; @@ -22,9 +27,9 @@ await Promise.all([ '--eval', ` import assert from 'node:assert'; import { register } from 'node:module'; - + register(${JSON.stringify(loaderURL)}); - + assert.deepStrictEqual( { ...await import('data:') }, { default: 1 } diff --git a/test/fixtures/es-module-loaders/hooks-input.mjs b/test/fixtures/es-module-loaders/hooks-input.mjs index 1d3759f458224e..854b8e619281e4 100644 --- a/test/fixtures/es-module-loaders/hooks-input.mjs +++ b/test/fixtures/es-module-loaders/hooks-input.mjs @@ -37,6 +37,7 @@ export async function resolve(specifier, context, next) { 'conditions', 'importAttributes', 'parentURL', + 'importAssertions', ]); assert.ok(Array.isArray(context.conditions)); assert.strictEqual(typeof next, 'function'); @@ -71,9 +72,10 @@ export async function load(url, context, next) { assert.ok(new URL(url)); // Ensure `context` has all and only the properties it's supposed to - assert.deepStrictEqual(Object.keys(context), [ + assert.deepStrictEqual(Reflect.ownKeys(context), [ 'format', 'importAttributes', + 'importAssertions', ]); assert.strictEqual(context.format, 'test'); assert.strictEqual(typeof next, 'function');