require(esm): return the namespace when the module is evicted from require.cache during evaluation

src/js/builtins/CommonJS.ts

@@ -106,34 +106,33 @@ export function overridableRequire(this: JSCommonJSModule, originalId: string, o
 
   // -1 means we need to lookup the module from the ESM registry.
   if (out === -1) {
+    // Keep the namespace $requireESM returns instead of looking the registry up
+    // again: the module may have deleted itself from require.cache while evaluating.
+    let namespace: any;
     try {
-      out = $requireESM(id);
+      namespace = $requireESM(id);
     } catch (exception) {
       // Since the ESM code is mostly JS, we need to handle exceptions here.
       $requireMap.$delete(id);
       throw exception;
     }
 
-    // If we can pull out a ModuleNamespaceObject, let's do it.
-    const namespace = $esmNamespaceForCjs(id);
-    if (namespace !== undefined) {
-      // In Bun, when __esModule is not defined, it's a CustomAccessor on the prototype.
-      // Various libraries expect __esModule to be set when using ESM from require().
-      // We don't want to always inject the __esModule export into every module,
-      // And creating an Object wrapper causes the actual exports to not be own properties.
-      // So instead of either of those, we make it so that the __esModule property can be set at runtime.
-      // It only supports "true" and undefined. Anything non-truthy is treated as undefined.
-      // https://github.com/oven-sh/bun/issues/14411
-      if (namespace.__esModule === undefined) {
-        try {
-          namespace.__esModule = true;
-        } catch {
-          // https://github.com/oven-sh/bun/issues/17816
-        }
+    // In Bun, when __esModule is not defined, it's a CustomAccessor on the prototype.
+    // Various libraries expect __esModule to be set when using ESM from require().
+    // We don't want to always inject the __esModule export into every module,
+    // And creating an Object wrapper causes the actual exports to not be own properties.
+    // So instead of either of those, we make it so that the __esModule property can be set at runtime.
+    // It only supports "true" and undefined. Anything non-truthy is treated as undefined.
+    // https://github.com/oven-sh/bun/issues/14411
+    if (namespace.__esModule === undefined) {
+      try {
+        namespace.__esModule = true;
+      } catch {
+        // https://github.com/oven-sh/bun/issues/17816
       }
-
-      return (mod.exports = namespace["module.exports"] ?? namespace);
     }
+
+    return (mod.exports = namespace["module.exports"] ?? namespace);
   }
 
   const c = $evaluateCommonJSModule(mod, this);
@@ -320,42 +319,40 @@ export function requireESM(this, resolved: string) {
     exports = $loadEsmIntoCjs(resolved);
   }
   if (exports === undefined) {
-    throw new TypeError(`require() failed to evaluate module "${resolved}". This is an internal consistentency error.`);
+    throw new TypeError(`require() failed to evaluate module "${resolved}". This is an internal consistency error.`);
   }
   return exports;
 }
 
 export function requireESMFromHijackedExtension(this: JSCommonJSModule, id: string) {
   $assert(this);
+  // As in overridableRequire, the module may have deleted itself from
+  // require.cache while evaluating, so don't look the namespace up again.
+  let namespace: any;
   try {
-    $requireESM(id);
+    namespace = $requireESM(id);
   } catch (exception) {
     // Since the ESM code is mostly JS, we need to handle exceptions here.
     $requireMap.$delete(id);
     throw exception;
   }
 
-  // If we can pull out a ModuleNamespaceObject, let's do it.
-  const namespace = $esmNamespaceForCjs(id);
-  if (namespace !== undefined) {
-    // In Bun, when __esModule is not defined, it's a CustomAccessor on the prototype.
-    // Various libraries expect __esModule to be set when using ESM from require().
-    // We don't want to always inject the __esModule export into every module,
-    // And creating an Object wrapper causes the actual exports to not be own properties.
-    // So instead of either of those, we make it so that the __esModule property can be set at runtime.
-    // It only supports "true" and undefined. Anything non-truthy is treated as undefined.
-    // https://github.com/oven-sh/bun/issues/14411
-    if (namespace.__esModule === undefined) {
-      try {
-        namespace.__esModule = true;
-      } catch {
-        // https://github.com/oven-sh/bun/issues/17816
-      }
+  // In Bun, when __esModule is not defined, it's a CustomAccessor on the prototype.
+  // Various libraries expect __esModule to be set when using ESM from require().
+  // We don't want to always inject the __esModule export into every module,
+  // And creating an Object wrapper causes the actual exports to not be own properties.
+  // So instead of either of those, we make it so that the __esModule property can be set at runtime.
+  // It only supports "true" and undefined. Anything non-truthy is treated as undefined.
+  // https://github.com/oven-sh/bun/issues/14411
+  if (namespace.__esModule === undefined) {
+    try {
+      namespace.__esModule = true;
+    } catch {
+      // https://github.com/oven-sh/bun/issues/17816
     }
-
-    this.exports = namespace["module.exports"] ?? namespace;
-    return;
   }
+
+  this.exports = namespace["module.exports"] ?? namespace;
 }
 
 $visibility = "Private";

src/jsc/bindings/ZigGlobalObject.cpp

@@ -825,10 +825,18 @@ JSC_DEFINE_HOST_FUNCTION(functionEsmLoadSync, (JSC::JSGlobalObject * lexicalGlob
 
     auto* loader = globalObject->moduleLoader();
     bool entryExistedBefore = false;
+    // fetchCommonJSModule() already parsed the file into the registry, so the
+    // record to evaluate is normally known now. Keep it rather than re-reading
+    // the registry after the load: the module's top level (or a dependency's)
+    // may `delete require.cache[...]` while it runs, and like a static import
+    // this load still returns the instance it evaluated. The eviction only
+    // affects the next load.
+    JSC::AbstractModuleRecord* record = nullptr;
     if (auto* entry = loader->registryEntry(key)) {
         entryExistedBefore = true;
-        if (isModuleEvaluated(entry->record())) {
-            auto* ns = entry->record()->getModuleNamespace(globalObject, false);
+        record = entry->record();
+        if (isModuleEvaluated(record)) {
+            auto* ns = record->getModuleNamespace(globalObject, false);
             RETURN_IF_EXCEPTION(scope, {});
             return JSValue::encode(ns);
         }
@@ -837,6 +845,13 @@ JSC_DEFINE_HOST_FUNCTION(functionEsmLoadSync, (JSC::JSGlobalObject * lexicalGlob
     JSPromise* promise = loader->loadModuleSync(globalObject, key, nullptr, nullptr);
     RETURN_IF_EXCEPTION(scope, {});
 
+    // Builtin ES modules and module mocks arrive without a parsed entry; the
+    // load itself creates it.
+    if (!record) {
+        if (auto* entry = loader->registryEntry(key))
+            record = entry->record();
+    }
+
     switch (promise->status()) {
     case JSPromise::Status::Fulfilled:
         break;
@@ -859,12 +874,10 @@ JSC_DEFINE_HOST_FUNCTION(functionEsmLoadSync, (JSC::JSGlobalObject * lexicalGlob
         // it OR any dependency has top-level await, in which case bindings can
         // still be in TDZ and we must throw the "async module" error instead
         // of returning a half-initialized namespace.
-        if (auto* entry = loader->registryEntry(key)) {
-            if (auto* cyclic = dynamicDowncast<JSC::CyclicModuleRecord>(entry->record())) {
-                auto status = cyclic->status();
-                if ((status == JSC::CyclicModuleRecord::Status::Evaluating || status == JSC::CyclicModuleRecord::Status::Evaluated) && !cyclic->hasTLA() && !cyclic->evaluationError())
-                    break;
-            }
+        if (auto* cyclic = dynamicDowncast<JSC::CyclicModuleRecord>(record)) {
+            auto status = cyclic->status();
+            if ((status == JSC::CyclicModuleRecord::Status::Evaluating || status == JSC::CyclicModuleRecord::Status::Evaluated) && !cyclic->hasTLA() && !cyclic->evaluationError())
+                break;
         }
         // Only drop the entry we created. If the entry already existed (an
         // outer import() is mid-load, or the module is EvaluatingAsync from a
@@ -878,9 +891,8 @@ JSC_DEFINE_HOST_FUNCTION(functionEsmLoadSync, (JSC::JSGlobalObject * lexicalGlob
     }
     }
 
-    auto* entry = loader->registryEntry(key);
-    if (!entry || !entry->record()) [[unlikely]]
-        return throwVMTypeError(globalObject, scope, makeString("require() failed to evaluate module \""_s, keyString, "\". This is an internal consistentency error."_s));
+    if (!record) [[unlikely]]
+        return throwVMTypeError(globalObject, scope, makeString("require() failed to evaluate module \""_s, keyString, "\". This is an internal consistency error."_s));
 
     // The loadModule promise resolved, so the entire graph linked + evaluated
     // synchronously. We deliberately do NOT gate on CyclicModuleRecord::status()
@@ -891,7 +903,6 @@ JSC_DEFINE_HOST_FUNCTION(functionEsmLoadSync, (JSC::JSGlobalObject * lexicalGlob
     // the namespace in that state matches the old loader's behaviour and Node's
     // require(esm) cycle semantics. evaluationError() still surfaces a real
     // throw from the module body.
-    auto* record = entry->record();
     if (auto* cyclic = dynamicDowncast<JSC::CyclicModuleRecord>(record)) {
         if (JSValue err = cyclic->evaluationError()) {
             scope.throwException(globalObject, err);

test/js/bun/resolve/require.test.ts

@@ -1,4 +1,4 @@
-import { bunRun, tempDirWithFiles } from "harness";
+import { bunRun, tempDir, tempDirWithFiles } from "harness";
 import fs from "node:fs";
 import path from "node:path";
 const fixture = (...segs: string[]): string => path.join(import.meta.dirname, "fixtures", "require", ...segs);
@@ -83,3 +83,115 @@ describe("require(specifier)", () => {
     });
   });
 });
+
+// The ESM registry entry is removed while the module's own top level is still
+// running. require() must still return the namespace of the instance it just
+// evaluated; the eviction applies to the next require(), which evaluates the
+// file again.
+describe.concurrent("require(esm) of a module evicted from require.cache during its own evaluation", () => {
+  const selfEvicting = /* js */ `
+    globalThis.evaluations = (globalThis.evaluations ?? 0) + 1;
+    delete require.cache[import.meta.path];
+    export const x = 1;
+    export const evaluation = globalThis.evaluations;
+  `;
+
+  it("returns the namespace when the module deletes itself from require.cache", async () => {
+    using dir = tempDir("require-esm-self-evict", {
+      "self-evict.mjs": selfEvicting,
+      "entry.cjs": /* js */ `
+        const first = require("./self-evict.mjs");
+        const cachedAfterFirst = require.resolve("./self-evict.mjs") in require.cache;
+        const second = require("./self-evict.mjs");
+        console.log(JSON.stringify({
+          first: { x: first.x, evaluation: first.evaluation },
+          cachedAfterFirst,
+          second: { x: second.x, evaluation: second.evaluation },
+          sameNamespace: first === second,
+        }));
+      `,
+    });
+    const { stdout, stderr, exitCode } = await bunRun(path.join(dir, "entry.cjs"));
+    expect(stderr).toBe("");
+    expect(JSON.parse(stdout)).toEqual({
+      first: { x: 1, evaluation: 1 },
+      cachedAfterFirst: false,
+      second: { x: 1, evaluation: 2 },
+      sameNamespace: false,
+    });
+    expect(exitCode).toBe(0);
+  });
+
+  it("returns the namespace when a dependency clears require.cache", async () => {
+    using dir = tempDir("require-esm-dep-clears-cache", {
+      "clear-cache.mjs": /* js */ `
+        for (const key of Object.keys(require.cache)) delete require.cache[key];
+        export const cleared = true;
+      `,
+      "leaf.mjs": /* js */ `
+        export const leaf = "leaf";
+      `,
+      "root.mjs": /* js */ `
+        import { cleared } from "./clear-cache.mjs";
+        export * from "./leaf.mjs";
+        export { cleared };
+        globalThis.evaluations = (globalThis.evaluations ?? 0) + 1;
+        export const evaluation = globalThis.evaluations;
+      `,
+      "entry.cjs": /* js */ `
+        const first = require("./root.mjs");
+        const cachedAfterFirst = require.resolve("./root.mjs") in require.cache;
+        const second = require("./root.mjs");
+        console.log(JSON.stringify({
+          first: { cleared: first.cleared, leaf: first.leaf, evaluation: first.evaluation },
+          cachedAfterFirst,
+          second: { cleared: second.cleared, leaf: second.leaf, evaluation: second.evaluation },
+          sameNamespace: first === second,
+        }));
+      `,
+    });
+    const { stdout, stderr, exitCode } = await bunRun(path.join(dir, "entry.cjs"));
+    expect(stderr).toBe("");
+    expect(JSON.parse(stdout)).toEqual({
+      first: { cleared: true, leaf: "leaf", evaluation: 1 },
+      cachedAfterFirst: false,
+      second: { cleared: true, leaf: "leaf", evaluation: 2 },
+      sameNamespace: false,
+    });
+    expect(exitCode).toBe(0);
+  });
+
+  // A user-wrapped require.extensions loader reaches the ESM namespace through
+  // Module._extensions (requireESMFromHijackedExtension) rather than require().
+  it("sets module.exports when loaded through a wrapped require.extensions loader", async () => {
+    using dir = tempDir("require-esm-self-evict-extensions", {
+      "self-evict.mjs": selfEvicting,
+      "entry.cjs": /* js */ `
+        const builtinLoader = require.extensions[".mjs"];
+        let wrapperCalls = 0;
+        require.extensions[".mjs"] = function (module, filename) {
+          wrapperCalls++;
+          builtinLoader(module, filename);
+        };
+        const first = require("./self-evict.mjs");
+        const cachedAfterFirst = require.resolve("./self-evict.mjs") in require.cache;
+        const second = require("./self-evict.mjs");
+        console.log(JSON.stringify({
+          wrapperCalls,
+          first: { x: first.x, evaluation: first.evaluation },
+          cachedAfterFirst,
+          second: { x: second.x, evaluation: second.evaluation },
+        }));
+      `,
+    });
+    const { stdout, stderr, exitCode } = await bunRun(path.join(dir, "entry.cjs"));
+    expect(stderr).toBe("");
+    expect(JSON.parse(stdout)).toEqual({
+      wrapperCalls: 2,
+      first: { x: 1, evaluation: 1 },
+      cachedAfterFirst: false,
+      second: { x: 1, evaluation: 2 },
+    });
+    expect(exitCode).toBe(0);
+  });
+});