Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

esm: fix base URL for network imports #42131

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions lib/internal/modules/cjs/helpers.js
Expand Up @@ -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;
Expand Down
6 changes: 4 additions & 2 deletions lib/internal/modules/cjs/loader.js
Expand Up @@ -1017,7 +1017,8 @@ function wrapSafe(filename, content, cjsModuleInstance) {
displayErrors: true,
importModuleDynamically: async (specifier, _, importAssertions) => {
bmeck marked this conversation as resolved.
Show resolved Hide resolved
const loader = asyncESM.esmLoader;
return loader.import(specifier, normalizeReferrerURL(filename),
return loader.import(specifier,
loader.getBaseURL(normalizeReferrerURL(filename)),
importAssertions);
},
});
Expand All @@ -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.getBaseURL(normalizeReferrerURL(filename)),
importAssertions);
},
});
Expand Down
16 changes: 5 additions & 11 deletions lib/internal/modules/esm/initialize_import_meta.js
Expand Up @@ -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');

Expand All @@ -24,6 +21,10 @@ function createImportMetaResolve(defaultParentUrl) {
};
}

/**
* @param {object} meta
* @param {{url: string}} context
*/
function initializeImportMeta(meta, context) {
let url = context.url;

Expand All @@ -32,14 +33,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.getBaseURL(url);

meta.url = url;
}
Expand Down
46 changes: 45 additions & 1 deletion lib/internal/modules/esm/loader.js
Expand Up @@ -17,11 +17,13 @@ const {
RegExpPrototypeExec,
SafeArrayIterator,
SafeWeakMap,
StringPrototypeStartsWith,
globalThis,
} = primordials;
const { MessageChannel } = require('internal/worker/io');

const {
ERR_INTERNAL_ASSERTION,
ERR_INVALID_ARG_TYPE,
ERR_INVALID_ARG_VALUE,
ERR_INVALID_RETURN_PROPERTY_VALUE,
Expand All @@ -47,6 +49,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.
Expand Down Expand Up @@ -209,7 +214,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.getBaseURL(url),
importAssertions);
}
});

Expand All @@ -225,6 +232,43 @@ 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
bmeck marked this conversation as resolved.
Show resolved Hide resolved
* synchronous resolution using only string operations
* ~= resolveImportMap(new URL(specifier, importerHREF))
*
* The url used for subsequent resolution is the response URL after
bmeck marked this conversation as resolved.
Show resolved Hide resolved
* 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
bmeck marked this conversation as resolved.
Show resolved Hide resolved
*
* MUST BE SYNCHRONOUS for import.meta initialization
* MUST BE CALLED AFTER receiving the url body due to I/O
bmeck marked this conversation as resolved.
Show resolved Hide resolved
bmeck marked this conversation as resolved.
Show resolved Hide resolved
* @param {string} url
* @returns {string}
bmeck marked this conversation as resolved.
Show resolved Hide resolved
*/
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
// immediately and synchronously
url = fetchModule(new URL(url), { parentURL: url }).resolvedHREF;
bmeck marked this conversation as resolved.
Show resolved Hide resolved
// 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;
}

/**
* Get a (possibly still pending) module job from the cache,
* or create one and return its Promise.
Expand Down
3 changes: 2 additions & 1 deletion lib/internal/modules/esm/module_job.js
Expand Up @@ -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.getBaseURL(url);
const jobPromise = this.loader.getModuleJob(specifier, baseURL, assertions);
ArrayPrototypePush(dependencyJobs, jobPromise);
const job = await jobPromise;
return job.modulePromise;
Expand Down
4 changes: 3 additions & 1 deletion lib/internal/modules/esm/translators.js
Expand Up @@ -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.getBaseURL(url),
assertions);
}

// Strategy for loading a standard JavaScript module.
Expand Down
15 changes: 15 additions & 0 deletions test/es-module/test-http-imports.mjs
Expand Up @@ -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 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');
bmeck marked this conversation as resolved.
Show resolved Hide resolved
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,
Expand Down