From be6806bdf28a34b1006a9369ed551c4e8959b6a3 Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Fri, 25 Feb 2022 10:05:55 -0600 Subject: [PATCH 1/6] esm: fix base URL for network imports --- lib/internal/modules/cjs/helpers.js | 5 +++ lib/internal/modules/cjs/loader.js | 8 ++-- .../modules/esm/initialize_import_meta.js | 14 +++---- lib/internal/modules/esm/loader.js | 41 ++++++++++++++++++- lib/internal/modules/esm/module_job.js | 3 +- lib/internal/modules/esm/translators.js | 4 +- test/es-module/test-http-imports.mjs | 15 +++++++ 7 files changed, 76 insertions(+), 14 deletions(-) diff --git a/lib/internal/modules/cjs/helpers.js b/lib/internal/modules/cjs/helpers.js index 3d27a19a250162..3f8ed4f3d57cd0 100644 --- a/lib/internal/modules/cjs/helpers.js +++ b/lib/internal/modules/cjs/helpers.js @@ -193,6 +193,11 @@ function addBuiltinLibsToObject(object, dummyModuleName) { }); } +/** + * + * @param {string | URL} referrer + * @returns {string} + */ function normalizeReferrerURL(referrer) { if (typeof referrer === 'string' && path.isAbsolute(referrer)) { return pathToFileURL(referrer).href; diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index d09f30f53e5b44..17a93dc94f0082 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -1015,9 +1015,10 @@ function wrapSafe(filename, content, cjsModuleInstance) { filename, lineOffset: 0, displayErrors: true, - importModuleDynamically: async (specifier, _, importAssertions) => { + importModuleDynamically: (specifier, _, importAssertions) => { const loader = asyncESM.esmLoader; - return loader.import(specifier, normalizeReferrerURL(filename), + return loader.import(specifier, + loader.baseURL(normalizeReferrerURL(filename)), importAssertions); }, }); @@ -1033,7 +1034,8 @@ function wrapSafe(filename, content, cjsModuleInstance) { filename, importModuleDynamically(specifier, _, importAssertions) { const loader = asyncESM.esmLoader; - return loader.import(specifier, normalizeReferrerURL(filename), + return loader.import(specifier, + loader.baseURL(normalizeReferrerURL(filename)), importAssertions); }, }); diff --git a/lib/internal/modules/esm/initialize_import_meta.js b/lib/internal/modules/esm/initialize_import_meta.js index cb9fa23f966f76..f4bf80cde70326 100644 --- a/lib/internal/modules/esm/initialize_import_meta.js +++ b/lib/internal/modules/esm/initialize_import_meta.js @@ -24,6 +24,11 @@ function createImportMetaResolve(defaultParentUrl) { }; } +/** + * + * @param {object} meta + * @param {{url: string}} context + */ function initializeImportMeta(meta, context) { let url = context.url; @@ -32,14 +37,7 @@ function initializeImportMeta(meta, context) { meta.resolve = createImportMetaResolve(url); } - if ( - StringPrototypeStartsWith(url, 'http:') || - StringPrototypeStartsWith(url, 'https:') - ) { - // The request & response have already settled, so they are in fetchModule's - // cache, in which case, fetchModule returns immediately and synchronously - url = fetchModule(new URL(url), context).resolvedHREF; - } + url = asyncESM.esmLoader.baseURL(url); meta.url = url; } diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 61e609d9ad6c62..65564aa2318246 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -17,6 +17,7 @@ const { RegExpPrototypeExec, SafeArrayIterator, SafeWeakMap, + StringPrototypeStartsWith, globalThis, } = primordials; const { MessageChannel } = require('internal/worker/io'); @@ -47,6 +48,9 @@ const { defaultLoad } = require('internal/modules/esm/load'); const { translators } = require( 'internal/modules/esm/translators'); const { getOptionValue } = require('internal/options'); +const { + fetchModule, +} = require('internal/modules/esm/fetch_module'); /** * An ESMLoader instance is used as the main entry point for loading ES modules. @@ -209,7 +213,9 @@ class ESMLoader { const module = new ModuleWrap(url, undefined, source, 0, 0); callbackMap.set(module, { importModuleDynamically: (specifier, { url }, importAssertions) => { - return this.import(specifier, url, importAssertions); + return this.import(specifier, + this.baseURL(url), + importAssertions); } }); @@ -225,6 +231,39 @@ class ESMLoader { }; } + /** + * Returns the url to use for resolution for a given cache key url + * These are not guaranteed to be the same. + * + * In WHATWG HTTP spec for ESM the cache key is the non-I/O bound + * synchronous resolution using only string operations + * ~= resolveImportMap(new URL(specifier, importerHREF)) + * + * The url used for subsequent resolution is the response URL after + * all redirects have been resolved. + * + * https://example.com/foo redirecting to https://example.com/bar + * would have a cache key of https://example.com/foo and baseURL + * of https://example.com/bar + * + * MUST BE SYNCHRONOUS for import.meta initialization + * MUST BE CALLED AFTER body for url is received due to I/O + * @param {string} url + * @returns {string} + */ + baseURL(url) { + if ( + StringPrototypeStartsWith(url, 'http:') || + StringPrototypeStartsWith(url, 'https:') + ) { + // The request & response have already settled, so they are in + // fetchModule's cache, in which case, fetchModule returns + // immediately and synchronously + url = fetchModule(new URL(url), {parentURL: url}).resolvedHREF; + } + return url; + } + /** * Get a (possibly still pending) module job from the cache, * or create one and return its Promise. diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index fd1c6166330e76..524272664e2a0d 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -76,7 +76,8 @@ class ModuleJob { // these `link` callbacks depending on each other. const dependencyJobs = []; const promises = this.module.link(async (specifier, assertions) => { - const jobPromise = this.loader.getModuleJob(specifier, url, assertions); + const baseURL = this.loader.baseURL(url); + const jobPromise = this.loader.getModuleJob(specifier, baseURL, assertions); ArrayPrototypePush(dependencyJobs, jobPromise); const job = await jobPromise; return job.modulePromise; diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index da95703faad9fd..93fd1dbb57874b 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -103,7 +103,9 @@ function errPath(url) { } async function importModuleDynamically(specifier, { url }, assertions) { - return asyncESM.esmLoader.import(specifier, url, assertions); + return asyncESM.esmLoader.import(specifier, + asyncESM.esmLoader.baseURL(url), + assertions); } // Strategy for loading a standard JavaScript module. diff --git a/test/es-module/test-http-imports.mjs b/test/es-module/test-http-imports.mjs index 766645f9125384..b35a34df978fd3 100644 --- a/test/es-module/test-http-imports.mjs +++ b/test/es-module/test-http-imports.mjs @@ -116,6 +116,21 @@ for (const { protocol, createServer } of [ assert.strict.notEqual(redirectedNS.default, ns.default); assert.strict.equal(redirectedNS.url, url.href); + // Redirects have same import.meta.url but different cache + // entry on Web + const relativeAfterRedirect = new URL(url.href + 'foo/index.js'); + const redirected = new URL(url.href + 'bar/index.js'); + redirected.searchParams.set('body', `export let relativeDepURL = (await import("./baz.js")).url`); + relativeAfterRedirect.searchParams.set('redirect', JSON.stringify({ + status: 302, + location: redirected.href + })); + const relativeAfterRedirectedNS = await import(relativeAfterRedirect.href); + assert.strict.equal( + relativeAfterRedirectedNS.relativeDepURL, + url.href + 'bar/baz.js' + ); + const crossProtocolRedirect = new URL(url.href); crossProtocolRedirect.searchParams.set('redirect', JSON.stringify({ status: 302, From 03f54a8a0e717bee74aeaa480214d0ab96f7731e Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Fri, 25 Feb 2022 10:52:16 -0600 Subject: [PATCH 2/6] fixup: lint --- lib/internal/modules/cjs/helpers.js | 2 +- lib/internal/modules/cjs/loader.js | 2 +- lib/internal/modules/esm/initialize_import_meta.js | 8 ++------ lib/internal/modules/esm/loader.js | 10 +++++----- test/es-module/test-http-imports.mjs | 2 +- 5 files changed, 10 insertions(+), 14 deletions(-) diff --git a/lib/internal/modules/cjs/helpers.js b/lib/internal/modules/cjs/helpers.js index 3f8ed4f3d57cd0..3ae63b46195f75 100644 --- a/lib/internal/modules/cjs/helpers.js +++ b/lib/internal/modules/cjs/helpers.js @@ -194,7 +194,7 @@ function addBuiltinLibsToObject(object, dummyModuleName) { } /** - * + * * @param {string | URL} referrer * @returns {string} */ diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 17a93dc94f0082..841f0ad3b580da 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -1017,7 +1017,7 @@ function wrapSafe(filename, content, cjsModuleInstance) { displayErrors: true, importModuleDynamically: (specifier, _, importAssertions) => { const loader = asyncESM.esmLoader; - return loader.import(specifier, + return loader.import(specifier, loader.baseURL(normalizeReferrerURL(filename)), importAssertions); }, diff --git a/lib/internal/modules/esm/initialize_import_meta.js b/lib/internal/modules/esm/initialize_import_meta.js index f4bf80cde70326..f9a2c0570244bb 100644 --- a/lib/internal/modules/esm/initialize_import_meta.js +++ b/lib/internal/modules/esm/initialize_import_meta.js @@ -3,12 +3,9 @@ const { getOptionValue } = require('internal/options'); const experimentalImportMetaResolve = getOptionValue('--experimental-import-meta-resolve'); -const { fetchModule } = require('internal/modules/esm/fetch_module'); -const { URL } = require('internal/url'); const { PromisePrototypeThen, PromiseReject, - StringPrototypeStartsWith, } = primordials; const asyncESM = require('internal/process/esm_loader'); @@ -25,9 +22,8 @@ function createImportMetaResolve(defaultParentUrl) { } /** - * - * @param {object} meta - * @param {{url: string}} context + * @param {object} meta + * @param {{url: string}} context */ function initializeImportMeta(meta, context) { let url = context.url; diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 65564aa2318246..ac8e36c7ada9d5 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -234,18 +234,18 @@ class ESMLoader { /** * Returns the url to use for resolution for a given cache key url * These are not guaranteed to be the same. - * + * * In WHATWG HTTP spec for ESM the cache key is the non-I/O bound * synchronous resolution using only string operations * ~= resolveImportMap(new URL(specifier, importerHREF)) - * + * * The url used for subsequent resolution is the response URL after * all redirects have been resolved. - * + * * https://example.com/foo redirecting to https://example.com/bar * would have a cache key of https://example.com/foo and baseURL * of https://example.com/bar - * + * * MUST BE SYNCHRONOUS for import.meta initialization * MUST BE CALLED AFTER body for url is received due to I/O * @param {string} url @@ -259,7 +259,7 @@ class ESMLoader { // The request & response have already settled, so they are in // fetchModule's cache, in which case, fetchModule returns // immediately and synchronously - url = fetchModule(new URL(url), {parentURL: url}).resolvedHREF; + url = fetchModule(new URL(url), { parentURL: url }).resolvedHREF; } return url; } diff --git a/test/es-module/test-http-imports.mjs b/test/es-module/test-http-imports.mjs index b35a34df978fd3..01fc08dc388ebb 100644 --- a/test/es-module/test-http-imports.mjs +++ b/test/es-module/test-http-imports.mjs @@ -120,7 +120,7 @@ for (const { protocol, createServer } of [ // entry on Web const relativeAfterRedirect = new URL(url.href + 'foo/index.js'); const redirected = new URL(url.href + 'bar/index.js'); - redirected.searchParams.set('body', `export let relativeDepURL = (await import("./baz.js")).url`); + redirected.searchParams.set('body', 'export let relativeDepURL = (await import("./baz.js")).url'); relativeAfterRedirect.searchParams.set('redirect', JSON.stringify({ status: 302, location: redirected.href From b9c145aaa2149f3141f7014927b9f687771a6e7a Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Fri, 25 Feb 2022 15:32:55 -0600 Subject: [PATCH 3/6] fixup: Update lib/internal/modules/esm/loader.js Co-authored-by: Mestery --- lib/internal/modules/esm/loader.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index ac8e36c7ada9d5..a47beb30bfe83f 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -247,7 +247,7 @@ class ESMLoader { * of https://example.com/bar * * MUST BE SYNCHRONOUS for import.meta initialization - * MUST BE CALLED AFTER body for url is received due to I/O + * MUST BE CALLED AFTER receiving the url body due to I/O * @param {string} url * @returns {string} */ From c336ad07fd21eb5ec56cdf9b049e853cd8d99c6b Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Fri, 25 Feb 2022 16:47:22 -0600 Subject: [PATCH 4/6] fixup: Apply suggestions from code review Co-authored-by: Mohammed Keyvanzadeh --- lib/internal/modules/esm/loader.js | 2 +- test/es-module/test-http-imports.mjs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index a47beb30bfe83f..504b55f7101531 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -232,7 +232,7 @@ class ESMLoader { } /** - * Returns the url to use for resolution for a given cache key url + * Returns the url to use for the resolution of a given cache key url * These are not guaranteed to be the same. * * In WHATWG HTTP spec for ESM the cache key is the non-I/O bound diff --git a/test/es-module/test-http-imports.mjs b/test/es-module/test-http-imports.mjs index 01fc08dc388ebb..b07d1103236190 100644 --- a/test/es-module/test-http-imports.mjs +++ b/test/es-module/test-http-imports.mjs @@ -116,7 +116,7 @@ for (const { protocol, createServer } of [ assert.strict.notEqual(redirectedNS.default, ns.default); assert.strict.equal(redirectedNS.url, url.href); - // Redirects have same import.meta.url but different cache + // Redirects have the same import.meta.url but different cache // entry on Web const relativeAfterRedirect = new URL(url.href + 'foo/index.js'); const redirected = new URL(url.href + 'bar/index.js'); From e0da1a3725ce3f3c6a43fd28f3b9fe3f4bbe959a Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Mon, 28 Feb 2022 10:57:14 -0600 Subject: [PATCH 5/6] fixup: assert baseURL is a string for network modules --- lib/internal/modules/esm/loader.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 504b55f7101531..b5d28b5d57780e 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -23,6 +23,7 @@ const { const { MessageChannel } = require('internal/worker/io'); const { + ERR_INTERNAL_ASSERTION, ERR_INVALID_ARG_TYPE, ERR_INVALID_ARG_VALUE, ERR_INVALID_RETURN_PROPERTY_VALUE, @@ -260,6 +261,10 @@ class ESMLoader { // fetchModule's cache, in which case, fetchModule returns // immediately and synchronously url = fetchModule(new URL(url), { parentURL: url }).resolvedHREF; + // This should only occur if the module hasn't been fetched yet + if (typeof url !== 'string') { + throw new ERR_INTERNAL_ASSERTION(`Base url for module ${url} not loaded.`); + } } return url; } From d6e832692c33688ab379f056a4a3a596f8701894 Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Wed, 2 Mar 2022 15:06:32 -0600 Subject: [PATCH 6/6] fixup: apply code review changes --- lib/internal/modules/cjs/loader.js | 6 +++--- lib/internal/modules/esm/initialize_import_meta.js | 2 +- lib/internal/modules/esm/loader.js | 4 ++-- lib/internal/modules/esm/module_job.js | 2 +- lib/internal/modules/esm/translators.js | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 841f0ad3b580da..9e4229af9faaf8 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -1015,10 +1015,10 @@ function wrapSafe(filename, content, cjsModuleInstance) { filename, lineOffset: 0, displayErrors: true, - importModuleDynamically: (specifier, _, importAssertions) => { + importModuleDynamically: async (specifier, _, importAssertions) => { const loader = asyncESM.esmLoader; return loader.import(specifier, - loader.baseURL(normalizeReferrerURL(filename)), + loader.getBaseURL(normalizeReferrerURL(filename)), importAssertions); }, }); @@ -1035,7 +1035,7 @@ function wrapSafe(filename, content, cjsModuleInstance) { importModuleDynamically(specifier, _, importAssertions) { const loader = asyncESM.esmLoader; return loader.import(specifier, - loader.baseURL(normalizeReferrerURL(filename)), + loader.getBaseURL(normalizeReferrerURL(filename)), importAssertions); }, }); diff --git a/lib/internal/modules/esm/initialize_import_meta.js b/lib/internal/modules/esm/initialize_import_meta.js index f9a2c0570244bb..f1daabbb6425aa 100644 --- a/lib/internal/modules/esm/initialize_import_meta.js +++ b/lib/internal/modules/esm/initialize_import_meta.js @@ -33,7 +33,7 @@ function initializeImportMeta(meta, context) { meta.resolve = createImportMetaResolve(url); } - url = asyncESM.esmLoader.baseURL(url); + url = asyncESM.esmLoader.getBaseURL(url); meta.url = url; } diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index b5d28b5d57780e..16b832527f7c64 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -215,7 +215,7 @@ class ESMLoader { callbackMap.set(module, { importModuleDynamically: (specifier, { url }, importAssertions) => { return this.import(specifier, - this.baseURL(url), + this.getBaseURL(url), importAssertions); } }); @@ -252,7 +252,7 @@ class ESMLoader { * @param {string} url * @returns {string} */ - baseURL(url) { + getBaseURL(url) { if ( StringPrototypeStartsWith(url, 'http:') || StringPrototypeStartsWith(url, 'https:') diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index 524272664e2a0d..e012eebc4ac971 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -76,7 +76,7 @@ class ModuleJob { // these `link` callbacks depending on each other. const dependencyJobs = []; const promises = this.module.link(async (specifier, assertions) => { - const baseURL = this.loader.baseURL(url); + const baseURL = this.loader.getBaseURL(url); const jobPromise = this.loader.getModuleJob(specifier, baseURL, assertions); ArrayPrototypePush(dependencyJobs, jobPromise); const job = await jobPromise; diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index 93fd1dbb57874b..d7f4c7edec63d3 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -104,7 +104,7 @@ function errPath(url) { async function importModuleDynamically(specifier, { url }, assertions) { return asyncESM.esmLoader.import(specifier, - asyncESM.esmLoader.baseURL(url), + asyncESM.esmLoader.getBaseURL(url), assertions); }