From e1ebe9a5f1db2d78bff246341fcd944b98feb56c Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Tue, 17 May 2022 21:00:27 +0200 Subject: [PATCH 1/8] esm: fix http(s) import via custom loader --- lib/internal/modules/esm/loader.js | 17 +++++++++++++---- lib/internal/modules/esm/module_job.js | 6 +++++- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 8bc22ef697db3e..2ce307cfc4963f 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -351,12 +351,21 @@ 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; - // This should only occur if the module hasn't been fetched yet - if (typeof url !== 'string') { // [2] - throw new ERR_INTERNAL_ASSERTION(`Base url for module ${url} not loaded.`); + const module = fetchModule(new URL(url), { parentURL: url }); + + if (typeof module?.resolvedHREF === 'string') { // [2] + url = module.resolvedHREF; + } else { // This should only occur if the module hasn't been fetched yet + if (getOptionValue('--experimental-network-imports')) { + throw new ERR_INTERNAL_ASSERTION( + `Base url for module ${url} not loaded.` + ); + } else { + url = module; + } } } + return url; } diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index e012eebc4ac971..d654958ad14cfd 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -76,7 +76,11 @@ class ModuleJob { // these `link` callbacks depending on each other. const dependencyJobs = []; const promises = this.module.link(async (specifier, assertions) => { - const baseURL = this.loader.getBaseURL(url); + const base = await this.loader.getBaseURL(url); + const baseURL = typeof base === 'string' ? + base : + base.resolvedHREF; + const jobPromise = this.loader.getModuleJob(specifier, baseURL, assertions); ArrayPrototypePush(dependencyJobs, jobPromise); const job = await jobPromise; From b1d06bd886960e88e6b02d7c88085663a153907c Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Fri, 20 May 2022 10:50:26 +0200 Subject: [PATCH 2/8] fixup: add test case --- .../test-esm-loader-http-imports.mjs | 72 +++++++++++++++++++ .../es-module-loaders/http-loader.mjs | 40 +++++++++++ 2 files changed, 112 insertions(+) create mode 100644 test/es-module/test-esm-loader-http-imports.mjs create mode 100644 test/fixtures/es-module-loaders/http-loader.mjs diff --git a/test/es-module/test-esm-loader-http-imports.mjs b/test/es-module/test-esm-loader-http-imports.mjs new file mode 100644 index 00000000000000..d81ef581b9933a --- /dev/null +++ b/test/es-module/test-esm-loader-http-imports.mjs @@ -0,0 +1,72 @@ +import { mustCall } from '../common/index.mjs'; +import fixtures from '../common/fixtures.js'; +import { strictEqual } from 'node:assert'; +import { spawn } from 'node:child_process'; +import http from 'node:http'; +import path from 'node:path'; +import { promisify } from 'node:util'; + + +const files = { + 'main.mjs': `export * from './lib.mjs';`, + 'lib.mjs': `export { sum } from './sum.mjs';`, + 'sum.mjs': `export function sum(a, b) { return a + b }`, +}; + +const requestListener = ({ url }, rsp) => { + const filename = path.basename(url); + const content = files[filename]; + + if (content) { + return rsp + .writeHead(200, { 'Content-Type': 'application/javascript' }) + .end(content); + } + + return rsp + .writeHead(404) + .end(); +}; + +const server = http.createServer(requestListener); + +await promisify(server.listen.bind(server))({ + host: '127.0.0.1', + port: 0, +}); + +const { + address: host, + port, +} = server.address(); + +{ // Verify nested HTTP imports work + const child = spawn( // `spawn` MUST be used (vs `spawnSync`) to avoid blocking the event loop + process.execPath, + [ + '--no-warnings', + `--loader`, + fixtures.fileURL('es-module-loaders', 'http-loader.mjs'), + '--input-type=module', + '--eval', + `import * as main from 'http://${host}:${port}/main.mjs'; console.log(main)`, + ] + ); + + let stderr = ''; + let stdout = ''; + + child.stderr.setEncoding('utf8'); + child.stderr.on('data', (data) => stderr += data); + child.stdout.setEncoding('utf8'); + child.stdout.on('data', (data) => stdout += data); + + child.on('close', mustCall((code, signal) => { + strictEqual(code, 0); + strictEqual(signal, null); + strictEqual(stderr, ''); + strictEqual(stdout, '[Module: null prototype] { sum: [Function: sum] }\n'); + + server.close(); + })); +} diff --git a/test/fixtures/es-module-loaders/http-loader.mjs b/test/fixtures/es-module-loaders/http-loader.mjs new file mode 100644 index 00000000000000..2f6bf056b7fec1 --- /dev/null +++ b/test/fixtures/es-module-loaders/http-loader.mjs @@ -0,0 +1,40 @@ +import { get } from 'http'; + +export function resolve(specifier, context, nextResolve) { + const { parentURL = null } = context; + + if (specifier.startsWith('http://')) { + return { + shortCircuit: true, + url: specifier + }; + } else if (parentURL && parentURL.startsWith('http://')) { + return { + shortCircuit: true, + url: new URL(specifier, parentURL).href + }; + } + + return nextResolve(specifier, context); +} + +export function load(url, context, nextLoad) { + if (url.startsWith('http://')) { + return new Promise((resolve, reject) => { + get(url, (rsp) => { + let data = ''; + rsp.on('data', (chunk) => data += chunk); + rsp.on('end', () => { + resolve({ + format: 'module', + shortCircuit: true, + source: data, + }); + }); + }) + .on('error', reject); + }); + } + + return nextLoad(url, context); +} From 714f94ef0dfd1d882f89be2c986fd861aa79a1bf Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Fri, 20 May 2022 18:17:57 +0200 Subject: [PATCH 3/8] fixup: expand code docs for `ESMLoader::getBaseURL` and update return doc --- lib/internal/modules/esm/loader.js | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 2ce307cfc4963f..672a0a09c0818c 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -338,19 +338,21 @@ class ESMLoader { * 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 {string} url - * @returns {string} + * ! 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 ( StringPrototypeStartsWith(url, 'http:') || StringPrototypeStartsWith(url, 'https:') ) { - // The request & response have already settled, so they are in - // fetchModule's cache, in which case, fetchModule returns + // 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 + // BUT, when a custom loader is used, the cache is bypassed and it hits an + // unsettled promise. const module = fetchModule(new URL(url), { parentURL: url }); if (typeof module?.resolvedHREF === 'string') { // [2] @@ -360,8 +362,8 @@ class ESMLoader { throw new ERR_INTERNAL_ASSERTION( `Base url for module ${url} not loaded.` ); - } else { - url = module; + } else { // a custom loader was used instead of network-imports + url = module; // ! this is an unsettled promise } } } From 8aaa87301764a3595740ce0733a6c052e392029e Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Fri, 20 May 2022 18:34:05 +0200 Subject: [PATCH 4/8] fixup: de-lint --- lib/internal/modules/esm/loader.js | 4 ++-- test/es-module/test-esm-loader-http-imports.mjs | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 672a0a09c0818c..bb4b330a02c1ee 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -362,9 +362,9 @@ class ESMLoader { throw new ERR_INTERNAL_ASSERTION( `Base url for module ${url} not loaded.` ); - } else { // a custom loader was used instead of network-imports - url = module; // ! this is an unsettled promise } + // A custom loader was used instead of network-imports + url = module; // ! this is an unsettled promise } } diff --git a/test/es-module/test-esm-loader-http-imports.mjs b/test/es-module/test-esm-loader-http-imports.mjs index d81ef581b9933a..c405f56770d6d8 100644 --- a/test/es-module/test-esm-loader-http-imports.mjs +++ b/test/es-module/test-esm-loader-http-imports.mjs @@ -8,9 +8,9 @@ import { promisify } from 'node:util'; const files = { - 'main.mjs': `export * from './lib.mjs';`, - 'lib.mjs': `export { sum } from './sum.mjs';`, - 'sum.mjs': `export function sum(a, b) { return a + b }`, + 'main.mjs': 'export * from "./lib.mjs";', + 'lib.mjs': 'export { sum } from "./sum.mjs";', + 'sum.mjs': 'export function sum(a, b) { return a + b }', }; const requestListener = ({ url }, rsp) => { @@ -45,7 +45,7 @@ const { process.execPath, [ '--no-warnings', - `--loader`, + '--loader', fixtures.fileURL('es-module-loaders', 'http-loader.mjs'), '--input-type=module', '--eval', From 59a5de3fa8360c124f95b29973cf71d877f457fb Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Fri, 20 May 2022 18:35:00 +0200 Subject: [PATCH 5/8] fixup: emphasis gotcha --- test/es-module/test-esm-loader-http-imports.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/es-module/test-esm-loader-http-imports.mjs b/test/es-module/test-esm-loader-http-imports.mjs index c405f56770d6d8..96dbe943419ad8 100644 --- a/test/es-module/test-esm-loader-http-imports.mjs +++ b/test/es-module/test-esm-loader-http-imports.mjs @@ -41,7 +41,7 @@ const { } = server.address(); { // Verify nested HTTP imports work - const child = spawn( // `spawn` MUST be used (vs `spawnSync`) to avoid blocking the event loop + const child = spawn( // ! `spawn` MUST be used (vs `spawnSync`) to avoid blocking the event loop process.execPath, [ '--no-warnings', From 98d262d5499bab9089de4479a249ba2ae28360a0 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Fri, 20 May 2022 16:14:58 -0700 Subject: [PATCH 6/8] use inFetchCache guard --- lib/internal/modules/esm/fetch_module.js | 13 ++++++++++- lib/internal/modules/esm/loader.js | 29 +++++++++++++----------- 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/lib/internal/modules/esm/fetch_module.js b/lib/internal/modules/esm/fetch_module.js index f6c2fc8827aa73..47bf5e5520b9a8 100644 --- a/lib/internal/modules/esm/fetch_module.js +++ b/lib/internal/modules/esm/fetch_module.js @@ -238,6 +238,17 @@ function fetchModule(parsed, { parentURL }) { return fetchWithRedirects(parsed); } +/** + * Checks if the given canonical URL exists in the fetch cache + * + * @param {url} string + * @returns {in_cache} boolean + */ +function inFetchCache(key) { + return cacheForGET.has(key); +} + module.exports = { - fetchModule: fetchModule, + fetchModule, + inFetchCache, }; diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index bb4b330a02c1ee..07d3b4c1a0de95 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -57,6 +57,7 @@ const { translators } = require( const { getOptionValue } = require('internal/options'); const { fetchModule, + inFetchCache, } = require('internal/modules/esm/fetch_module'); @@ -344,30 +345,32 @@ class ESMLoader { * @returns {string|Promise} */ getBaseURL(url) { - if ( + 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 - // BUT, when a custom loader is used, the cache is bypassed and it hits an - // unsettled promise. - const module = fetchModule(new URL(url), { parentURL: url }); - - if (typeof module?.resolvedHREF === 'string') { // [2] - url = module.resolvedHREF; - } else { // This should only occur if the module hasn't been fetched yet - if (getOptionValue('--experimental-network-imports')) { + // 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; + } else { + // Internal error throw new ERR_INTERNAL_ASSERTION( `Base url for module ${url} not loaded.` ); } - // A custom loader was used instead of network-imports - url = module; // ! this is an unsettled promise + } 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; } From 12696a5ecef3508713bf0f98f2e0a6ab84e00096 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Fri, 20 May 2022 16:19:35 -0700 Subject: [PATCH 7/8] lint fixes --- lib/internal/modules/esm/fetch_module.js | 4 ++-- lib/internal/modules/esm/loader.js | 9 ++++----- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/lib/internal/modules/esm/fetch_module.js b/lib/internal/modules/esm/fetch_module.js index 47bf5e5520b9a8..eeca8fc5e8edcc 100644 --- a/lib/internal/modules/esm/fetch_module.js +++ b/lib/internal/modules/esm/fetch_module.js @@ -241,8 +241,8 @@ function fetchModule(parsed, { parentURL }) { /** * Checks if the given canonical URL exists in the fetch cache * - * @param {url} string - * @returns {in_cache} boolean + * @param {string} key + * @returns {boolean} */ function inFetchCache(key) { return cacheForGET.has(key); diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 07d3b4c1a0de95..c867595bccc9e2 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -358,12 +358,11 @@ class ESMLoader { const module = fetchModule(new URL(url), { parentURL: url }); if (typeof module?.resolvedHREF === 'string') { return module.resolvedHREF; - } else { - // Internal error - throw new ERR_INTERNAL_ASSERTION( - `Base url for module ${url} not loaded.` - ); } + // 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 From eec825360cb38d43dc23cfd03c40ae7fe39296d5 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Tue, 24 May 2022 19:25:31 +0200 Subject: [PATCH 8/8] Apply suggestions from code review --- test/es-module/test-esm-loader-http-imports.mjs | 4 ++-- test/fixtures/es-module-loaders/http-loader.mjs | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/test/es-module/test-esm-loader-http-imports.mjs b/test/es-module/test-esm-loader-http-imports.mjs index 96dbe943419ad8..5f6cc47f388271 100644 --- a/test/es-module/test-esm-loader-http-imports.mjs +++ b/test/es-module/test-esm-loader-http-imports.mjs @@ -62,10 +62,10 @@ const { child.stdout.on('data', (data) => stdout += data); child.on('close', mustCall((code, signal) => { - strictEqual(code, 0); - strictEqual(signal, null); strictEqual(stderr, ''); strictEqual(stdout, '[Module: null prototype] { sum: [Function: sum] }\n'); + strictEqual(code, 0); + strictEqual(signal, null); server.close(); })); diff --git a/test/fixtures/es-module-loaders/http-loader.mjs b/test/fixtures/es-module-loaders/http-loader.mjs index 2f6bf056b7fec1..f0add5d5b419f8 100644 --- a/test/fixtures/es-module-loaders/http-loader.mjs +++ b/test/fixtures/es-module-loaders/http-loader.mjs @@ -6,12 +6,12 @@ export function resolve(specifier, context, nextResolve) { if (specifier.startsWith('http://')) { return { shortCircuit: true, - url: specifier + url: specifier, }; - } else if (parentURL && parentURL.startsWith('http://')) { + } else if (parentURL?.startsWith('http://')) { return { shortCircuit: true, - url: new URL(specifier, parentURL).href + url: new URL(specifier, parentURL).href, }; }