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 8d33f78

Browse filesBrowse files
joyeecheungmarco-ippolito
joyeecheung
authored and
marco-ippolito
committed
module: allow ESM that failed to be required to be re-imported
When a ESM module cannot be loaded by require due to the presence of TLA, its module status would be stopped at kInstantiated. In this case, when it's imported again, we should allow it to be evaluated asynchronously, as it's also a common pattern for users to retry with dynamic import when require fails. PR-URL: #55502 Backport-PR-URL: #56927 Fixes: #55500 Refs: #52697 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
1 parent 8192dd6 commit 8d33f78
Copy full SHA for 8d33f78

File tree

Expand file treeCollapse file tree

8 files changed

+152
-4
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

8 files changed

+152
-4
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
+20-4Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ let debug = require('internal/util/debuglog').debuglog('esm', (fn) => {
2121
debug = fn;
2222
});
2323

24-
const { ModuleWrap, kEvaluated } = internalBinding('module_wrap');
24+
const { ModuleWrap, kInstantiated } = internalBinding('module_wrap');
2525

2626
const { decorateErrorStack, kEmptyObject } = require('internal/util');
2727
const {
@@ -346,10 +346,26 @@ class ModuleJobSync extends ModuleJobBase {
346346
}
347347

348348
async run() {
349+
// This path is hit by a require'd module that is imported again.
349350
const status = this.module.getStatus();
350-
assert(status === kEvaluated,
351-
`A require()-d module that is imported again must be evaluated. Status = ${status}`);
352-
return { __proto__: null, module: this.module };
351+
if (status > kInstantiated) {
352+
if (this.evaluationPromise) {
353+
await this.evaluationPromise;
354+
}
355+
return { __proto__: null, module: this.module };
356+
} else if (status === kInstantiated) {
357+
// The evaluation may have been canceled because instantiateSync() detected TLA first.
358+
// But when it is imported again, it's fine to re-evaluate it asynchronously.
359+
const timeout = -1;
360+
const breakOnSigint = false;
361+
this.evaluationPromise = this.module.evaluate(timeout, breakOnSigint);
362+
await this.evaluationPromise;
363+
this.evaluationPromise = undefined;
364+
return { __proto__: null, module: this.module };
365+
}
366+
367+
assert.fail('Unexpected status of a module that is imported again after being required. ' +
368+
`Status = ${status}`);
353369
}
354370

355371
runSync() {
Collapse file

‎test/es-module/test-require-module-retry-import-errored.js‎

Copy file name to clipboard
+35Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
// This tests that after failing to require an ESM that contains TLA,
2+
// retrying with import() still works, and produces consistent results.
3+
'use strict';
4+
const common = require('../common');
5+
const assert = require('assert');
6+
7+
const { exportedReject } = require('../fixtures/es-modules/tla/export-promise.mjs');
8+
9+
assert.throws(() => {
10+
require('../fixtures/es-modules/tla/await-export-promise.mjs');
11+
}, {
12+
code: 'ERR_REQUIRE_ASYNC_MODULE'
13+
});
14+
15+
const interval = setInterval(() => {}, 1000); // Keep the test running, because await alone doesn't.
16+
const err = new Error('rejected');
17+
18+
const p1 = import('../fixtures/es-modules/tla/await-export-promise.mjs')
19+
.then(common.mustNotCall(), common.mustCall((e) => {
20+
assert.strictEqual(e, err);
21+
}));
22+
23+
const p2 = import('../fixtures/es-modules/tla/await-export-promise.mjs')
24+
.then(common.mustNotCall(), common.mustCall((e) => {
25+
assert.strictEqual(e, err);
26+
}));
27+
28+
const p3 = import('../fixtures/es-modules/tla/await-export-promise.mjs')
29+
.then(common.mustNotCall(), common.mustCall((e) => {
30+
assert.strictEqual(e, err);
31+
}));
32+
33+
exportedReject(err);
34+
35+
Promise.all([p1, p2, p3]).then(common.mustCall(() => { clearInterval(interval); }));
Collapse file

‎test/es-module/test-require-module-retry-import-evaluating.js‎

Copy file name to clipboard
+32Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
// This tests that after failing to require an ESM that contains TLA,
2+
// retrying with import() still works, and produces consistent results.
3+
'use strict';
4+
const common = require('../common');
5+
const assert = require('assert');
6+
7+
const { exportedResolve } = require('../fixtures/es-modules/tla/export-promise.mjs');
8+
9+
assert.throws(() => {
10+
require('../fixtures/es-modules/tla/await-export-promise.mjs');
11+
}, {
12+
code: 'ERR_REQUIRE_ASYNC_MODULE'
13+
});
14+
15+
const interval = setInterval(() => {}, 1000); // Keep the test running, because await alone doesn't.
16+
const value = { hello: 'world' };
17+
18+
const p1 = import('../fixtures/es-modules/tla/await-export-promise.mjs').then(common.mustCall((ns) => {
19+
assert.strictEqual(ns.default, value);
20+
}), common.mustNotCall());
21+
22+
const p2 = import('../fixtures/es-modules/tla/await-export-promise.mjs').then(common.mustCall((ns) => {
23+
assert.strictEqual(ns.default, value);
24+
}), common.mustNotCall());
25+
26+
const p3 = import('../fixtures/es-modules/tla/await-export-promise.mjs').then(common.mustCall((ns) => {
27+
assert.strictEqual(ns.default, value);
28+
}), common.mustNotCall());
29+
30+
exportedResolve(value);
31+
32+
Promise.all([p1, p2, p3]).then(common.mustCall(() => { clearInterval(interval); }));
Collapse file

‎test/es-module/test-require-module-tla-retry-import-2.js‎

Copy file name to clipboard
+26Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
// This tests that after loading a ESM with import() and then retrying
2+
// with require(), it errors as expected, and produces consistent results.
3+
'use strict';
4+
const common = require('../common');
5+
const assert = require('assert');
6+
7+
let ns;
8+
async function test() {
9+
const newNs = await import('../fixtures/es-modules/tla/export-async.mjs');
10+
if (ns === undefined) {
11+
ns = newNs;
12+
} else {
13+
// Check that re-evalaution is returning the same namespace.
14+
assert.strictEqual(ns, newNs);
15+
}
16+
assert.strictEqual(ns.hello, 'world');
17+
18+
assert.throws(() => {
19+
require('../fixtures/es-modules/tla/export-async.mjs');
20+
}, {
21+
code: 'ERR_REQUIRE_ASYNC_MODULE'
22+
});
23+
}
24+
25+
// Run the test twice to check consistency after caching.
26+
test().then(common.mustCall(test)).catch(common.mustNotCall());
Collapse file

‎test/es-module/test-require-module-tla-retry-import.js‎

Copy file name to clipboard
+25Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
// This tests that after failing to require an ESM that contains TLA,
2+
// retrying with import() still works, and produces consistent results.
3+
'use strict';
4+
const common = require('../common');
5+
const assert = require('assert');
6+
7+
let ns;
8+
async function test() {
9+
assert.throws(() => {
10+
require('../fixtures/es-modules/tla/export-async.mjs');
11+
}, {
12+
code: 'ERR_REQUIRE_ASYNC_MODULE'
13+
});
14+
const newNs = await import('../fixtures/es-modules/tla/export-async.mjs');
15+
if (ns === undefined) {
16+
ns = newNs;
17+
} else {
18+
// Check that re-evalaution is returning the same namespace.
19+
assert.strictEqual(ns, newNs);
20+
}
21+
assert.strictEqual(ns.hello, 'world');
22+
}
23+
24+
// Run the test twice to check consistency after caching.
25+
test().then(common.mustCall(test)).catch(common.mustNotCall());
Collapse file

‎test/fixtures/es-modules/tla/await-export-promise.mjs‎

Copy file name to clipboard
+4Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
import promise from './export-promise.mjs';
2+
let result;
3+
result = await promise;
4+
export default result;
Collapse file

‎test/fixtures/es-modules/tla/export-async.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+
let hello = await Promise.resolve('world');
2+
export { hello };
Collapse file

‎test/fixtures/es-modules/tla/export-promise.mjs‎

Copy file name to clipboard
+8Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
let exportedResolve;
2+
let exportedReject;
3+
const promise = new Promise((resolve, reject) => {
4+
exportedResolve = resolve;
5+
exportedReject = reject;
6+
});
7+
export default promise;
8+
export { exportedResolve, exportedReject };

0 commit comments

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