From 8b2861a8282cb048829b7b86ddb36d136f83e56b Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Tue, 26 Sep 2023 23:44:57 +0200 Subject: [PATCH 1/4] esm: fix cache collision on JSON files using file: URL --- lib/internal/modules/esm/translators.js | 9 ++++-- test/es-module/test-esm-json.mjs | 41 +++++++++++++++++++++++- test/es-module/test-esm-virtual-json.mjs | 30 +++++++++++++++++ 3 files changed, 76 insertions(+), 4 deletions(-) create mode 100644 test/es-module/test-esm-virtual-json.mjs diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index bd67593f993e07..f97e6139268832 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -11,6 +11,7 @@ const { SafeArrayIterator, SafeMap, SafeSet, + StringPrototypeIncludes, StringPrototypeReplaceAll, StringPrototypeSlice, StringPrototypeStartsWith, @@ -443,9 +444,11 @@ translators.set('json', function jsonStrategy(url, source) { debug(`Loading JSONModule ${url}`); const pathname = StringPrototypeStartsWith(url, 'file:') ? fileURLToPath(url) : null; + const shouldCheckAndPopulateCJSModuleCache = + pathname && !StringPrototypeIncludes(url, '?') && !StringPrototypeIncludes(url, '#'); let modulePath; let module; - if (pathname) { + if (shouldCheckAndPopulateCJSModuleCache) { modulePath = isWindows ? StringPrototypeReplaceAll(pathname, '/', '\\') : pathname; module = CJSModule._cache[modulePath]; @@ -457,7 +460,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. @@ -484,7 +487,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); diff --git a/test/es-module/test-esm-json.mjs b/test/es-module/test-esm-json.mjs index 2740c0097f77da..c8795c1e4e1808 100644 --- a/test/es-module/test-esm-json.mjs +++ b/test/es-module/test-esm-json.mjs @@ -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' }; @@ -21,4 +24,40 @@ 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 () => { + const url = new URL('./foo.json', root); + await writeFile(url, JSON.stringify({ firstJson: true })); + const firstJson = await import(`${url}?a=1`, { + assert: { type: 'json' }, + }); + await writeFile(url, JSON.stringify({ firstJson: false })); + const secondJson = await import(`${url}?a=2`, { + assert: { type: 'json' }, + }); + + assert.notDeepStrictEqual(firstJson, secondJson); + }); + + await t.test('js', async () => { + const url = new URL('./foo.mjs', root); + await writeFile(url, ` + export const firstJS = true + `); + const firstJS = await import(`${url}?a=1`); + await writeFile(url, ` + export const firstJS = false + `); + const secondJS = await import(`${url}?a=2`); + assert.notDeepStrictEqual(firstJS, secondJS); + }); + } finally { + await rm(root, { force: true, recursive: true }); + } + }); }); diff --git a/test/es-module/test-esm-virtual-json.mjs b/test/es-module/test-esm-virtual-json.mjs new file mode 100644 index 00000000000000..8ff185a428ef01 --- /dev/null +++ b/test/es-module/test-esm-virtual-json.mjs @@ -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' } }), +); From cdd3ad895467a00a9fd12acfcfcc71312eb69012 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 27 Sep 2023 01:51:19 +0200 Subject: [PATCH 2/4] Update lib/internal/modules/esm/translators.js --- lib/internal/modules/esm/translators.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index f97e6139268832..f175fdea52ac90 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -445,6 +445,7 @@ translators.set('json', function jsonStrategy(url, source) { 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; From ea1eafbe969ca67bd7be91589ff153357173d7c6 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 27 Sep 2023 13:35:43 +0200 Subject: [PATCH 3/4] increase coverage --- test/es-module/test-esm-json.mjs | 49 +++++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 14 deletions(-) diff --git a/test/es-module/test-esm-json.mjs b/test/es-module/test-esm-json.mjs index c8795c1e4e1808..82232838b79150 100644 --- a/test/es-module/test-esm-json.mjs +++ b/test/es-module/test-esm-json.mjs @@ -31,30 +31,51 @@ describe('ESM: importing JSON', () => { 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({ firstJson: true })); - const firstJson = await import(`${url}?a=1`, { + await writeFile(url, JSON.stringify({ id: i++ })); + const absoluteURL = await import(`${url}`, { assert: { type: 'json' }, }); - await writeFile(url, JSON.stringify({ firstJson: false })); - const secondJson = await import(`${url}?a=2`, { + 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(firstJson, secondJson); + 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 const firstJS = true - `); - const firstJS = await import(`${url}?a=1`); - await writeFile(url, ` - export const firstJS = false - `); - const secondJS = await import(`${url}?a=2`); - assert.notDeepStrictEqual(firstJS, secondJS); + 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 }); From 5f2f868df4a88a895defe6143deb0e550b00f047 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Fri, 29 Sep 2023 00:13:11 +0200 Subject: [PATCH 4/4] Update lib/internal/modules/esm/translators.js Co-authored-by: Jordan Harband --- lib/internal/modules/esm/translators.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index f175fdea52ac90..cf9afb741aab85 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -445,7 +445,7 @@ translators.set('json', function jsonStrategy(url, source) { 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. + // 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;