Skip to content

Commit

Permalink
esm: fix cache collision on JSON files using file: URL
Browse files Browse the repository at this point in the history
PR-URL: #49887
Fixes: #49724
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
  • Loading branch information
aduh95 authored Sep 29, 2023
1 parent 8530180 commit 7f0e36a
Show file tree
Hide file tree
Showing 3 changed files with 98 additions and 4 deletions.
10 changes: 7 additions & 3 deletions lib/internal/modules/esm/translators.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const {
SafeArrayIterator,
SafeMap,
SafeSet,
StringPrototypeIncludes,
StringPrototypeReplaceAll,
StringPrototypeSlice,
StringPrototypeStartsWith,
Expand Down Expand Up @@ -443,9 +444,12 @@ translators.set('json', function jsonStrategy(url, source) {
debug(`Loading JSONModule ${url}`);
const pathname = StringPrototypeStartsWith(url, 'file:') ?
fileURLToPath(url) : null;
const shouldCheckAndPopulateCJSModuleCache =
// We want to involve the CJS loader cache only for `file:` URL with no search query and no hash.
pathname && !StringPrototypeIncludes(url, '?') && !StringPrototypeIncludes(url, '#');
let modulePath;
let module;
if (pathname) {
if (shouldCheckAndPopulateCJSModuleCache) {
modulePath = isWindows ?
StringPrototypeReplaceAll(pathname, '/', '\\') : pathname;
module = CJSModule._cache[modulePath];
Expand All @@ -457,7 +461,7 @@ translators.set('json', function jsonStrategy(url, source) {
}
}
source = stringify(source);
if (pathname) {
if (shouldCheckAndPopulateCJSModuleCache) {
// A require call could have been called on the same file during loading and
// that resolves synchronously. To make sure we always return the identical
// export, we have to check again if the module already exists or not.
Expand All @@ -484,7 +488,7 @@ translators.set('json', function jsonStrategy(url, source) {
err.message = errPath(url) + ': ' + err.message;
throw err;
}
if (pathname) {
if (shouldCheckAndPopulateCJSModuleCache) {
CJSModule._cache[modulePath] = module;
}
cjsCache.set(url, module);
Expand Down
62 changes: 61 additions & 1 deletion test/es-module/test-esm-json.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ import { spawnPromisified } from '../common/index.mjs';
import * as fixtures from '../common/fixtures.mjs';
import assert from 'node:assert';
import { execPath } from 'node:process';
import { describe, it } from 'node:test';
import { describe, it, test } from 'node:test';

import { mkdir, rm, writeFile } from 'node:fs/promises';
import * as tmpdir from '../common/tmpdir.js';

import secret from '../fixtures/experimental.json' assert { type: 'json' };

Expand All @@ -21,4 +24,61 @@ describe('ESM: importing JSON', () => {
assert.strictEqual(code, 0);
assert.strictEqual(signal, null);
});

test('should load different modules when the URL is different', async (t) => {
const root = tmpdir.fileURL(`./test-esm-json-${Math.random()}/`);
try {
await mkdir(root, { recursive: true });

await t.test('json', async () => {
let i = 0;
const url = new URL('./foo.json', root);
await writeFile(url, JSON.stringify({ id: i++ }));
const absoluteURL = await import(`${url}`, {
assert: { type: 'json' },
});
await writeFile(url, JSON.stringify({ id: i++ }));
const queryString = await import(`${url}?a=2`, {
assert: { type: 'json' },
});
await writeFile(url, JSON.stringify({ id: i++ }));
const hash = await import(`${url}#a=2`, {
assert: { type: 'json' },
});
await writeFile(url, JSON.stringify({ id: i++ }));
const queryStringAndHash = await import(`${url}?a=2#a=2`, {
assert: { type: 'json' },
});

assert.notDeepStrictEqual(absoluteURL, queryString);
assert.notDeepStrictEqual(absoluteURL, hash);
assert.notDeepStrictEqual(queryString, hash);
assert.notDeepStrictEqual(absoluteURL, queryStringAndHash);
assert.notDeepStrictEqual(queryString, queryStringAndHash);
assert.notDeepStrictEqual(hash, queryStringAndHash);
});

await t.test('js', async () => {
let i = 0;
const url = new URL('./foo.mjs', root);
await writeFile(url, `export default ${JSON.stringify({ id: i++ })}\n`);
const absoluteURL = await import(`${url}`);
await writeFile(url, `export default ${JSON.stringify({ id: i++ })}\n`);
const queryString = await import(`${url}?a=1`);
await writeFile(url, `export default ${JSON.stringify({ id: i++ })}\n`);
const hash = await import(`${url}#a=1`);
await writeFile(url, `export default ${JSON.stringify({ id: i++ })}\n`);
const queryStringAndHash = await import(`${url}?a=1#a=1`);

assert.notDeepStrictEqual(absoluteURL, queryString);
assert.notDeepStrictEqual(absoluteURL, hash);
assert.notDeepStrictEqual(queryString, hash);
assert.notDeepStrictEqual(absoluteURL, queryStringAndHash);
assert.notDeepStrictEqual(queryString, queryStringAndHash);
assert.notDeepStrictEqual(hash, queryStringAndHash);
});
} finally {
await rm(root, { force: true, recursive: true });
}
});
});
30 changes: 30 additions & 0 deletions test/es-module/test-esm-virtual-json.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import '../common/index.mjs';
import * as fixtures from '../common/fixtures.mjs';
import { register } from 'node:module';
import assert from 'node:assert';

async function resolve(referrer, context, next) {
const result = await next(referrer, context);
const url = new URL(result.url);
url.searchParams.set('randomSeed', Math.random());
result.url = url.href;
return result;
}

function load(url, context, next) {
if (context.importAssertions.type === 'json') {
return {
shortCircuit: true,
format: 'json',
source: JSON.stringify({ data: Math.random() }),
};
}
return next(url, context);
}

register(`data:text/javascript,export ${encodeURIComponent(resolve)};export ${encodeURIComponent(load)}`);

assert.notDeepStrictEqual(
await import(fixtures.fileURL('empty.json'), { assert: { type: 'json' } }),
await import(fixtures.fileURL('empty.json'), { assert: { type: 'json' } }),
);

0 comments on commit 7f0e36a

Please sign in to comment.