FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

module: only put directly require-d ESM into require.cache · nodejs/node@728472a · GitHub

/ node Public

Commit 728472a

Browse files
joyeecheung
authored and
targos
committed
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. PR-URL: #59874 Refs: #59868 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
1 parent 993f05d commit 728472a

9 files changed

Lines changed: 80 additions & 17 deletions

File tree

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

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ const {
1717
const {
1818
kIsExecuting,
1919
kRequiredModuleSymbol,
20-
Module: CJSModule,
2120
} = require('internal/modules/cjs/loader');
2221
const { imported_cjs_symbol } = internalBinding('symbols');
2322

@@ -560,19 +559,6 @@ class ModuleLoader {
560559
throw new ERR_UNKNOWN_MODULE_FORMAT(format, url);
561560
}
562561

563-
// Populate the CJS cache with a facade for ESM in case subsequent require(esm) is
564-
// looking it up from the cache. The parent module of the CJS cache entry would be the
565-
// first CJS module that loads it with require(). This is an approximation, because
566-
// ESM caches more and it does not get re-loaded and updated every time an `import` is
567-
// encountered, unlike CJS require(), and we only use the parent entry to provide
568-
// more information in error messages.
569-
if (format === 'module') {
570-
const parentFilename = urlToFilename(parentURL);
571-
const parent = parentFilename ? CJSModule._cache[parentFilename] : undefined;
572-
const cjsModule = lazyLoadTranslators().cjsEmplaceModuleCacheEntryForURL(url, parent);
573-
debug('cjsEmplaceModuleCacheEntryForURL', url, parent, cjsModule);
574-
}
575-
576562
const result = FunctionPrototypeCall(translator, this, url, source, parentURL === undefined);
577563
assert(result instanceof ModuleWrap);
578564
return result;

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

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -154,9 +154,22 @@ function loadCJSModule(module, source, url, filename, isMain) {
154154
// requires a separate cache to be populated as well as introducing several quirks. This is not ideal.
155155
const job = cascadedLoader.getModuleJobForRequireInImportedCJS(specifier, url, importAttributes);
156156
job.runSync();
157-
const mod = cjsCache.get(job.url);
158-
assert(mod, `Imported CJS module ${url} failed to load module ${job.url} using require()`);
159-
if (!job.module.synthetic && !mod.loaded) {
157+
let mod = cjsCache.get(job.url);
158+
assert(job.module, `Imported CJS module ${url} failed to load module ${job.url} using require() due to race condition`);
159+
160+
if (job.module.synthetic) {
161+
assert(mod, `Imported CJS module ${url} failed to load module ${job.url} using require() due to missed cache`);
162+
return mod.exports;
163+
}
164+
165+
// The module being required is a source text module.
166+
if (!mod) {
167+
mod = cjsEmplaceModuleCacheEntry(job.url);
168+
cjsCache.set(job.url, mod);
169+
}
170+
// The module has been cached by the re-invented require. Update the exports object
171+
// from the namespace object and return the evaluated exports.
172+
if (!mod.loaded) {
160173
debug('populateCJSExportsFromESM from require(esm) in imported CJS', url, mod, job.module);
161174
populateCJSExportsFromESM(mod, job.module, job.module.getNamespace());
162175
mod.loaded = true;

‎test/es-module/test-esm-in-require-cache-2.mjs‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
// This tests the behavior of ESM in require.cache when it's loaded from import.
2+
3+
import '../common/index.mjs';
4+
import assert from 'node:assert';
5+
import * as fixtures from '../common/fixtures.mjs';
6+
const filename = fixtures.path('es-modules', 'esm-in-require-cache', 'esm.mjs');
7+
import { Module } from 'node:module';
8+
9+
// Imported ESM should not be in the require cache.
10+
let { name } = await import('../fixtures/es-modules/esm-in-require-cache/import-esm.mjs');
11+
assert.strictEqual(name, 'esm');
12+
assert(!Module._cache[filename]);
13+
14+
({ name } = await import('../fixtures/es-modules/esm-in-require-cache/esm.mjs'));
15+
assert.strictEqual(name, 'esm');
16+
assert(!Module._cache[filename]);
17+
18+
// Requiring ESM indirectly should not put it in the cache.
19+
({ name } = await import('../fixtures/es-modules/esm-in-require-cache/require-import-esm.cjs'));
20+
assert.strictEqual(name, 'esm');
21+
assert(!Module._cache[filename]);
22+
23+
// After being required directly, it should be in the cache.
24+
({ name } = await import('../fixtures/es-modules/esm-in-require-cache/import-require-esm.mjs'));
25+
assert.strictEqual(name, 'esm');
26+
assert(Module._cache[filename]);

‎test/es-module/test-esm-in-require-cache.js‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
// This tests the behavior of ESM in require.cache when it's loaded from require.
2+
'use strict';
3+
require('../common');
4+
const assert = require('node:assert');
5+
const fixtures = require('../common/fixtures');
6+
const filename = fixtures.path('es-modules', 'esm-in-require-cache', 'esm.mjs');
7+
8+
// Requiring ESM indirectly should not put it in the cache.
9+
let { name } = require('../fixtures/es-modules/esm-in-require-cache/import-esm.mjs');
10+
assert.strictEqual(name, 'esm');
11+
assert(!require.cache[filename]);
12+
13+
({ name } = require('../fixtures/es-modules/esm-in-require-cache/require-import-esm.cjs'));
14+
assert.strictEqual(name, 'esm');
15+
assert(!require.cache[filename]);
16+
17+
// After being required directly, it should be in the cache.
18+
({ name } = require('../fixtures/es-modules/esm-in-require-cache/esm.mjs'));
19+
assert.strictEqual(name, 'esm');
20+
assert(require.cache[filename]);
21+
delete require.cache[filename];
22+
23+
({ name } = require('../fixtures/es-modules/esm-in-require-cache/import-require-esm.mjs'));
24+
assert.strictEqual(name, 'esm');
25+
assert(require.cache[filename]);

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
export const name = 'esm';

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
export { name } from './esm.mjs';

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
export { name, cache } from './require-esm.cjs'

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
const path = require('path');
2+
3+
const name = require('./esm.mjs').name;
4+
exports.name = name;
5+
6+
const filename = path.join(__dirname, 'esm.mjs');
7+
const cache = require.cache[filename];
8+
exports.cache = require.cache[filename];
9+

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
exports.name = require('./import-esm.mjs').name;

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL