Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 30ed93d

Browse filesBrowse files
joyeecheungmarco-ippolito
joyeecheung
authored and
marco-ippolito
committed
module: cache synchronous module jobs before linking
So that if there are circular dependencies in the synchronous module graph, they could be resolved using the cached jobs. In case linking fails and the error gets caught, reset the cache right after linking. If it succeeds, the caller will cache it again. Otherwise the error bubbles up to users, and since we unset the cache for the unlinkable module the next attempt would still fail. PR-URL: #52868 Backport-PR-URL: #56927 Fixes: #52864 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Refs: #52697
1 parent a03faf2 commit 30ed93d
Copy full SHA for 30ed93d

File tree

Expand file treeCollapse file tree

6 files changed

+43
-14
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

6 files changed

+43
-14
lines changed
Open diff view settings
Collapse file

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

Copy file name to clipboardExpand all lines: lib/internal/modules/esm/module_job.js
+25-14Lines changed: 25 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -287,22 +287,33 @@ class ModuleJobSync extends ModuleJobBase {
287287
#loader = null;
288288
constructor(loader, url, importAttributes, moduleWrap, isMain, inspectBrk) {
289289
super(url, importAttributes, moduleWrap, isMain, inspectBrk, true);
290-
assert(this.module instanceof ModuleWrap);
291290
this.#loader = loader;
292-
const moduleRequests = this.module.getModuleRequests();
293-
// Specifiers should be aligned with the moduleRequests array in order.
294-
const specifiers = Array(moduleRequests.length);
295-
const modules = Array(moduleRequests.length);
296-
const jobs = Array(moduleRequests.length);
297-
for (let i = 0; i < moduleRequests.length; ++i) {
298-
const { specifier, attributes } = moduleRequests[i];
299-
const job = this.#loader.getModuleJobForRequire(specifier, url, attributes);
300-
specifiers[i] = specifier;
301-
modules[i] = job.module;
302-
jobs[i] = job;
291+
292+
assert(this.module instanceof ModuleWrap);
293+
// Store itself into the cache first before linking in case there are circular
294+
// references in the linking.
295+
loader.loadCache.set(url, importAttributes.type, this);
296+
297+
try {
298+
const moduleRequests = this.module.getModuleRequests();
299+
// Specifiers should be aligned with the moduleRequests array in order.
300+
const specifiers = Array(moduleRequests.length);
301+
const modules = Array(moduleRequests.length);
302+
const jobs = Array(moduleRequests.length);
303+
for (let i = 0; i < moduleRequests.length; ++i) {
304+
const { specifier, attributes } = moduleRequests[i];
305+
const job = this.#loader.getModuleJobForRequire(specifier, url, attributes);
306+
specifiers[i] = specifier;
307+
modules[i] = job.module;
308+
jobs[i] = job;
309+
}
310+
this.module.link(specifiers, modules);
311+
this.linked = jobs;
312+
} finally {
313+
// Restore it - if it succeeds, we'll reset in the caller; Otherwise it's
314+
// not cached and if the error is caught, subsequent attempt would still fail.
315+
loader.loadCache.delete(url, importAttributes.type);
303316
}
304-
this.module.link(specifiers, modules);
305-
this.linked = jobs;
306317
}
307318

308319
get modulePromise() {
Collapse file

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

Copy file name to clipboardExpand all lines: lib/internal/modules/esm/module_map.js
+6Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,12 @@ class LoadCache extends SafeMap {
114114
validateString(type, 'type');
115115
return super.get(url)?.[type] !== undefined;
116116
}
117+
delete(url, type = kImplicitAssertType) {
118+
const cached = super.get(url);
119+
if (cached) {
120+
cached[type] = undefined;
121+
}
122+
}
117123
}
118124

119125
module.exports = {
Collapse file

‎test/es-module/test-require-module-cycle-cjs-esm-esm.js‎

Copy file name to clipboard
+8Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
// Flags: --experimental-require-module
2+
'use strict';
3+
4+
// This tests that ESM <-> ESM cycle is allowed in a require()-d graph.
5+
const common = require('../common');
6+
const cycle = require('../fixtures/es-modules/cjs-esm-esm-cycle/c.cjs');
7+
8+
common.expectRequiredModule(cycle, { b: 5 });
Collapse file

‎test/fixtures/es-modules/cjs-esm-esm-cycle/a.mjs‎

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

‎test/fixtures/es-modules/cjs-esm-esm-cycle/b.mjs‎

Copy file name to clipboard
+2Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
import './a.mjs'
2+
export const b = 5;
Collapse file

‎test/fixtures/es-modules/cjs-esm-esm-cycle/c.cjs‎

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

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.