New module registry fixes

docs/reference/detail/legacy-module-registry.md

@@ -3,7 +3,8 @@
 Reference for reasoning about the original (legacy) module registry implementation,
 how it interfaces with V8's module APIs, and how modules flow from config through
 compilation to evaluation. For V8 module internals, see
-[v8-module-internals.md](v8-module-internals.md).
+[v8-module-internals.md](v8-module-internals.md). For the new replacement
+implementation, see [new-module-registry.md](new-module-registry.md).
 
 ## Source Files
 

docs/reference/detail/new-module-registry.md

@@ -118,6 +118,19 @@ IsolateModuleRegistry
 | `EVAL` | 0x04 | Requires evaluation outside IoContext (deferred to `EvalCallback`) |
 | `WASM` | 0x08 | WebAssembly module                                                 |
 
+### `Module::ContentType` enum
+
+Used to validate import attributes (e.g. `with { type: 'json' }`). Set at
+module construction time.
+
+| ContentType | Meaning                                              |
+| ----------- | ---------------------------------------------------- |
+| `NONE`      | No specific content type (ESM, CJS, builtin objects) |
+| `JSON`      | JSON module                                          |
+| `TEXT`      | Text module                                          |
+| `DATA`      | Data/binary module                                   |
+| `WASM`      | WebAssembly module                                   |
+
 ## Module Subclasses
 
 `Module` is an abstract base class. Two concrete implementations exist
@@ -165,9 +178,14 @@ SyntheticModule extends Module {
 
 - `getDescriptor()`: Calls `v8::Module::CreateSyntheticModule` with the declared
   export names (always includes `"default"` plus any `namedExports`).
-- `evaluate()`: Creates a resolved Promise, calls the `callback` which uses the
-  `ModuleNamespace` helper to set exports via `SetSyntheticModuleExport`.
-- For modules with `Flags::EVAL`, delegates to the `Evaluator` first.
+- `evaluate()`: Ensures the module is instantiated, optionally delegates to the
+  `Evaluator` (for `Flags::EVAL` modules), then calls `module->Evaluate()` to
+  enter V8's status machine. V8 calls back into `evaluationSteps` which calls
+  `actuallyEvaluate()`.
+- `actuallyEvaluate()`: Creates a resolved Promise, calls the `callback` which
+  uses the `ModuleNamespace` helper to set exports via
+  `SetSyntheticModuleExport`. This is always invoked via V8's evaluation steps
+  callback, never called directly from external code.
 
 ### Static `evaluationSteps` Callback
 
@@ -177,7 +195,14 @@ as the V8 `SyntheticModuleEvaluationSteps`. When V8 calls it:
 1. Gets the `IsolateModuleRegistry` from context embedder data.
 2. Looks up the `Entry` by v8::Module identity (hash-indexed, O(1) — unlike the
    legacy registry's O(n) linear scan).
-3. Calls `module.evaluate()` on the found entry.
+3. Calls `actuallyEvaluate()` on the found module — not `evaluate()`, to avoid
+   reentry into `module->Evaluate()`.
+
+This design ensures V8 always manages the status transitions (`kEvaluating` →
+`kEvaluated` or `kErrored`) regardless of whether evaluation was initiated by
+V8 (static import) or by our code (dynamic import, require). The `evaluate()`
+method is the external entry point that goes through V8; `evaluationSteps` is
+V8's callback entry point that goes directly to the work.
 
 ## ModuleBundle — Sources of Modules
 
@@ -355,17 +380,22 @@ builder.setEvalCallback([](Lock& js, const auto& module, auto v8Module,
 ### How it flows
 
 1. `module.evaluate(js, v8Module, observer, evaluator)` is called.
-2. For modules with `Flags::EVAL` set (all ESM modules, and synthetic modules
-   that opt in):
+2. For ESM modules (`Flags::EVAL` always set):
    - The `evaluator(js, module, v8Module, observer)` is invoked.
    - The evaluator calls `ModuleRegistry::evaluateImpl` which invokes the
-     `EvalCallback`.
-   - If the callback returns a `Promise<Value>`, it is wrapped and returned.
-3. If the evaluator returns `kj::none` (no callback set), or the module doesn't
-   have `Flags::EVAL`, the module's `actuallyEvaluate()` is called directly.
-4. For ESM: `actuallyEvaluate` calls `v8::Module::Evaluate()`.
-5. For Synthetic: `actuallyEvaluate` creates a resolved Promise, then invokes
-   the synthetic `callback` to set exports.
+     `EvalCallback` (wrapping evaluation in `SuppressIoContextScope`).
+   - The `EvalCallback` calls `v8Module->Evaluate()`. V8 evaluates the
+     source text directly.
+3. For Synthetic modules (dynamic import and require paths):
+   - If `Flags::EVAL` is set, the evaluator is invoked first (same as ESM).
+   - Otherwise, `module->Evaluate()` is called directly.
+   - V8 sets status to `kEvaluating` and calls the `evaluationSteps` callback.
+   - `evaluationSteps` calls `actuallyEvaluate()` which runs the callback
+     to set exports.
+   - V8 receives the result and sets status to `kEvaluated` or `kErrored`.
+4. For static imports of synthetic modules, V8 drives `Module::Evaluate()`
+   itself, which calls `evaluationSteps` → `actuallyEvaluate()`. The same
+   code path executes; only the initial caller differs.
 
 ## `import.meta` Support
 
@@ -419,12 +449,14 @@ compile cache).
 `Module::newCjsStyleModuleHandler<T, TypeWrapper>` is a template that creates a
 handler for CommonJS-style modules:
 
-1. Guards against re-evaluation with `EvaluateOnce` (CJS modules evaluate once).
-2. Allocates a JSG resource object of type `T` (e.g. `CommonJsModuleContext`).
-3. Compiles the source as a function via `ScriptCompiler::CompileFunction` with
+1. Allocates a JSG resource object of type `T` (e.g. `CommonJsModuleContext`).
+2. Compiles the source as a function via `ScriptCompiler::CompileFunction` with
    the JSG object as extension object (providing `module`, `exports`, `require`).
-4. Calls the compiled function.
-5. Extracts `exports` from the JSG object and sets them on the module namespace.
+3. Calls the compiled function.
+4. Extracts `exports` from the JSG object and sets them on the module namespace.
+
+Re-evaluation is prevented by V8's module status machine (`kEvaluated` prevents
+re-entry), not by application-level guards.
 
 ## Specifier Processing
 
@@ -622,6 +654,22 @@ Aliases are single-level redirects. When a bundle lookup returns a string
 2. `lookupImpl` recurses with the new specifier, but with a `recursed=true`
    flag that prevents further recursion. Only one level of aliasing is supported.
 
+### Error Propagation for Errored Dependencies
+
+V8's `InnerModuleEvaluation` only recurses into `SourceTextModule` dependencies
+when checking for `kErrored` status — it skips `SyntheticModule` dependencies
+entirely. This means if a synthetic module is `kErrored`, an ESM that imports
+it would evaluate successfully with `undefined` export values instead of
+propagating the error.
+
+To work around this, `resolveModuleCallback` checks the resolved module's
+status after resolution. If the module is `kErrored`, the callback throws the
+module's cached exception instead of returning the handle to V8. This prevents
+V8 from instantiating an ESM graph containing errored synthetic dependencies.
+
+Similarly, `dynamicResolve` checks for `kErrored` before calling `evaluate()`,
+returning a rejected promise with the cached exception.
+
 ## Thread Safety Model
 
 ### Shared State (ModuleRegistry, ModuleBundle, Module)
@@ -667,14 +715,46 @@ Aliases are single-level redirects. When a bundle lookup returns a string
    functions can be called multiple times from multiple isolates. They create
    fresh JS objects each time.
 
-7. **Import attributes are rejected.** The current implementation throws
-   `TypeError` for any import attributes. This is the spec-recommended default
-   for unrecognized attributes.
-
-8. **No `require(esm)` convention matching.** Unlike the legacy registry which
-   had complex `require()` return value logic (checking `__cjsUnwrapDefault`,
-   `module.exports` key, etc.), the new registry's `require()` always returns
-   the full module namespace. CJS interop is handled at the module handler level.
+7. **Import attributes are validated.** The `type` import attribute is supported
+   for both static and dynamic imports. Each `Module` has a `ContentType`
+   (`NONE`, `JSON`, `TEXT`, `DATA`, `WASM`) set at construction time. Supported
+   type values and their corresponding TC39 proposals:
+   - `type: 'json'` — JSON Modules (TC39 Stage 4, finished) → `ContentType::JSON`.
+     This is the only type currently enabled.
+   - `type: 'text'` — Import Text (TC39 Stage 3) → `ContentType::TEXT`.
+     Recognized but rejected with "not yet supported" pending Stage 4.
+   - `type: 'bytes'` — Import Bytes (TC39 Stage 2.7) → `ContentType::DATA`.
+     Recognized but rejected with "not yet supported". The proposal requires
+     `Uint8Array` backed by an immutable `ArrayBuffer`, which is not yet
+     implemented. Data modules currently expose mutable `ArrayBuffer`s.
+     All three types are parsed in `parseImportAttributes`. Only `json` passes
+     validation in `validateImportType`; `text` and `bytes` are rejected there
+     with specific error messages. The `ContentType` enum and module tagging are
+     fully plumbed for all three, ready to enable when the proposals are ready.
+     When a type attribute is specified, the resolved module's content type must
+     match or a `TypeError` is thrown. Unrecognized attribute keys (anything
+     other than `type`) and unsupported type values are rejected with `TypeError`.
+     Attribute parsing is handled
+     by `parseImportAttributes()` which reads V8's `FixedArray` of key-value-
+     location triples. Validation is handled by `validateImportType()` which
+     compares the declared type against `Module::contentType()`. Both static
+     imports (`resolveModuleCallback`) and dynamic imports
+     (`dynamicImportModuleCallback` → `dynamicResolve`) use these helpers.
+
+8. **`require()` return value semantics (UNWRAP_DEFAULT).** When callers use the
+   `UNWRAP_DEFAULT` require option (used by `createRequire` and
+   `CommonJsModuleContext::require`), the return value depends on the module type:
+   - **User bundle ESM**: returns the module namespace (matching Node.js
+     `require(esm)` behavior), unless the module exports `__cjsUnwrapDefault`
+     as truthy (a convention used by bundlers like esbuild when transpiling CJS
+     to ESM), in which case the `default` export is returned.
+   - **Builtin ESM** (`node:assert`, `node:buffer`, etc.): returns the `default`
+     export, because workerd implements builtins as ESM that wrap CJS-style APIs
+     in a default export.
+   - **Synthetic modules** (CJS, JSON, Text, Data, WASM): returns the `default`
+     export, which is where the module's value lives (`module.exports` for CJS,
+     parsed value for JSON, etc.).
+     Without `UNWRAP_DEFAULT`, `require()` always returns the full module namespace.
 
 9. **Python modules are not yet supported.** The new registry currently throws
    `KJ_FAIL_ASSERT` for `PythonModule` content. Python support remains on the

docs/reference/detail/v8-module-internals.md

@@ -1,7 +1,11 @@
 # V8 Module System Internals
 
 Reference for reasoning about V8's module lifecycle, binding resolution, and evaluation
-semantics. All file paths are relative to the V8 source root
+semantics. For how workerd interfaces with these V8 APIs, see
+[new-module-registry.md](new-module-registry.md) (current implementation) and
+[legacy-module-registry.md](legacy-module-registry.md) (legacy implementation).
+
+All file paths are relative to the V8 source root
 (`external/+http_archive+v8/`). Line numbers are approximate and may drift across V8
 versions.
 

src/workerd/api/commonjs.c++

@@ -25,9 +25,15 @@ jsg::JsValue CommonJsModuleContext::require(jsg::Lock& js, kj::String specifier)
   }
 
   if (FeatureFlags::get(js).getNewModuleRegistry()) {
-    return jsg::modules::ModuleRegistry::resolve(js, specifier, "default"_kj,
-        jsg::modules::ResolveContext::Type::BUNDLE, jsg::modules::ResolveContext::Source::REQUIRE,
-        KJ_ASSERT_NONNULL(pathOrSpecifier.tryGet<jsg::Url>()));
+    auto& referrer = KJ_ASSERT_NONNULL(pathOrSpecifier.tryGet<jsg::Url>());
+    KJ_IF_SOME(ns,
+        jsg::modules::ModuleRegistry::tryResolveModuleNamespace(js, specifier,
+            jsg::modules::ResolveContext::Type::BUNDLE,
+            jsg::modules::ResolveContext::Source::REQUIRE, referrer,
+            jsg::modules::UnwrapDefault::YES)) {
+      return ns;
+    }
+    JSG_FAIL_REQUIRE(Error, kj::str("Module not found: ", specifier));
   }
 
   auto& path = KJ_ASSERT_NONNULL(pathOrSpecifier.tryGet<kj::Path>());

src/workerd/api/node/module.c++

@@ -32,9 +32,14 @@ jsg::JsValue ModuleUtil::createRequire(jsg::Lock& js, kj::String path) {
           specifier = kj::mv(nodeSpec);
         }
       }
-      return jsg::modules::ModuleRegistry::resolve(js, specifier, "default"_kj,
-          jsg::modules::ResolveContext::Type::BUNDLE, jsg::modules::ResolveContext::Source::REQUIRE,
-          referrer);
+      KJ_IF_SOME(val,
+          jsg::modules::ModuleRegistry::tryResolveModuleNamespace(js, specifier,
+              jsg::modules::ResolveContext::Type::BUNDLE,
+              jsg::modules::ResolveContext::Source::REQUIRE, referrer,
+              jsg::modules::UnwrapDefault::YES)) {
+        return val;
+      }
+      JSG_FAIL_REQUIRE(Error, kj::str("Module not found: ", specifier));
     }));
   }
 

src/workerd/api/tests/new-module-registry-test.js

@@ -7,6 +7,7 @@ import {
   ok,
   rejects,
   strictEqual,
+  throws,
   deepStrictEqual,
 } from 'assert'; // Intentionally omit the 'node:' prefix
 import { foo, default as def } from 'foo';
@@ -190,6 +191,17 @@ await rejects(import('%90%E8%54%C1'), {
   message: /Module not found/,
 });
 
+// The cjs6 module attempts to require and esm with a top-level await, which is rejected
+// following node.js' established require(esm) precedent.
+await rejects(import('cjs6'), {
+  message: /^Top-level await is not supported/,
+});
+
+// Cannot directly require an ESM with top-level await either.
+throws(() => myRequire('tla'), {
+  message: /^Top-level await is not supported/,
+});
+
 // Verify that a module is unable to perform IO operations at the top level, even if
 // the dynamic import is initiated within the scope of an active IoContext.
 export const noTopLevelIo = {
@@ -246,15 +258,14 @@ export const queryAndFragment = {
   },
 };
 
-// We do not currently support import attributes. Per the recommendation
-// in the spec, we throw an error when they are encountered.
+// Unrecognized import attributes are rejected.
 export const importAssertionsFail = {
   async test() {
     await rejects(import('ia'), {
-      message: /^Import attributes are not supported/,
+      message: /^Unsupported import attribute: "a"/,
     });
     await rejects(import('foo', { with: { a: 'abc' } }), {
-      message: /^Import attributes are not supported/,
+      message: /^Unsupported import attribute: "a"/,
     });
   },
 };

src/workerd/api/tests/new-module-registry-test.wd-test

@@ -34,6 +34,7 @@ const unitTests :Workerd.Config = (
           # Intentional circular dependency
           (name = "cjs4", commonJsModule = "module.exports = require('cjs5')"),
           (name = "cjs5", commonJsModule = "module.exports = require('cjs4')"),
+          (name = "cjs6", commonJsModule = "module.exports = require('tla')"),
 
           # Other module types work
           (name = "text", text = "abc"),
@@ -68,6 +69,8 @@ const unitTests :Workerd.Config = (
 
           # Unicode characters in the path should be handled as UTF-8 and supported.
           (name = "部品", esModule = "export default 1;"),
+
+          (name = "tla", esModule = "export default await import('text')"),
         ],
         compatibilityFlags = [
           "nodejs_compat_v2",

src/workerd/io/worker-modules.h

@@ -155,17 +155,19 @@ static kj::Arc<jsg::modules::ModuleRegistry> newWorkerModuleRegistry(
           // module registry. We can safely pass a reference to the module handler.
           // It will not be copied into a JS string until the module is actually
           // evaluated.
-          bundleBuilder.addSyntheticModule(
-              def.name, jsg::modules::Module::newTextModuleHandler(content.body));
+          bundleBuilder.addSyntheticModule(def.name,
+              jsg::modules::Module::newTextModuleHandler(content.body), nullptr,
+              jsg::modules::Module::ContentType::TEXT);
           break;
         }
         KJ_CASE_ONEOF(content, Worker::Script::DataModule) {
           // The content.body is memory-resident and is expected to outlive the
           // module registry. We can safely pass a reference to the module handler.
           // It will not be copied into a JS string until the module is actually
           // evaluated.
-          bundleBuilder.addSyntheticModule(
-              def.name, jsg::modules::Module::newDataModuleHandler(content.body));
+          bundleBuilder.addSyntheticModule(def.name,
+              jsg::modules::Module::newDataModuleHandler(content.body), nullptr,
+              jsg::modules::Module::ContentType::DATA);
           break;
         }
         KJ_CASE_ONEOF(content, Worker::Script::WasmModule) {
@@ -181,8 +183,9 @@ static kj::Arc<jsg::modules::ModuleRegistry> newWorkerModuleRegistry(
           // module registry. We can safely pass a reference to the module handler.
           // It will not be copied into a JS string until the module is actually
           // evaluated.
-          bundleBuilder.addSyntheticModule(
-              def.name, jsg::modules::Module::newJsonModuleHandler(content.body));
+          bundleBuilder.addSyntheticModule(def.name,
+              jsg::modules::Module::newJsonModuleHandler(content.body), nullptr,
+              jsg::modules::Module::ContentType::JSON);
           break;
         }
         KJ_CASE_ONEOF(content, Worker::Script::CommonJsModule) {

src/workerd/jsg/jsg.c++

@@ -358,7 +358,8 @@ kj::Maybe<JsObject> Lock::resolveInternalModule(kj::StringPtr specifier) {
   auto& isolate = IsolateBase::from(v8Isolate);
   if (isolate.isUsingNewModuleRegistry()) {
     return jsg::modules::ModuleRegistry::tryResolveModuleNamespace(
-        *this, specifier, jsg::modules::ResolveContext::Type::BUILTIN);
+        *this, specifier, jsg::modules::ResolveContext::Type::BUILTIN)
+        .map([](JsValue val) { return KJ_ASSERT_NONNULL(val.tryCast<JsObject>()); });
   }
 
   // Use the original module registry implementation
@@ -372,14 +373,16 @@ kj::Maybe<JsObject> Lock::resolvePublicBuiltinModule(kj::StringPtr specifier) {
   auto& isolate = IsolateBase::from(v8Isolate);
   KJ_ASSERT(isolate.isUsingNewModuleRegistry());
   return jsg::modules::ModuleRegistry::tryResolveModuleNamespace(
-      *this, specifier, jsg::modules::ResolveContext::Type::PUBLIC_BUILTIN);
+      *this, specifier, jsg::modules::ResolveContext::Type::PUBLIC_BUILTIN)
+      .map([](JsValue val) { return KJ_ASSERT_NONNULL(val.tryCast<JsObject>()); });
 }
 
 kj::Maybe<JsObject> Lock::resolveModule(kj::StringPtr specifier, RequireEsm requireEsm) {
   auto& isolate = IsolateBase::from(v8Isolate);
   if (isolate.isUsingNewModuleRegistry()) {
     return jsg::modules::ModuleRegistry::tryResolveModuleNamespace(
-        *this, specifier, jsg::modules::ResolveContext::Type::BUNDLE);
+        *this, specifier, jsg::modules::ResolveContext::Type::BUNDLE)
+        .map([](JsValue val) { return KJ_ASSERT_NONNULL(val.tryCast<JsObject>()); });
   }
 
   auto moduleRegistry = jsg::ModuleRegistry::from(*this);