Skip to content

Commit

Permalink
module: add application/json in accept header when fetching json module
Browse files Browse the repository at this point in the history
PR-URL: #50119
Refs: #50116
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
  • Loading branch information
marco-ippolito authored and targos committed Nov 14, 2023
1 parent 921e36e commit 667d245
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 6 deletions.
26 changes: 20 additions & 6 deletions lib/internal/modules/esm/fetch_module.js
Original file line number Diff line number Diff line change
Expand Up @@ -134,19 +134,32 @@ function isRedirect(statusCode) {
}
}

/**
* @typedef AcceptMimes possible values of Accept header when fetching a module
* @property {Promise<string> | string} default default Accept header value.
* @property {Record<string, string>} json Accept header value when fetching module with importAttributes json.
* @type {AcceptMimes}
*/
const acceptMimes = {
__proto_: null,
default: '*/*',
json: 'application/json,*/*;charset=utf-8;q=0.5',
};

/**
* @param {URL} parsed
* @returns {Promise<CacheEntry> | CacheEntry}
*/
function fetchWithRedirects(parsed) {
function fetchWithRedirects(parsed, context) {
const existing = cacheForGET.get(parsed.href);
if (existing) {
return existing;
}
const handler = parsed.protocol === 'http:' ? HTTPGet : HTTPSGet;
const result = (async () => {
const accept = acceptMimes[context.importAttributes?.type] ?? acceptMimes.default;
const req = handler(parsed, {
headers: { Accept: '*/*' },
headers: { Accept: accept },
});
// Note that `once` is used here to handle `error` and that it hits the
// `finally` on network error/timeout.
Expand All @@ -162,7 +175,7 @@ function fetchWithRedirects(parsed) {
'cannot redirect to non-network location',
);
}
const entry = await fetchWithRedirects(location);
const entry = await fetchWithRedirects(location, context);
cacheForGET.set(parsed.href, entry);
return entry;
}
Expand Down Expand Up @@ -262,7 +275,8 @@ async function isLocalAddress(hostname) {
* @param {ESModuleContext} context
* @returns {ReturnType<typeof fetchWithRedirects>}
*/
function fetchModule(parsed, { parentURL }) {
function fetchModule(parsed, context) {
const { parentURL } = context;
const { href } = parsed;
const existing = cacheForGET.get(href);
if (existing) {
Expand All @@ -277,10 +291,10 @@ function fetchModule(parsed, { parentURL }) {
'http can only be used to load local resources (use https instead).',
);
}
return fetchWithRedirects(parsed);
return fetchWithRedirects(parsed, context);
});
}
return fetchWithRedirects(parsed);
return fetchWithRedirects(parsed, context);
}

module.exports = {
Expand Down
12 changes: 12 additions & 0 deletions test/es-module/test-http-imports.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,11 @@ for (const { protocol, createServer } of [
const server = createServer(function(_req, res) {
const url = new URL(_req.url, host);
const redirect = url.searchParams.get('redirect');

if (url.pathname === 'json') {
common.mustCall(() => assert.strictEqual(_req.header.content, 'application/json,*/*;charset=utf-8;q=0.5'));
}

if (url.pathname === '/not-found') {
res.writeHead(404);
res.end();
Expand Down Expand Up @@ -204,6 +209,13 @@ for (const { protocol, createServer } of [
{ code: 'ERR_MODULE_NOT_FOUND' },
);

const jsonUrl = new URL(url.href + 'json');
jsonUrl.searchParams.set('mime', 'application/json');
jsonUrl.searchParams.set('body', '{"x": 1}');
const json = await import(jsonUrl.href, { with: { type: 'json' } });
assert.deepStrictEqual(Object.keys(json), ['default']);
assert.strictEqual(json.default.x, 1);

server.close();
}
}

0 comments on commit 667d245

Please sign in to comment.