From 50d64edd49805a0a26414511e165aad179cd488c Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Fri, 20 May 2022 17:52:16 -0700 Subject: [PATCH] esm: refactor responseURL handling PR-URL: https://github.com/nodejs/node/pull/43164 Reviewed-By: Antoine du Hamel Reviewed-By: Minwoo Jung Reviewed-By: Jacob Smith --- lib/internal/modules/cjs/loader.js | 6 +- lib/internal/modules/esm/fetch_module.js | 11 --- lib/internal/modules/esm/get_source.js | 60 ------------- .../modules/esm/initialize_import_meta.js | 4 +- lib/internal/modules/esm/load.js | 65 +++++++++++++- lib/internal/modules/esm/loader.js | 89 ++++++------------- lib/internal/modules/esm/module_job.js | 7 +- lib/internal/modules/esm/translators.js | 8 +- test/parallel/test-bootstrap-modules.js | 1 - 9 files changed, 97 insertions(+), 154 deletions(-) delete mode 100644 lib/internal/modules/esm/get_source.js diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 66f8e5f98ca891..34327a70045fca 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -1030,8 +1030,7 @@ function wrapSafe(filename, content, cjsModuleInstance) { displayErrors: true, importModuleDynamically: async (specifier, _, importAssertions) => { const loader = asyncESM.esmLoader; - return loader.import(specifier, - loader.getBaseURL(normalizeReferrerURL(filename)), + return loader.import(specifier, normalizeReferrerURL(filename), importAssertions); }, }); @@ -1047,8 +1046,7 @@ function wrapSafe(filename, content, cjsModuleInstance) { filename, importModuleDynamically(specifier, _, importAssertions) { const loader = asyncESM.esmLoader; - return loader.import(specifier, - loader.getBaseURL(normalizeReferrerURL(filename)), + return loader.import(specifier, normalizeReferrerURL(filename), importAssertions); }, }); diff --git a/lib/internal/modules/esm/fetch_module.js b/lib/internal/modules/esm/fetch_module.js index eeca8fc5e8edcc..c65587e34c488f 100644 --- a/lib/internal/modules/esm/fetch_module.js +++ b/lib/internal/modules/esm/fetch_module.js @@ -238,17 +238,6 @@ function fetchModule(parsed, { parentURL }) { return fetchWithRedirects(parsed); } -/** - * Checks if the given canonical URL exists in the fetch cache - * - * @param {string} key - * @returns {boolean} - */ -function inFetchCache(key) { - return cacheForGET.has(key); -} - module.exports = { fetchModule, - inFetchCache, }; diff --git a/lib/internal/modules/esm/get_source.js b/lib/internal/modules/esm/get_source.js deleted file mode 100644 index ab2a9888f76fe7..00000000000000 --- a/lib/internal/modules/esm/get_source.js +++ /dev/null @@ -1,60 +0,0 @@ -'use strict'; - -const { - ArrayPrototypeConcat, - RegExpPrototypeExec, - decodeURIComponent, -} = primordials; -const { getOptionValue } = require('internal/options'); -const { fetchModule } = require('internal/modules/esm/fetch_module'); - -// Do not eagerly grab .manifest, it may be in TDZ -const policy = getOptionValue('--experimental-policy') ? - require('internal/process/policy') : - null; -const experimentalNetworkImports = - getOptionValue('--experimental-network-imports'); - -const { Buffer: { from: BufferFrom } } = require('buffer'); - -const fs = require('internal/fs/promises').exports; -const { URL } = require('internal/url'); -const { - ERR_INVALID_URL, - ERR_UNSUPPORTED_ESM_URL_SCHEME, -} = require('internal/errors').codes; -const readFileAsync = fs.readFile; - -const DATA_URL_PATTERN = /^[^/]+\/[^,;]+(?:[^,]*?)(;base64)?,([\s\S]*)$/; - -async function defaultGetSource(url, context, defaultGetSource) { - const parsed = new URL(url); - let source; - if (parsed.protocol === 'file:') { - source = await readFileAsync(parsed); - } else if (parsed.protocol === 'data:') { - const match = RegExpPrototypeExec(DATA_URL_PATTERN, parsed.pathname); - if (!match) { - throw new ERR_INVALID_URL(url); - } - const { 1: base64, 2: body } = match; - source = BufferFrom(decodeURIComponent(body), base64 ? 'base64' : 'utf8'); - } else if (experimentalNetworkImports && ( - parsed.protocol === 'https:' || - parsed.protocol === 'http:' - )) { - const res = await fetchModule(parsed, context); - source = await res.body; - } else { - throw new ERR_UNSUPPORTED_ESM_URL_SCHEME(parsed, ArrayPrototypeConcat([ - 'file', - 'data', - experimentalNetworkImports ? ['https', 'http'] : [], - ])); - } - if (policy?.manifest) { - policy.manifest.assertIntegrity(parsed, source); - } - return source; -} -exports.defaultGetSource = defaultGetSource; diff --git a/lib/internal/modules/esm/initialize_import_meta.js b/lib/internal/modules/esm/initialize_import_meta.js index f1daabbb6425aa..d6be06f23e1493 100644 --- a/lib/internal/modules/esm/initialize_import_meta.js +++ b/lib/internal/modules/esm/initialize_import_meta.js @@ -26,15 +26,13 @@ function createImportMetaResolve(defaultParentUrl) { * @param {{url: string}} context */ function initializeImportMeta(meta, context) { - let url = context.url; + const { url } = context; // Alphabetical if (experimentalImportMetaResolve) { meta.resolve = createImportMetaResolve(url); } - url = asyncESM.esmLoader.getBaseURL(url); - meta.url = url; } diff --git a/lib/internal/modules/esm/load.js b/lib/internal/modules/esm/load.js index 6defb598a2abf7..86b31830457240 100644 --- a/lib/internal/modules/esm/load.js +++ b/lib/internal/modules/esm/load.js @@ -1,8 +1,67 @@ 'use strict'; +const { + ArrayPrototypePush, + RegExpPrototypeExec, + decodeURIComponent, +} = primordials; + const { defaultGetFormat } = require('internal/modules/esm/get_format'); -const { defaultGetSource } = require('internal/modules/esm/get_source'); const { validateAssertions } = require('internal/modules/esm/assert'); +const { getOptionValue } = require('internal/options'); +const { fetchModule } = require('internal/modules/esm/fetch_module'); + +// Do not eagerly grab .manifest, it may be in TDZ +const policy = getOptionValue('--experimental-policy') ? + require('internal/process/policy') : + null; +const experimentalNetworkImports = + getOptionValue('--experimental-network-imports'); + +const { Buffer: { from: BufferFrom } } = require('buffer'); + +const { readFile: readFileAsync } = require('internal/fs/promises').exports; +const { URL } = require('internal/url'); +const { + ERR_INVALID_URL, + ERR_UNSUPPORTED_ESM_URL_SCHEME, +} = require('internal/errors').codes; + +const DATA_URL_PATTERN = /^[^/]+\/[^,;]+(?:[^,]*?)(;base64)?,([\s\S]*)$/; + +async function getSource(url, context) { + const parsed = new URL(url); + let responseURL = url; + let source; + if (parsed.protocol === 'file:') { + source = await readFileAsync(parsed); + } else if (parsed.protocol === 'data:') { + const match = RegExpPrototypeExec(DATA_URL_PATTERN, parsed.pathname); + if (!match) { + throw new ERR_INVALID_URL(url); + } + const { 1: base64, 2: body } = match; + source = BufferFrom(decodeURIComponent(body), base64 ? 'base64' : 'utf8'); + } else if (experimentalNetworkImports && ( + parsed.protocol === 'https:' || + parsed.protocol === 'http:' + )) { + const res = await fetchModule(parsed, context); + source = await res.body; + responseURL = res.resolvedHREF; + } else { + const supportedSchemes = ['file', 'data']; + if (experimentalNetworkImports) { + ArrayPrototypePush(supportedSchemes, 'http', 'https'); + } + throw new ERR_UNSUPPORTED_ESM_URL_SCHEME(parsed, supportedSchemes); + } + if (policy?.manifest) { + policy.manifest.assertIntegrity(parsed, source); + } + return { responseURL, source }; +} + /** * Node.js default load hook. @@ -11,6 +70,7 @@ const { validateAssertions } = require('internal/modules/esm/assert'); * @returns {object} */ async function defaultLoad(url, context) { + let responseURL = url; const { importAssertions } = context; let { format, @@ -29,11 +89,12 @@ async function defaultLoad(url, context) { ) { source = null; } else if (source == null) { - source = await defaultGetSource(url, context); + ({ responseURL, source } = await getSource(url, context)); } return { format, + responseURL, source, }; } diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index c867595bccc9e2..c16f9122f1d0e6 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -17,14 +17,12 @@ const { RegExpPrototypeExec, SafeArrayIterator, SafeWeakMap, - StringPrototypeStartsWith, globalThis, } = primordials; const { MessageChannel } = require('internal/worker/io'); const { ERR_LOADER_CHAIN_INCOMPLETE, - ERR_INTERNAL_ASSERTION, ERR_INVALID_ARG_TYPE, ERR_INVALID_ARG_VALUE, ERR_INVALID_RETURN_PROPERTY_VALUE, @@ -55,11 +53,6 @@ const { defaultLoad } = require('internal/modules/esm/load'); const { translators } = require( 'internal/modules/esm/translators'); const { getOptionValue } = require('internal/options'); -const { - fetchModule, - inFetchCache, -} = require('internal/modules/esm/fetch_module'); - /** * @typedef {object} ExportedHooks @@ -306,9 +299,7 @@ class ESMLoader { const module = new ModuleWrap(url, undefined, source, 0, 0); callbackMap.set(module, { importModuleDynamically: (specifier, { url }, importAssertions) => { - return this.import(specifier, - this.getBaseURL(url), - importAssertions); + return this.import(specifier, url, importAssertions); } }); @@ -324,55 +315,6 @@ class ESMLoader { }; } - /** - * 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 - * 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 receiving the url body due to I/O - * @param {URL['href']} url - * @returns {string|Promise} - */ - getBaseURL(url) { - if (getOptionValue('--experimental-network-imports') && ( - StringPrototypeStartsWith(url, 'http:') || - StringPrototypeStartsWith(url, 'https:') - )) { - // When using network-imports, the request & response have already settled - // so they are in fetchModule's cache, in which case, fetchModule returns - // immediately and synchronously - // Unless a custom loader bypassed the fetch cache, in which case we just - // use the original url - if (inFetchCache(url)) { - const module = fetchModule(new URL(url), { parentURL: url }); - if (typeof module?.resolvedHREF === 'string') { - return module.resolvedHREF; - } - // Internal error - throw new ERR_INTERNAL_ASSERTION( - `Base url for module ${url} not loaded.` - ); - } else { - // A custom loader was used instead of network-imports. - // Adding support for a response URL resolve return in custom loaders is - // pending. - return url; - } - } - return url; - } - /** * Get a (possibly still pending) module job from the cache, * or create one and return its Promise. @@ -431,6 +373,7 @@ class ESMLoader { const moduleProvider = async (url, isMain) => { const { format: finalFormat, + responseURL, source, } = await this.load(url, { format, @@ -440,10 +383,10 @@ class ESMLoader { const translator = translators.get(finalFormat); if (!translator) { - throw new ERR_UNKNOWN_MODULE_FORMAT(finalFormat, url); + throw new ERR_UNKNOWN_MODULE_FORMAT(finalFormat, responseURL); } - return FunctionPrototypeCall(translator, this, url, source, isMain); + return FunctionPrototypeCall(translator, this, responseURL, source, isMain); }; const inspectBrk = ( @@ -607,6 +550,29 @@ class ESMLoader { format, source, } = loaded; + let responseURL = loaded.responseURL; + + if (responseURL === undefined) { + responseURL = url; + } + + let responseURLObj; + if (typeof responseURL === 'string') { + try { + responseURLObj = new URL(responseURL); + } catch { + // responseURLObj not defined will throw in next branch. + } + } + + if (responseURLObj?.href !== responseURL) { + throw new ERR_INVALID_RETURN_PROPERTY_VALUE( + 'undefined or a fully resolved URL string', + hookErrIdentifier, + 'responseURL', + responseURL, + ); + } if (format == null) { const dataUrl = RegExpPrototypeExec( @@ -644,6 +610,7 @@ class ESMLoader { return { format, + responseURL, source, }; } diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index d654958ad14cfd..fd1c6166330e76 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -76,12 +76,7 @@ class ModuleJob { // these `link` callbacks depending on each other. const dependencyJobs = []; const promises = this.module.link(async (specifier, assertions) => { - const base = await this.loader.getBaseURL(url); - const baseURL = typeof base === 'string' ? - base : - base.resolvedHREF; - - const jobPromise = this.loader.getModuleJob(specifier, baseURL, assertions); + const jobPromise = this.loader.getModuleJob(specifier, url, 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 d7f4c7edec63d3..bcd1775bac898e 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -103,9 +103,7 @@ function errPath(url) { } async function importModuleDynamically(specifier, { url }, assertions) { - return asyncESM.esmLoader.import(specifier, - asyncESM.esmLoader.getBaseURL(url), - assertions); + return asyncESM.esmLoader.import(specifier, url, assertions); } // Strategy for loading a standard JavaScript module. @@ -116,9 +114,7 @@ translators.set('module', async function moduleStrategy(url, source, isMain) { debug(`Translating StandardModule ${url}`); const module = new ModuleWrap(url, undefined, source, 0, 0); moduleWrap.callbackMap.set(module, { - initializeImportMeta: (meta, wrap) => this.importMetaInitialize(meta, { - url: wrap.url - }), + initializeImportMeta: (meta, wrap) => this.importMetaInitialize(meta, { url }), importModuleDynamically, }); return module; diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js index 9b9dc5c18590cd..703fbc6f10af3a 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -78,7 +78,6 @@ const expectedModules = new Set([ 'NativeModule internal/modules/esm/fetch_module', 'NativeModule internal/modules/esm/formats', 'NativeModule internal/modules/esm/get_format', - 'NativeModule internal/modules/esm/get_source', 'NativeModule internal/modules/esm/handle_process_exit', 'NativeModule internal/modules/esm/initialize_import_meta', 'NativeModule internal/modules/esm/load',