From a9fdaedb115f53edc1efae834f91af94b5728951 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 12 Sep 2025 22:35:53 +0200 Subject: [PATCH 1/2] module: only put directly require-d ESM into require.cache This reduces the impact of https://redirect.github.com/nodejs/node/pull/59679 by delaying the require.cache population of ESM until they are directly required. After that, it's necessary for them to be in the cache to maintain correctness. --- lib/internal/modules/esm/loader.js | 13 ---------- lib/internal/modules/esm/translators.js | 19 +++++++++++--- .../es-module/test-esm-in-require-cache-2.mjs | 26 +++++++++++++++++++ test/es-module/test-esm-in-require-cache.js | 25 ++++++++++++++++++ .../es-modules/esm-in-require-cache/esm.mjs | 1 + .../esm-in-require-cache/import-esm.mjs | 1 + .../import-require-esm.mjs | 1 + .../esm-in-require-cache/require-esm.cjs | 9 +++++++ .../require-import-esm.cjs | 1 + 9 files changed, 80 insertions(+), 16 deletions(-) create mode 100644 test/es-module/test-esm-in-require-cache-2.mjs create mode 100644 test/es-module/test-esm-in-require-cache.js create mode 100644 test/fixtures/es-modules/esm-in-require-cache/esm.mjs create mode 100644 test/fixtures/es-modules/esm-in-require-cache/import-esm.mjs create mode 100644 test/fixtures/es-modules/esm-in-require-cache/import-require-esm.mjs create mode 100644 test/fixtures/es-modules/esm-in-require-cache/require-esm.cjs create mode 100644 test/fixtures/es-modules/esm-in-require-cache/require-import-esm.cjs diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index cdf92206f54190..abd8eaf02c05cc 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -560,19 +560,6 @@ class ModuleLoader { throw new ERR_UNKNOWN_MODULE_FORMAT(format, url); } - // Populate the CJS cache with a facade for ESM in case subsequent require(esm) is - // looking it up from the cache. The parent module of the CJS cache entry would be the - // first CJS module that loads it with require(). This is an approximation, because - // ESM caches more and it does not get re-loaded and updated every time an `import` is - // encountered, unlike CJS require(), and we only use the parent entry to provide - // more information in error messages. - if (format === 'module') { - const parentFilename = urlToFilename(parentURL); - const parent = parentFilename ? CJSModule._cache[parentFilename] : undefined; - const cjsModule = lazyLoadTranslators().cjsEmplaceModuleCacheEntryForURL(url, parent); - debug('cjsEmplaceModuleCacheEntryForURL', url, parent, cjsModule); - } - const result = FunctionPrototypeCall(translator, this, url, source, parentURL === undefined); assert(result instanceof ModuleWrap); return result; diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index 7c8e20caf0182c..147d96bda8098f 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -154,9 +154,22 @@ function loadCJSModule(module, source, url, filename, isMain) { // requires a separate cache to be populated as well as introducing several quirks. This is not ideal. const job = cascadedLoader.getModuleJobForRequireInImportedCJS(specifier, url, importAttributes); job.runSync(); - const mod = cjsCache.get(job.url); - assert(mod, `Imported CJS module ${url} failed to load module ${job.url} using require()`); - if (!job.module.synthetic && !mod.loaded) { + let mod = cjsCache.get(job.url); + assert(job.module, `Imported CJS module ${url} failed to load module ${job.url} using require() due to race condition`); + + if (job.module.synthetic) { + assert(mod, `Imported CJS module ${url} failed to load module ${job.url} using require() due to missed cache`); + return mod.exports; + } + + // The module being required is a source text module. + if (!mod) { + mod = cjsEmplaceModuleCacheEntry(job.url); + cjsCache.set(job.url, mod); + } + // The module has been cached by the re-invented require. Update the exports object + // from the namespace object and return the evaluated exports. + if (!mod.loaded) { debug('populateCJSExportsFromESM from require(esm) in imported CJS', url, mod, job.module); populateCJSExportsFromESM(mod, job.module, job.module.getNamespace()); mod.loaded = true; diff --git a/test/es-module/test-esm-in-require-cache-2.mjs b/test/es-module/test-esm-in-require-cache-2.mjs new file mode 100644 index 00000000000000..b751883029b934 --- /dev/null +++ b/test/es-module/test-esm-in-require-cache-2.mjs @@ -0,0 +1,26 @@ +// This tests the behavior of ESM in require.cache when it's loaded from import. + +import '../common/index.mjs'; +import assert from 'node:assert'; +import * as fixtures from '../common/fixtures.mjs'; +const filename = fixtures.path('es-modules', 'esm-in-require-cache', 'esm.mjs'); +import { Module } from 'node:module'; + +// Imported ESM should not be in the require cache. +let { name } = await import('../fixtures/es-modules/esm-in-require-cache/import-esm.mjs'); +assert.strictEqual(name, 'esm'); +assert(!Module._cache[filename]); + +({ name } = await import('../fixtures/es-modules/esm-in-require-cache/esm.mjs')); +assert.strictEqual(name, 'esm'); +assert(!Module._cache[filename]); + +// Requiring ESM indirectly should not put it in the cache. +({ name } = await import('../fixtures/es-modules/esm-in-require-cache/require-import-esm.cjs')); +assert.strictEqual(name, 'esm'); +assert(!Module._cache[filename]); + +// After being required directly, it should be in the cache. +({ name } = await import('../fixtures/es-modules/esm-in-require-cache/import-require-esm.mjs')); +assert.strictEqual(name, 'esm'); +assert(Module._cache[filename]); diff --git a/test/es-module/test-esm-in-require-cache.js b/test/es-module/test-esm-in-require-cache.js new file mode 100644 index 00000000000000..bcdf0a896b5f97 --- /dev/null +++ b/test/es-module/test-esm-in-require-cache.js @@ -0,0 +1,25 @@ +// This tests the behavior of ESM in require.cache when it's loaded from require. + +require('../common'); +const assert = require('node:assert'); +const fixtures = require('../common/fixtures'); +const filename = fixtures.path('es-modules', 'esm-in-require-cache', 'esm.mjs'); + +// Requiring ESM indirectly should not put it in the cache. +let { name } = require('../fixtures/es-modules/esm-in-require-cache/import-esm.mjs'); +assert.strictEqual(name, 'esm'); +assert(!require.cache[filename]); + +({ name } = require('../fixtures/es-modules/esm-in-require-cache/require-import-esm.cjs')); +assert.strictEqual(name, 'esm'); +assert(!require.cache[filename]); + +// After being required directly, it should be in the cache. +({ name } = require('../fixtures/es-modules/esm-in-require-cache/esm.mjs')); +assert.strictEqual(name, 'esm'); +assert(require.cache[filename]); +delete require.cache[filename]; + +({ name } = require('../fixtures/es-modules/esm-in-require-cache/import-require-esm.mjs')); +assert.strictEqual(name, 'esm'); +assert(require.cache[filename]); diff --git a/test/fixtures/es-modules/esm-in-require-cache/esm.mjs b/test/fixtures/es-modules/esm-in-require-cache/esm.mjs new file mode 100644 index 00000000000000..787bbe86d3e771 --- /dev/null +++ b/test/fixtures/es-modules/esm-in-require-cache/esm.mjs @@ -0,0 +1 @@ +export const name = 'esm'; diff --git a/test/fixtures/es-modules/esm-in-require-cache/import-esm.mjs b/test/fixtures/es-modules/esm-in-require-cache/import-esm.mjs new file mode 100644 index 00000000000000..b70b27fe1620b9 --- /dev/null +++ b/test/fixtures/es-modules/esm-in-require-cache/import-esm.mjs @@ -0,0 +1 @@ +export { name } from './esm.mjs'; diff --git a/test/fixtures/es-modules/esm-in-require-cache/import-require-esm.mjs b/test/fixtures/es-modules/esm-in-require-cache/import-require-esm.mjs new file mode 100644 index 00000000000000..1d8e14adb2f5ed --- /dev/null +++ b/test/fixtures/es-modules/esm-in-require-cache/import-require-esm.mjs @@ -0,0 +1 @@ +export { name, cache } from './require-esm.cjs' diff --git a/test/fixtures/es-modules/esm-in-require-cache/require-esm.cjs b/test/fixtures/es-modules/esm-in-require-cache/require-esm.cjs new file mode 100644 index 00000000000000..52ead05a698255 --- /dev/null +++ b/test/fixtures/es-modules/esm-in-require-cache/require-esm.cjs @@ -0,0 +1,9 @@ +const path = require('path'); + +const name = require('./esm.mjs').name; +exports.name = name; + +const filename = path.join(__dirname, 'esm.mjs'); +const cache = require.cache[filename]; +exports.cache = require.cache[filename]; + diff --git a/test/fixtures/es-modules/esm-in-require-cache/require-import-esm.cjs b/test/fixtures/es-modules/esm-in-require-cache/require-import-esm.cjs new file mode 100644 index 00000000000000..ddf16383f7a4df --- /dev/null +++ b/test/fixtures/es-modules/esm-in-require-cache/require-import-esm.cjs @@ -0,0 +1 @@ +exports.name = require('./import-esm.mjs').name; From 787d53abba6acd1afd6ad2305ae1da1c0e95d515 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 13 Sep 2025 11:55:25 +0200 Subject: [PATCH 2/2] fixup! module: only put directly require-d ESM into require.cache --- lib/internal/modules/esm/loader.js | 1 - test/es-module/test-esm-in-require-cache.js | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index abd8eaf02c05cc..02ec3183bc596b 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -17,7 +17,6 @@ const { const { kIsExecuting, kRequiredModuleSymbol, - Module: CJSModule, } = require('internal/modules/cjs/loader'); const { imported_cjs_symbol } = internalBinding('symbols'); diff --git a/test/es-module/test-esm-in-require-cache.js b/test/es-module/test-esm-in-require-cache.js index bcdf0a896b5f97..d3365ce14f80b6 100644 --- a/test/es-module/test-esm-in-require-cache.js +++ b/test/es-module/test-esm-in-require-cache.js @@ -1,5 +1,5 @@ // This tests the behavior of ESM in require.cache when it's loaded from require. - +'use strict'; require('../common'); const assert = require('node:assert'); const fixtures = require('../common/fixtures');