Skip to content

Commit 9577a00

Browse files
aduh95aduh95
committed
esm: fix cache collision on JSON files using file: URL
PR-URL: nodejs#49887 Fixes: nodejs#49724 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>
1 parent 39f107f commit 9577a00

File tree

5 files changed

+104
-10
lines changed

5 files changed

+104
-10
lines changed

‎lib/internal/modules/esm/translators.js

+7-3
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ const {
1111
SafeArrayIterator,
1212
SafeMap,
1313
SafeSet,
14+
StringPrototypeIncludes,
1415
StringPrototypeReplaceAll,
1516
StringPrototypeSlice,
1617
StringPrototypeStartsWith,
@@ -440,9 +441,12 @@ translators.set('json', function jsonStrategy(url, source) {
440441
debug(`Loading JSONModule ${url}`);
441442
const pathname = StringPrototypeStartsWith(url, 'file:') ?
442443
fileURLToPath(url) : null;
444+
const shouldCheckAndPopulateCJSModuleCache =
445+
// We want to involve the CJS loader cache only for `file:` URL with no search query and no hash.
446+
pathname && !StringPrototypeIncludes(url, '?') && !StringPrototypeIncludes(url, '#');
443447
let modulePath;
444448
let module;
445-
if (pathname) {
449+
if (shouldCheckAndPopulateCJSModuleCache) {
446450
modulePath = isWindows ?
447451
StringPrototypeReplaceAll(pathname, '/', '\\') : pathname;
448452
module = CJSModule._cache[modulePath];
@@ -454,7 +458,7 @@ translators.set('json', function jsonStrategy(url, source) {
454458
}
455459
}
456460
source = stringify(source);
457-
if (pathname) {
461+
if (shouldCheckAndPopulateCJSModuleCache) {
458462
// A require call could have been called on the same file during loading and
459463
// that resolves synchronously. To make sure we always return the identical
460464
// export, we have to check again if the module already exists or not.
@@ -481,7 +485,7 @@ translators.set('json', function jsonStrategy(url, source) {
481485
err.message = errPath(url) + ': ' + err.message;
482486
throw err;
483487
}
484-
if (pathname) {
488+
if (shouldCheckAndPopulateCJSModuleCache) {
485489
CJSModule._cache[modulePath] = module;
486490
}
487491
cjsCache.set(url, module);

‎test/es-module/test-esm-json.mjs

+61-1
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,10 @@ import { spawnPromisified } from '../common/index.mjs';
22
import * as fixtures from '../common/fixtures.mjs';
33
import assert from 'node:assert';
44
import { execPath } from 'node:process';
5-
import { describe, it } from 'node:test';
5+
import { describe, it, test } from 'node:test';
6+
7+
import { mkdir, rm, writeFile } from 'node:fs/promises';
8+
import * as tmpdir from '../common/tmpdir.js';
69

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

@@ -21,4 +24,61 @@ describe('ESM: importing JSON', () => {
2124
assert.strictEqual(code, 0);
2225
assert.strictEqual(signal, null);
2326
});
27+
28+
test('should load different modules when the URL is different', async (t) => {
29+
const root = tmpdir.fileURL(`./test-esm-json-${Math.random()}/`);
30+
try {
31+
await mkdir(root, { recursive: true });
32+
33+
await t.test('json', async () => {
34+
let i = 0;
35+
const url = new URL('./foo.json', root);
36+
await writeFile(url, JSON.stringify({ id: i++ }));
37+
const absoluteURL = await import(`${url}`, {
38+
assert: { type: 'json' },
39+
});
40+
await writeFile(url, JSON.stringify({ id: i++ }));
41+
const queryString = await import(`${url}?a=2`, {
42+
assert: { type: 'json' },
43+
});
44+
await writeFile(url, JSON.stringify({ id: i++ }));
45+
const hash = await import(`${url}#a=2`, {
46+
assert: { type: 'json' },
47+
});
48+
await writeFile(url, JSON.stringify({ id: i++ }));
49+
const queryStringAndHash = await import(`${url}?a=2#a=2`, {
50+
assert: { type: 'json' },
51+
});
52+
53+
assert.notDeepStrictEqual(absoluteURL, queryString);
54+
assert.notDeepStrictEqual(absoluteURL, hash);
55+
assert.notDeepStrictEqual(queryString, hash);
56+
assert.notDeepStrictEqual(absoluteURL, queryStringAndHash);
57+
assert.notDeepStrictEqual(queryString, queryStringAndHash);
58+
assert.notDeepStrictEqual(hash, queryStringAndHash);
59+
});
60+
61+
await t.test('js', async () => {
62+
let i = 0;
63+
const url = new URL('./foo.mjs', root);
64+
await writeFile(url, `export default ${JSON.stringify({ id: i++ })}\n`);
65+
const absoluteURL = await import(`${url}`);
66+
await writeFile(url, `export default ${JSON.stringify({ id: i++ })}\n`);
67+
const queryString = await import(`${url}?a=1`);
68+
await writeFile(url, `export default ${JSON.stringify({ id: i++ })}\n`);
69+
const hash = await import(`${url}#a=1`);
70+
await writeFile(url, `export default ${JSON.stringify({ id: i++ })}\n`);
71+
const queryStringAndHash = await import(`${url}?a=1#a=1`);
72+
73+
assert.notDeepStrictEqual(absoluteURL, queryString);
74+
assert.notDeepStrictEqual(absoluteURL, hash);
75+
assert.notDeepStrictEqual(queryString, hash);
76+
assert.notDeepStrictEqual(absoluteURL, queryStringAndHash);
77+
assert.notDeepStrictEqual(queryString, queryStringAndHash);
78+
assert.notDeepStrictEqual(hash, queryStringAndHash);
79+
});
80+
} finally {
81+
await rm(root, { force: true, recursive: true });
82+
}
83+
});
2484
});

‎test/es-module/test-esm-virtual-json.mjs

+30
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
import '../common/index.mjs';
2+
import * as fixtures from '../common/fixtures.mjs';
3+
import { register } from 'node:module';
4+
import assert from 'node:assert';
5+
6+
async function resolve(referrer, context, next) {
7+
const result = await next(referrer, context);
8+
const url = new URL(result.url);
9+
url.searchParams.set('randomSeed', Math.random());
10+
result.url = url.href;
11+
return result;
12+
}
13+
14+
function load(url, context, next) {
15+
if (context.importAssertions.type === 'json') {
16+
return {
17+
shortCircuit: true,
18+
format: 'json',
19+
source: JSON.stringify({ data: Math.random() }),
20+
};
21+
}
22+
return next(url, context);
23+
}
24+
25+
register(`data:text/javascript,export ${encodeURIComponent(resolve)};export ${encodeURIComponent(load)}`);
26+
27+
assert.notDeepStrictEqual(
28+
await import(fixtures.fileURL('empty.json'), { assert: { type: 'json' } }),
29+
await import(fixtures.fileURL('empty.json'), { assert: { type: 'json' } }),
30+
);

‎test/fixtures/errors/force_colors.snapshot

+5-5
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,11 @@ throw new Error('Should include grayed stack trace')
44

55
Error: Should include grayed stack trace
66
at Object.<anonymous> (/test*force_colors.js:1:7)
7-
 at Module._compile (node:internal*modules*cjs*loader:1241:14)
8-
 at Module._extensions..js (node:internal*modules*cjs*loader:1295:10)
9-
 at Module.load (node:internal*modules*cjs*loader:1091:32)
10-
 at Module._load (node:internal*modules*cjs*loader:938:12)
11-
 at Function.executeUserEntryPoint [as runMain] (node:internal*modules*run_main:83:12)
7+
 at Module._compile (node:internal*modules*cjs*loader:1406:14)
8+
 at Module._extensions..js (node:internal*modules*cjs*loader:1464:10)
9+
 at Module.load (node:internal*modules*cjs*loader:1239:32)
10+
 at Module._load (node:internal*modules*cjs*loader:1061:12)
11+
 at Function.executeUserEntryPoint [as runMain] (node:internal*modules*run_main:102:12)
1212
 at node:internal*main*run_main_module:23:47
1313

1414
Node.js *

‎test/fixtures/source-map/output/source_map_sourcemapping_url_string.snapshot

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
11
Error: an exception.
22
at Object.<anonymous> (*typescript-sourcemapping_url_string.ts:3:7)
3-
at Module._compile (node:internal*modules*cjs*loader:1241:14)
3+
at Module._compile (node:internal*modules*cjs*loader:1406:14)

0 commit comments

Comments
 (0)