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

module: allow cycles in require() in the CJS handling in ESM loader · nodejs/node@a3c7a63 · GitHub

/ node Public

Commit a3c7a63

Browse files
joyeecheung
authored and
targos
committed
module: allow cycles in require() in the CJS handling in ESM loader
When --import is used, the ESM loader is used to handle even pure CJS entry points, and it can run into CJS module facades in the evaluating state when the parent CJS module is being evaluated. In this case it should be allowed, since the ESM <-> CJS cycles that are meant to be disallowed (for the time being) should already be detected before evaluation and wouldn't get here, and CJS <-> CJS cycles are fine. PR-URL: #58598 Fixes: #58515 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
1 parent aa657f0 commit a3c7a63

7 files changed

Lines changed: 42 additions & 4 deletions

File tree

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

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ const {
2626
ModuleWrap,
2727
kErrored,
2828
kEvaluated,
29+
kEvaluating,
2930
kEvaluationPhase,
3031
kInstantiated,
3132
kUninstantiated,
@@ -338,8 +339,14 @@ class ModuleJob extends ModuleJobBase {
338339
return { __proto__: null, module: this.module, namespace };
339340
}
340341
throw this.module.getError();
341-
342-
} else if (status === kEvaluated) {
342+
} else if (status === kEvaluating || status === kEvaluated) {
343+
// kEvaluating can show up when this is being used to deal with CJS <-> CJS cycles.
344+
// Allow it for now, since we only need to ban ESM <-> CJS cycles which would be
345+
// detected earlier during the linking phase, though the CJS handling in the ESM
346+
// loader won't be able to emit warnings on pending circular exports like what
347+
// the CJS loader does.
348+
// TODO(joyeecheung): remove the re-invented require() in the ESM loader and
349+
// always handle CJS using the CJS loader to eliminate the quirks.
343350
return { __proto__: null, module: this.module, namespace: this.module.getNamespaceSync() };
344351
}
345352
assert.fail(`Unexpected module status ${status}.`);

‎src/module_wrap.cc‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -783,11 +783,10 @@ void ModuleWrap::GetNamespaceSync(const FunctionCallbackInfo<Value>& args) {
783783
return realm->env()->ThrowError(
784784
"Cannot get namespace, module has not been instantiated");
785785
case Module::Status::kInstantiated:
786+
case Module::Status::kEvaluating:
786787
case Module::Status::kEvaluated:
787788
case Module::Status::kErrored:
788789
break;
789-
case Module::Status::kEvaluating:
790-
UNREACHABLE();
791790
}
792791

793792
if (module->IsGraphAsync()) {

‎test/es-module/test-import-preload-require-cycle.js‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
'use strict';
2+
3+
// This tests that --import preload does not break CJS entry points that contains
4+
// require cycles.
5+
6+
require('../common');
7+
const fixtures = require('../common/fixtures');
8+
const { spawnSyncAndAssert } = require('../common/child_process');
9+
10+
spawnSyncAndAssert(
11+
process.execPath,
12+
[
13+
'--import',
14+
fixtures.fileURL('import-require-cycle/preload.mjs'),
15+
fixtures.path('import-require-cycle/c.js'),
16+
],
17+
{
18+
stdout: /cycle equality true/,
19+
}
20+
);

‎test/fixtures/import-require-cycle/a.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
module.exports.b = require('./b.js');

‎test/fixtures/import-require-cycle/b.js‎

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

‎test/fixtures/import-require-cycle/c.js‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
const obj = require('./b.js');
2+
3+
console.log('cycle equality', obj.a.b === obj);

‎test/fixtures/import-require-cycle/preload.mjs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
import * as mod from "module";
2+
3+
mod.registerHooks({
4+
load(url, context, nextLoad) {
5+
return nextLoad(url, context);
6+
},
7+
});

0 commit comments

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