diff --git a/src/jsc/bindings/JSCommonJSModule.cpp b/src/jsc/bindings/JSCommonJSModule.cpp index f849adacd02c..51224ac9ed66 100644 --- a/src/jsc/bindings/JSCommonJSModule.cpp +++ b/src/jsc/bindings/JSCommonJSModule.cpp @@ -63,6 +63,8 @@ #include #include #include "ModuleLoader.h" +#include +#include #include #include @@ -267,6 +269,16 @@ bool JSCommonJSModule::load(JSC::VM& vm, Zig::GlobalObject* globalObject) bool wasRemoved = globalObject->requireMap()->remove(globalObject, this->filename()); ASSERT(wasRemoved); + // Keep the error reachable for a replayed makeModule of this module's + // ESM synthetic source, matching commonJSModuleSyntheticSourceCode's + // error path. Plain CJS require never registers with the module + // loader, so this is a no-op there. + auto filenameString = this->filename().toWTFString(globalObject); + if (scope.exception()) [[unlikely]] + (void)scope.tryClearException(); + else if (auto* registryEntry = globalObject->moduleLoader()->registryEntry(JSC::Identifier::fromString(vm, filenameString))) + registryEntry->setEvaluationError(globalObject, exception->value()); + scope.throwException(globalObject, exception); return false; } @@ -1582,35 +1594,54 @@ static JSC::SourceCode commonJSModuleSyntheticSourceCode(const SourceOrigin& sou JSValue entry = globalObject->requireMap()->get(globalObject, keyValue); RETURN_IF_EXCEPTION(scope, {}); - if (entry) { - if (auto* moduleObject = dynamicDowncast(entry)) { - if (!moduleObject->hasEvaluated) { - evaluateCommonJSModuleOnce( - vm, - globalObject, - moduleObject, - moduleObject->m_dirname.get(), - moduleObject->m_filename.get()); - if (auto exception = scope.exception()) { - if (vm.hasPendingTerminationException()) [[unlikely]] - return; - (void)scope.tryClearException(); - - // On error, remove the module from the require map - // so that it can be re-evaluated on the next require. - globalObject->requireMap()->remove(globalObject, moduleObject->filename()); - RETURN_IF_EXCEPTION(scope, {}); - - scope.throwException(globalObject, exception); - return; - } + // JSMap::get returns undefined, which is truthy as a JSValue, + // for a missing key. A vanished entry means an evaluation + // attempt failed and was evicted (below); an empty module here + // would silently drop the module's side effects and its error. + if (entry.isUndefinedOrNull()) [[unlikely]] { + if (auto* registryEntry = globalObject->moduleLoader()->registryEntry(moduleKey)) { + JSValue error = registryEntry->error(globalObject); + RETURN_IF_EXCEPTION(scope, {}); + if (error) { + scope.throwException(globalObject, error); + return; } + } + throwException(globalObject, scope, createError(globalObject, makeString("Module \""_s, StringView(moduleKey.string()), "\" was removed from the require cache while it was being loaded."_s))); + return; + } - moduleObject->toSyntheticSource(globalObject, moduleKey, exportNames, exportValues); - RETURN_IF_EXCEPTION(scope, {}); + if (auto* moduleObject = dynamicDowncast(entry)) { + if (!moduleObject->hasEvaluated) { + evaluateCommonJSModuleOnce( + vm, + globalObject, + moduleObject, + moduleObject->m_dirname.get(), + moduleObject->m_filename.get()); + if (auto exception = scope.exception()) { + if (vm.hasPendingTerminationException()) [[unlikely]] + return; + (void)scope.tryClearException(); + + // On error, remove the module from the require map + // so that it can be re-evaluated on the next require. + globalObject->requireMap()->remove(globalObject, moduleObject->filename()); + RETURN_IF_EXCEPTION(scope, {}); + + // Keep the error reachable for a replayed makeModule + // (above); this throw's rejection can strand on an + // outer synchronous module queue and get dropped. + if (auto* registryEntry = globalObject->moduleLoader()->registryEntry(moduleKey)) + registryEntry->setEvaluationError(globalObject, exception->value()); + + scope.throwException(globalObject, exception); + return; + } } - } else { - // require map was cleared of the entry + + moduleObject->toSyntheticSource(globalObject, moduleKey, exportNames, exportValues); + RETURN_IF_EXCEPTION(scope, {}); } }, sourceOrigin, diff --git a/src/jsc/bindings/ZigGlobalObject.cpp b/src/jsc/bindings/ZigGlobalObject.cpp index 04724225b631..5719e1b28a23 100644 --- a/src/jsc/bindings/ZigGlobalObject.cpp +++ b/src/jsc/bindings/ZigGlobalObject.cpp @@ -825,7 +825,41 @@ JSC_DEFINE_HOST_FUNCTION(functionEsmLoadSync, (JSC::JSGlobalObject * lexicalGlob } } - JSPromise* promise = loader->loadModuleSync(globalObject, key, nullptr, nullptr); + // Inlined JSModuleLoader::loadModuleSync plus one extra step. A mid-fetch + // entry's settle reactions live on the *outer* drain's synchronous module + // queue, which this nested load cannot reach, so reusing its pending fetch + // promise would misreport the module as async. Re-fetch and settle the + // entry while our queue is current, mirroring hostLoadImportedModule's + // synchronous-replay path for dependency edges. + JSC::VM::SynchronousModuleQueue syncQueue; + syncQueue.prev = vm.m_synchronousModuleQueue; + vm.m_synchronousModuleQueue = &syncQueue; + + if (auto* entry = loader->registryEntry(key)) { + if (entry->status() == JSC::ModuleRegistryEntry::Status::Fetching) { + // Attaches the fetch-settled reaction if nothing has yet. + entry->ensureModulePromise(globalObject); + JSPromise* fetchPromise = entry->ensureFetchPromise(globalObject); + if (!scope.exception() && fetchPromise->status() == JSPromise::Status::Pending) { + JSPromise* fetched = loader->fetch(globalObject, JSC::jsString(vm, keyString), nullptr, nullptr); + if (!scope.exception()) { + // pipeFrom() already claimed the resolving-function flag, so + // the guarded fulfill()/reject() would no-op; settle directly. + if (fetched->status() == JSPromise::Status::Fulfilled) + fetchPromise->fulfillPromise(vm, fetched->result()); + else if (fetched->status() == JSPromise::Status::Rejected) + fetchPromise->rejectPromise(vm, fetched->result()); + } + } + } + } + + JSPromise* promise = nullptr; + if (!scope.exception()) + promise = loader->loadModule(globalObject, key, nullptr, nullptr, { JSC::ModuleLoadFlag::Evaluate }); + if (!scope.exception()) + JSC::JSModuleLoader::drainSynchronousModuleQueue(globalObject); + vm.m_synchronousModuleQueue = syncQueue.prev; RETURN_IF_EXCEPTION(scope, {}); switch (promise->status()) { diff --git a/test/js/bun/resolve/require-esm-nested-cjs-sibling.test.ts b/test/js/bun/resolve/require-esm-nested-cjs-sibling.test.ts new file mode 100644 index 000000000000..164b5da61b05 --- /dev/null +++ b/test/js/bun/resolve/require-esm-nested-cjs-sibling.test.ts @@ -0,0 +1,103 @@ +// When a CommonJS entry require()s an ESM graph, the whole graph loads on the +// loader's synchronous module queue. A CJS module inside that graph has its +// body evaluated mid-load, and that body can require() an ESM sibling whose +// registry entry is mid-fetch: the reactions that would settle it sit on the +// *outer* drain's queue, which the nested synchronous load cannot reach. +// +// That used to have two faces: +// 1. require() of the in-flight sibling threw a spurious +// `require() async module "..." is unsupported` TypeError even though the +// sibling has no top-level await. +// 2. The TypeError aborted the CJS module's body, which evicted it from the +// require cache; a replayed makeModule then found no cache entry and +// silently produced an *empty* module, so the CJS module's top-level code +// never ran at all: no throw, exit 0, one module of the graph skipped. +import { expect, test } from "bun:test"; +import { bunEnv, bunExe, tempDir } from "harness"; + +const graph = { + "main.cjs": `require("./a.mjs");`, + "a.mjs": `import "./b.mjs"; +import "./d.mjs"; +import "./e.mjs"; +(globalThis.T ??= []).push("a"); +console.log("N=" + globalThis.T.length + " " + globalThis.T.join(","));`, + "b.mjs": `import "./c.cjs"; +(globalThis.T ??= []).push("b");`, + "c.cjs": `require("./e.mjs"); +(globalThis.T ??= []).push("c");`, + "d.mjs": `import "./f.cjs"; +(globalThis.T ??= []).push("d");`, + "e.mjs": `import "./f.cjs"; +import "./h.mjs"; +(globalThis.T ??= []).push("e");`, + "f.cjs": `require("./h.mjs"); +(globalThis.T ??= []).push("f"); +console.log("f.cjs evaluated");`, + "h.mjs": `(globalThis.T ??= []).push("h");`, +}; + +test.concurrent("require() of an ESM entry evaluates every CommonJS module in the graph", async () => { + using dir = tempDir("require-esm-nested-cjs", graph); + await using proc = Bun.spawn({ + cmd: [bunExe(), "main.cjs"], + env: bunEnv, + cwd: String(dir), + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + // Same evaluation order Node prints. Before the fix, f.cjs was silently + // skipped: "N=6 h,e,c,b,d,a" and no "f.cjs evaluated" line. + expect(stdout).toBe("f.cjs evaluated\nN=7 h,f,e,c,b,d,a\n"); + expect(exitCode).toBe(0); +}); + +test.concurrent("a CommonJS module throwing inside a require()d ESM graph surfaces the error", async () => { + using dir = tempDir("require-esm-nested-cjs-throw", { + ...graph, + "f.cjs": `require("./h.mjs"); +throw new Error("boom from f.cjs");`, + }); + await using proc = Bun.spawn({ + cmd: [bunExe(), "main.cjs"], + env: bunEnv, + cwd: String(dir), + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // Before the fix this printed "N=6 h,e,c,b,d,a" and exited 0: the throw was + // swallowed along with the module. + expect(stderr).toContain("boom from f.cjs"); + expect(stdout).toBe(""); + expect(exitCode).toBe(1); +}); + +test.concurrent( + "a caught require() of a throwing CJS sibling still fails its import edge with the original error", + async () => { + using dir = tempDir("require-esm-nested-cjs-caught", { + "main.cjs": `require("./a.mjs");`, + "a.mjs": `import "./e.mjs"; +console.log("a evaluated");`, + "e.mjs": `import "./g.cjs"; +import "./f.cjs";`, + // g.cjs evaluates f.cjs first (both are in the require cache before either + // runs) and swallows the throw; the import edge of f.cjs must still reject + // with f's real error, not succeed with an empty module. + "g.cjs": `try { require("./f.cjs"); } catch {}`, + "f.cjs": `throw new Error("boom from f.cjs");`, + }); + await using proc = Bun.spawn({ + cmd: [bunExe(), "main.cjs"], + env: bunEnv, + cwd: String(dir), + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // Before the fix: "a evaluated" and exit 0, with f.cjs silently skipped. + expect(stderr).toContain("boom from f.cjs"); + expect(stdout).toBe(""); + expect(exitCode).toBe(1); + }, +);