From 6ea1c51794500d6b4dfc735043538b12847f7950 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 7 May 2024 08:11:54 +0800 Subject: [PATCH 1/3] test: add common.expectRequiredModule() To minimize changes if/when we change the layout of the result returned by require(esm). --- test/common/index.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/common/index.js b/test/common/index.js index d2c68c0fafb01b..84805723e4be6a 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -32,6 +32,7 @@ const net = require('net'); const path = require('path'); const { inspect } = require('util'); const { isMainThread } = require('worker_threads'); +const { isModuleNamespaceObject } = require('util/types'); const tmpdir = require('./tmpdir'); const bits = ['arm64', 'loong64', 'mips', 'mipsel', 'ppc64', 'riscv64', 's390x', 'x64'] @@ -938,6 +939,18 @@ function getPrintedStackTrace(stderr) { return result; } +/** + * Check the exports of require(esm). + * TODO(joyeecheung): use it in all the test-require-module-* tests to minimize changes + * if/when we change the layout of the result returned by require(esm). + * @param {object} mod result returned by require() + * @param {object} expectation shape of expected namespace. + */ +function expectRequiredModule(mod, expectation) { + assert(isModuleNamespaceObject(mod)); + assert.deepStrictEqual({ ...mod }, { ...expectation }); +} + const common = { allowGlobals, buildType, @@ -946,6 +959,7 @@ const common = { createZeroFilledFile, defaultAutoSelectFamilyAttemptTimeout, expectsError, + expectRequiredModule, expectWarning, gcUntil, getArrayBufferViews, From 0f4b1200cbc789e8e5081b5d9c62056740afa6e7 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 7 May 2024 08:12:16 +0800 Subject: [PATCH 2/3] 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. --- lib/internal/modules/esm/module_job.js | 39 ++++++++++++------- lib/internal/modules/esm/module_map.js | 6 +++ .../test-require-module-cycle-cjs-esm-esm.js | 8 ++++ .../es-modules/cjs-esm-esm-cycle/a.mjs | 2 + .../es-modules/cjs-esm-esm-cycle/b.mjs | 2 + .../es-modules/cjs-esm-esm-cycle/c.cjs | 1 + 6 files changed, 44 insertions(+), 14 deletions(-) create mode 100644 test/es-module/test-require-module-cycle-cjs-esm-esm.js create mode 100644 test/fixtures/es-modules/cjs-esm-esm-cycle/a.mjs create mode 100644 test/fixtures/es-modules/cjs-esm-esm-cycle/b.mjs create mode 100644 test/fixtures/es-modules/cjs-esm-esm-cycle/c.cjs diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index d7c6927049c42d..2f42909e0c6f82 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -295,22 +295,33 @@ class ModuleJobSync extends ModuleJobBase { #loader = null; constructor(loader, url, importAttributes, moduleWrap, isMain, inspectBrk) { super(url, importAttributes, moduleWrap, isMain, inspectBrk, true); - assert(this.module instanceof ModuleWrap); this.#loader = loader; - const moduleRequests = this.module.getModuleRequests(); - // Specifiers should be aligned with the moduleRequests array in order. - const specifiers = Array(moduleRequests.length); - const modules = Array(moduleRequests.length); - const jobs = Array(moduleRequests.length); - for (let i = 0; i < moduleRequests.length; ++i) { - const { specifier, attributes } = moduleRequests[i]; - const job = this.#loader.getModuleJobForRequire(specifier, url, attributes); - specifiers[i] = specifier; - modules[i] = job.module; - jobs[i] = job; + + assert(this.module instanceof ModuleWrap); + // Store itself into the cache first before linking in case there are circular + // references in the linking. + loader.loadCache.set(url, importAttributes.type, this); + + try { + const moduleRequests = this.module.getModuleRequests(); + // Specifiers should be aligned with the moduleRequests array in order. + const specifiers = Array(moduleRequests.length); + const modules = Array(moduleRequests.length); + const jobs = Array(moduleRequests.length); + for (let i = 0; i < moduleRequests.length; ++i) { + const { specifier, attributes } = moduleRequests[i]; + const job = this.#loader.getModuleJobForRequire(specifier, url, attributes); + specifiers[i] = specifier; + modules[i] = job.module; + jobs[i] = job; + } + this.module.link(specifiers, modules); + this.linked = jobs; + } finally { + // Restore it - if it succeeds, we'll reset in the caller; Otherwise it's + // not cached and if the error is caught, subsequent attempt would still fail. + loader.loadCache.delete(url, importAttributes.type); } - this.module.link(specifiers, modules); - this.linked = jobs; } get modulePromise() { diff --git a/lib/internal/modules/esm/module_map.js b/lib/internal/modules/esm/module_map.js index ab1171eaa47b02..247bde93cabd70 100644 --- a/lib/internal/modules/esm/module_map.js +++ b/lib/internal/modules/esm/module_map.js @@ -114,6 +114,12 @@ class LoadCache extends SafeMap { validateString(type, 'type'); return super.get(url)?.[type] !== undefined; } + delete(url, type = kImplicitAssertType) { + const cached = super.get(url); + if (cached) { + cached[type] = undefined; + } + } } module.exports = { diff --git a/test/es-module/test-require-module-cycle-cjs-esm-esm.js b/test/es-module/test-require-module-cycle-cjs-esm-esm.js new file mode 100644 index 00000000000000..a83d7ee7a71bb2 --- /dev/null +++ b/test/es-module/test-require-module-cycle-cjs-esm-esm.js @@ -0,0 +1,8 @@ +// Flags: --experimental-require-module +'use strict'; + +// This tests that ESM <-> ESM cycle is allowed in a require()-d graph. +const common = require('../common'); +const cycle = require('../fixtures/es-modules/cjs-esm-esm-cycle/c.cjs'); + +common.expectRequiredModule(cycle, { b: 5 }); diff --git a/test/fixtures/es-modules/cjs-esm-esm-cycle/a.mjs b/test/fixtures/es-modules/cjs-esm-esm-cycle/a.mjs new file mode 100644 index 00000000000000..4de3a7649de07b --- /dev/null +++ b/test/fixtures/es-modules/cjs-esm-esm-cycle/a.mjs @@ -0,0 +1,2 @@ +export { b } from './b.mjs'; + diff --git a/test/fixtures/es-modules/cjs-esm-esm-cycle/b.mjs b/test/fixtures/es-modules/cjs-esm-esm-cycle/b.mjs new file mode 100644 index 00000000000000..9e909cd6856455 --- /dev/null +++ b/test/fixtures/es-modules/cjs-esm-esm-cycle/b.mjs @@ -0,0 +1,2 @@ +import './a.mjs' +export const b = 5; diff --git a/test/fixtures/es-modules/cjs-esm-esm-cycle/c.cjs b/test/fixtures/es-modules/cjs-esm-esm-cycle/c.cjs new file mode 100644 index 00000000000000..f9361ecd59d11e --- /dev/null +++ b/test/fixtures/es-modules/cjs-esm-esm-cycle/c.cjs @@ -0,0 +1 @@ +module.exports = require('./a.mjs'); From c034992beec135c5a61ab29934596ef46bdf8e6f Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 8 May 2024 05:33:18 +0800 Subject: [PATCH 3/3] fixup! module: cache synchronous module jobs before linking Co-authored-by: Yagiz Nizipli --- test/fixtures/es-modules/cjs-esm-esm-cycle/a.mjs | 1 - 1 file changed, 1 deletion(-) diff --git a/test/fixtures/es-modules/cjs-esm-esm-cycle/a.mjs b/test/fixtures/es-modules/cjs-esm-esm-cycle/a.mjs index 4de3a7649de07b..798e86506da2a9 100644 --- a/test/fixtures/es-modules/cjs-esm-esm-cycle/a.mjs +++ b/test/fixtures/es-modules/cjs-esm-esm-cycle/a.mjs @@ -1,2 +1 @@ export { b } from './b.mjs'; -