From b0805e34251881b03b3063f1f40a3df0b9894e6c Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Wed, 25 May 2022 16:40:28 +0200 Subject: [PATCH] esm: fix http(s) import via custom loader PR-URL: https://github.com/nodejs/node/pull/43130 Reviewed-By: Geoffrey Booth Reviewed-By: Guy Bedford Reviewed-By: Antoine du Hamel --- lib/internal/modules/esm/fetch_module.js | 13 +++- lib/internal/modules/esm/loader.js | 37 ++++++---- lib/internal/modules/esm/module_job.js | 6 +- .../test-esm-loader-http-imports.mjs | 72 +++++++++++++++++++ .../es-module-loaders/http-loader.mjs | 40 +++++++++++ 5 files changed, 154 insertions(+), 14 deletions(-) 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/lib/internal/modules/esm/fetch_module.js b/lib/internal/modules/esm/fetch_module.js index f6c2fc8827aa73..eeca8fc5e8edcc 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 {string} key + * @returns {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 b1c652a47dd5ce..6391663d071420 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'); @@ -338,23 +339,35 @@ 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 ( + if (getOptionValue('--experimental-network-imports') && ( 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 - 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.`); + // 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; 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; 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..5f6cc47f388271 --- /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(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 new file mode 100644 index 00000000000000..f0add5d5b419f8 --- /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?.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); +}