module: only put directly require-d ESM into require.cache

This reduces the impact of
https://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.

PR-URL: #59874
Refs: #59868
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>

Author: joyeecheung

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');
 
@@ -560,19 +559,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;

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;

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]);

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.
+'use strict';
+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]);

test/fixtures/es-modules/esm-in-require-cache/esm.mjs

@@ -0,0 +1 @@
+export const name = 'esm';

test/fixtures/es-modules/esm-in-require-cache/import-esm.mjs

@@ -0,0 +1 @@
+export { name } from './esm.mjs';

test/fixtures/es-modules/esm-in-require-cache/import-require-esm.mjs

@@ -0,0 +1 @@
+export { name, cache } from './require-esm.cjs'

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];
+

test/fixtures/es-modules/esm-in-require-cache/require-import-esm.cjs

@@ -0,0 +1 @@
+exports.name = require('./import-esm.mjs').name;