From 5a9bf227a256ec42b8075d17a347f54465965b22 Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Mon, 14 Feb 2022 21:59:05 +0100 Subject: [PATCH 1/4] esm: remove erroneous `context.parentURL` property passed to `load` hook --- lib/internal/modules/esm/loader.js | 1 - test/es-module/test-esm-loader-hooks.mjs | 70 ++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 test/es-module/test-esm-loader-hooks.mjs diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index d5e0b61af6a309..61e609d9ad6c62 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -282,7 +282,6 @@ class ESMLoader { } = await this.load(url, { format, importAssertions, - parentURL, }); const translator = translators.get(finalFormat); diff --git a/test/es-module/test-esm-loader-hooks.mjs b/test/es-module/test-esm-loader-hooks.mjs new file mode 100644 index 00000000000000..ac57d3c62000e9 --- /dev/null +++ b/test/es-module/test-esm-loader-hooks.mjs @@ -0,0 +1,70 @@ +// Flags: --expose-internals +import '../common/index.mjs'; +import mod from 'internal/modules/esm/loader'; +import assert from 'assert'; + +const { ESMLoader } = mod; + +{ + const esmLoader = new ESMLoader(); + + const originalSpecifier = 'foo/bar'; + const importAssertions = { type: 'json' }; + const parentURL = 'file:///entrypoint.js'; + const resolvedURL = 'file:///foo/bar.js'; + const suggestedFormat = 'test'; + + const customLoader1 = { + resolve(specifier, context, defaultResolve) { + assert.strictEqual(specifier, originalSpecifier); + assert.deepStrictEqual(Object.keys(context), [ + 'conditions', + 'importAssertions', + 'parentURL', + ]); + assert.ok(Array.isArray(context.conditions)); + assert.strictEqual(context.importAssertions, importAssertions); + assert.strictEqual(context.parentURL, parentURL); + assert.strictEqual(typeof defaultResolve, 'function'); + + // This doesn't matter. just to avoid errors + return { + format: suggestedFormat, + url: resolvedURL, + }; + }, + load(resolvedURL, context, defaultLoad) { + assert.strictEqual(resolvedURL, resolvedURL); + assert.ok(new URL(resolvedURL)); + assert.deepStrictEqual(Object.keys(context), [ + 'format', + 'importAssertions', + ]); + assert.strictEqual(context.format, suggestedFormat); + assert.strictEqual(context.importAssertions, importAssertions); + assert.strictEqual(typeof defaultLoad, 'function'); + + // This doesn't matter. just to avoid errors + return { + format: 'module', + source: '', + }; + }, + }; + + esmLoader.addCustomLoaders(customLoader1); + + esmLoader.resolve( + originalSpecifier, + parentURL, + importAssertions, + ); + esmLoader.load( + resolvedURL, + { + format: suggestedFormat, + importAssertions, + }, + function mockDefaultLoad() {}, + ); +} From f733c70554e78a5a02479bbd0c4a341526bcc10b Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Tue, 15 Feb 2022 21:15:03 +0100 Subject: [PATCH 2/4] fixup: ensure custom hooks are called & clarify import name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * rename `mod` → `esmLoaderModule` --- test/es-module/test-esm-loader-hooks.mjs | 77 ++++++++++++------------ 1 file changed, 40 insertions(+), 37 deletions(-) diff --git a/test/es-module/test-esm-loader-hooks.mjs b/test/es-module/test-esm-loader-hooks.mjs index ac57d3c62000e9..0aa9941385bd0e 100644 --- a/test/es-module/test-esm-loader-hooks.mjs +++ b/test/es-module/test-esm-loader-hooks.mjs @@ -1,9 +1,9 @@ // Flags: --expose-internals -import '../common/index.mjs'; -import mod from 'internal/modules/esm/loader'; +import { mustCall } from '../common/index.mjs'; +import esmLoaderModule from 'internal/modules/esm/loader'; import assert from 'assert'; -const { ESMLoader } = mod; +const { ESMLoader } = esmLoaderModule; { const esmLoader = new ESMLoader(); @@ -14,42 +14,45 @@ const { ESMLoader } = mod; const resolvedURL = 'file:///foo/bar.js'; const suggestedFormat = 'test'; - const customLoader1 = { - resolve(specifier, context, defaultResolve) { - assert.strictEqual(specifier, originalSpecifier); - assert.deepStrictEqual(Object.keys(context), [ - 'conditions', - 'importAssertions', - 'parentURL', - ]); - assert.ok(Array.isArray(context.conditions)); - assert.strictEqual(context.importAssertions, importAssertions); - assert.strictEqual(context.parentURL, parentURL); - assert.strictEqual(typeof defaultResolve, 'function'); + function resolve(specifier, context, defaultResolve) { + assert.strictEqual(specifier, originalSpecifier); + assert.deepStrictEqual(Object.keys(context), [ + 'conditions', + 'importAssertions', + 'parentURL', + ]); + assert.ok(Array.isArray(context.conditions)); + assert.strictEqual(context.importAssertions, importAssertions); + assert.strictEqual(context.parentURL, parentURL); + assert.strictEqual(typeof defaultResolve, 'function'); - // This doesn't matter. just to avoid errors - return { - format: suggestedFormat, - url: resolvedURL, - }; - }, - load(resolvedURL, context, defaultLoad) { - assert.strictEqual(resolvedURL, resolvedURL); - assert.ok(new URL(resolvedURL)); - assert.deepStrictEqual(Object.keys(context), [ - 'format', - 'importAssertions', - ]); - assert.strictEqual(context.format, suggestedFormat); - assert.strictEqual(context.importAssertions, importAssertions); - assert.strictEqual(typeof defaultLoad, 'function'); + return { + format: suggestedFormat, + url: resolvedURL, + }; + } - // This doesn't matter. just to avoid errors - return { - format: 'module', - source: '', - }; - }, + function load(resolvedURL, context, defaultLoad) { + assert.strictEqual(resolvedURL, resolvedURL); + assert.ok(new URL(resolvedURL)); + assert.deepStrictEqual(Object.keys(context), [ + 'format', + 'importAssertions', + ]); + assert.strictEqual(context.format, suggestedFormat); + assert.strictEqual(context.importAssertions, importAssertions); + assert.strictEqual(typeof defaultLoad, 'function'); + + // This doesn't matter (just to avoid errors) + return { + format: 'module', + source: '', + }; + } + + const customLoader1 = { + resolve: mustCall(resolve), + load: mustCall(load), }; esmLoader.addCustomLoaders(customLoader1); From b0087997f7315b1452fe810fd7e88104fb495f93 Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Wed, 16 Feb 2022 19:13:11 +0100 Subject: [PATCH 3/4] fixup: add comments explaining purpose of new test set --- test/es-module/test-esm-loader-hooks.mjs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/test/es-module/test-esm-loader-hooks.mjs b/test/es-module/test-esm-loader-hooks.mjs index 0aa9941385bd0e..887d5f4d96cf5d 100644 --- a/test/es-module/test-esm-loader-hooks.mjs +++ b/test/es-module/test-esm-loader-hooks.mjs @@ -5,6 +5,9 @@ import assert from 'assert'; const { ESMLoader } = esmLoaderModule; +/** + * Verify custom hooks are called with appropriate arguments. + */ { const esmLoader = new ESMLoader(); @@ -16,6 +19,7 @@ const { ESMLoader } = esmLoaderModule; function resolve(specifier, context, defaultResolve) { assert.strictEqual(specifier, originalSpecifier); + // Ensure `context` has all and only the properties it's supposed to assert.deepStrictEqual(Object.keys(context), [ 'conditions', 'importAssertions', @@ -35,6 +39,7 @@ const { ESMLoader } = esmLoaderModule; function load(resolvedURL, context, defaultLoad) { assert.strictEqual(resolvedURL, resolvedURL); assert.ok(new URL(resolvedURL)); + // Ensure `context` has all and only the properties it's supposed to assert.deepStrictEqual(Object.keys(context), [ 'format', 'importAssertions', @@ -51,12 +56,14 @@ const { ESMLoader } = esmLoaderModule; } const customLoader1 = { + // Ensure ESMLoader actually calls the custom hooks resolve: mustCall(resolve), load: mustCall(load), }; esmLoader.addCustomLoaders(customLoader1); + // Manually trigger hook chains (since ESMLoader is not actually running) esmLoader.resolve( originalSpecifier, parentURL, @@ -68,6 +75,5 @@ const { ESMLoader } = esmLoaderModule; format: suggestedFormat, importAssertions, }, - function mockDefaultLoad() {}, ); } From f39a2ca799655e8a68906ca2884785001945e22a Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Thu, 17 Feb 2022 14:18:12 +0100 Subject: [PATCH 4/4] fixup: avoid undesirably bypassing implementation --- test/es-module/test-esm-loader-hooks.mjs | 25 +++++++++++------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/test/es-module/test-esm-loader-hooks.mjs b/test/es-module/test-esm-loader-hooks.mjs index 887d5f4d96cf5d..57a203342ac49c 100644 --- a/test/es-module/test-esm-loader-hooks.mjs +++ b/test/es-module/test-esm-loader-hooks.mjs @@ -12,7 +12,10 @@ const { ESMLoader } = esmLoaderModule; const esmLoader = new ESMLoader(); const originalSpecifier = 'foo/bar'; - const importAssertions = { type: 'json' }; + const importAssertions = Object.assign( + Object.create(null), + { type: 'json' }, + ); const parentURL = 'file:///entrypoint.js'; const resolvedURL = 'file:///foo/bar.js'; const suggestedFormat = 'test'; @@ -26,7 +29,7 @@ const { ESMLoader } = esmLoaderModule; 'parentURL', ]); assert.ok(Array.isArray(context.conditions)); - assert.strictEqual(context.importAssertions, importAssertions); + assert.deepStrictEqual(context.importAssertions, importAssertions); assert.strictEqual(context.parentURL, parentURL); assert.strictEqual(typeof defaultResolve, 'function'); @@ -45,7 +48,7 @@ const { ESMLoader } = esmLoaderModule; 'importAssertions', ]); assert.strictEqual(context.format, suggestedFormat); - assert.strictEqual(context.importAssertions, importAssertions); + assert.deepStrictEqual(context.importAssertions, importAssertions); assert.strictEqual(typeof defaultLoad, 'function'); // This doesn't matter (just to avoid errors) @@ -55,25 +58,19 @@ const { ESMLoader } = esmLoaderModule; }; } - const customLoader1 = { + const customLoader = { // Ensure ESMLoader actually calls the custom hooks resolve: mustCall(resolve), load: mustCall(load), }; - esmLoader.addCustomLoaders(customLoader1); + esmLoader.addCustomLoaders(customLoader); - // Manually trigger hook chains (since ESMLoader is not actually running) - esmLoader.resolve( + // Manually trigger hooks (since ESMLoader is not actually running) + const job = await esmLoader.getModuleJob( originalSpecifier, parentURL, importAssertions, ); - esmLoader.load( - resolvedURL, - { - format: suggestedFormat, - importAssertions, - }, - ); + await job.modulePromise; }