From 445f2e0bc1f279b15f11b3cd8bcc4cbd48e88b82 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 6 Jun 2025 12:08:57 +0200 Subject: [PATCH 1/2] 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. --- lib/internal/modules/esm/module_job.js | 11 ++++++++-- src/module_wrap.cc | 3 +-- .../test-import-preload-require-cycle.js | 20 +++++++++++++++++++ test/fixtures/import-require-cycle/a.js | 1 + test/fixtures/import-require-cycle/b.js | 1 + test/fixtures/import-require-cycle/c.js | 3 +++ .../fixtures/import-require-cycle/preload.mjs | 7 +++++++ 7 files changed, 42 insertions(+), 4 deletions(-) create mode 100644 test/es-module/test-import-preload-require-cycle.js create mode 100644 test/fixtures/import-require-cycle/a.js create mode 100644 test/fixtures/import-require-cycle/b.js create mode 100644 test/fixtures/import-require-cycle/c.js create mode 100644 test/fixtures/import-require-cycle/preload.mjs diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index 51aa863dd615f5..cac4f603229f33 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -26,6 +26,7 @@ const { ModuleWrap, kErrored, kEvaluated, + kEvaluating, kEvaluationPhase, kInstantiated, kUninstantiated, @@ -338,8 +339,14 @@ class ModuleJob extends ModuleJobBase { return { __proto__: null, module: this.module, namespace }; } throw this.module.getError(); - - } else if (status === kEvaluated) { + } else if (status === kEvaluating || status === kEvaluated) { + // kEvaluating can show up when this is being used to deal with CJS <-> CJS cycles. + // Allow it for now, since we only need to ban ESM <-> CJS cycles which would be + // detected earlier during the linking phase, though the CJS handling in the ESM + // loader won't be able to emit warnings on pending circular exports like what + // the CJS loader does. + // TODO(joyeecheung): remove the re-invented require() in the ESM loader and + // always handle CJS using the CJS loader to eliminate the quirks. return { __proto__: null, module: this.module, namespace: this.module.getNamespaceSync() }; } assert.fail(`Unexpected module status ${status}.`); diff --git a/src/module_wrap.cc b/src/module_wrap.cc index dd0f74c5580561..2b81a4d5da63ec 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -783,11 +783,10 @@ void ModuleWrap::GetNamespaceSync(const FunctionCallbackInfo& args) { return realm->env()->ThrowError( "Cannot get namespace, module has not been instantiated"); case Module::Status::kInstantiated: + case Module::Status::kEvaluating: case Module::Status::kEvaluated: case Module::Status::kErrored: break; - case Module::Status::kEvaluating: - UNREACHABLE(); } if (module->IsGraphAsync()) { diff --git a/test/es-module/test-import-preload-require-cycle.js b/test/es-module/test-import-preload-require-cycle.js new file mode 100644 index 00000000000000..657ddf4007a19c --- /dev/null +++ b/test/es-module/test-import-preload-require-cycle.js @@ -0,0 +1,20 @@ +'use strict'; + +// This tests that --import preload does not break CJS entry points that contains +// require cycles. + +require('../common'); +const fixtures = require('../common/fixtures'); +const { spawnSyncAndAssert } = require('../common/child_process'); + +spawnSyncAndAssert( + process.execPath, + [ + '--import', + fixtures.path('import-require-cycle/preload.mjs'), + fixtures.path('import-require-cycle/c.js'), + ], + { + stdout: /cycle equality true/, + } +); diff --git a/test/fixtures/import-require-cycle/a.js b/test/fixtures/import-require-cycle/a.js new file mode 100644 index 00000000000000..595a5085cf5ff9 --- /dev/null +++ b/test/fixtures/import-require-cycle/a.js @@ -0,0 +1 @@ +module.exports.b = require('./b.js'); diff --git a/test/fixtures/import-require-cycle/b.js b/test/fixtures/import-require-cycle/b.js new file mode 100644 index 00000000000000..869be257319c1c --- /dev/null +++ b/test/fixtures/import-require-cycle/b.js @@ -0,0 +1 @@ +module.exports.a = require('./a.js'); diff --git a/test/fixtures/import-require-cycle/c.js b/test/fixtures/import-require-cycle/c.js new file mode 100644 index 00000000000000..39099ad76074b3 --- /dev/null +++ b/test/fixtures/import-require-cycle/c.js @@ -0,0 +1,3 @@ +const obj = require('./b.js'); + +console.log('cycle equality', obj.a.b === obj); diff --git a/test/fixtures/import-require-cycle/preload.mjs b/test/fixtures/import-require-cycle/preload.mjs new file mode 100644 index 00000000000000..81eed70009e79e --- /dev/null +++ b/test/fixtures/import-require-cycle/preload.mjs @@ -0,0 +1,7 @@ +import * as mod from "module"; + +mod.registerHooks({ + load(url, context, nextLoad) { + return nextLoad(url, context); + }, +}); From 4d6f69300c04d6fb5e20ed78cb8259f336737f9f Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 9 Jun 2025 17:33:19 +0200 Subject: [PATCH 2/2] fixup! module: allow cycles in require() in the CJS handling in ESM loader --- test/es-module/test-import-preload-require-cycle.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/es-module/test-import-preload-require-cycle.js b/test/es-module/test-import-preload-require-cycle.js index 657ddf4007a19c..c47b64ba584802 100644 --- a/test/es-module/test-import-preload-require-cycle.js +++ b/test/es-module/test-import-preload-require-cycle.js @@ -11,7 +11,7 @@ spawnSyncAndAssert( process.execPath, [ '--import', - fixtures.path('import-require-cycle/preload.mjs'), + fixtures.fileURL('import-require-cycle/preload.mjs'), fixtures.path('import-require-cycle/c.js'), ], {