Refinements on the new module registry implementation

Author: jasnell

src/workerd/api/global-scope.c++

@@ -870,47 +870,36 @@ void ServiceWorkerGlobalScope::reportError(jsg::Lock& js, jsg::JsValue error) {
   }
 }
 
-namespace {
-jsg::JsValue resolveFromRegistry(jsg::Lock& js, kj::StringPtr specifier) {
-  auto moduleRegistry = jsg::ModuleRegistry::from(js);
-  if (moduleRegistry == nullptr) {
-    // TODO: Return the known built-in instead? This gets a bit tricky as we currently
-    // have no mechanism defined for accessing and caching the built-in module without
-    // the module registry. Should we even support this case at all? Without the module
-    // registry the user can't access the other importable modules anyway. For now, just
-    // returning undefined in this case seems the most appropriate.
-    return js.undefined();
-  }
-
-  auto spec = kj::Path::parse(specifier);
-  auto& info = JSG_REQUIRE_NONNULL(
-      moduleRegistry->resolve(js, spec), Error, kj::str("No such module: ", specifier));
-  auto module = info.module.getHandle(js);
-
-  jsg::instantiateModule(js, module);
-  return jsg::JsValue(js.v8Get(module->GetModuleNamespace().As<v8::Object>(), "default"_kj));
-}
-}  // namespace
-
 jsg::JsValue ServiceWorkerGlobalScope::getBuffer(jsg::Lock& js) {
   KJ_ASSERT(FeatureFlags::get(js).getNodeJsCompatV2());
-  auto value = resolveFromRegistry(js, "node:buffer"_kj);
-  auto obj = JSG_REQUIRE_NONNULL(
-      value.tryCast<jsg::JsObject>(), TypeError, "Invalid node:buffer implementation");
-  // Unlike the getProcess() case below, this getter is returning an object that
-  // is exported by the node:buffer module and not the module itself, so we need
-  // this additional get to the grab the reference to the thing we're actually
-  // returning.
-  auto buffer = obj.get(js, "Buffer"_kj);
-  JSG_REQUIRE(buffer.isFunction(), TypeError, "Invalid node:buffer implementation");
-  return buffer;
+  static const auto kSpecifier = "node:buffer"_kj;
+  KJ_IF_SOME(module, js.resolveModule(kSpecifier)) {
+    auto def = module.get(js, "default"_kj);
+    auto obj = KJ_ASSERT_NONNULL(def.tryCast<jsg::JsObject>());
+    auto buffer = obj.get(js, "Buffer"_kj);
+    JSG_REQUIRE(buffer.isFunction(), TypeError, "Invalid node:buffer implementation");
+    return buffer;
+  } else {
+    // If we are unable to resolve the node:buffer module here, it likely
+    // means that we don't actually have a module registry installed. Just
+    // return undefined in this case.
+    return js.undefined();
+  }
 }
 
 jsg::JsValue ServiceWorkerGlobalScope::getProcess(jsg::Lock& js) {
   KJ_ASSERT(FeatureFlags::get(js).getNodeJsCompatV2());
-  auto value = resolveFromRegistry(js, "node:process"_kj);
-  JSG_REQUIRE(value.isObject(), TypeError, "Invalid node:process implementation");
-  return value;
+  static const auto kSpecifier = "node:process"_kj;
+  KJ_IF_SOME(module, js.resolveModule(kSpecifier)) {
+    auto def = module.get(js, "default"_kj);
+    JSG_REQUIRE(def.isObject(), TypeError, "Invalid node:process implementation");
+    return def;
+  } else {
+    // If we are unable to resolve the node:process module here, it likely
+    // means that we don't actually have a module registry installed. Just
+    // return undefined in this case.
+    return js.undefined();
+  }
 }
 
 double Performance::now() {

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

@@ -16,6 +16,20 @@ strictEqual(import.meta.url, 'file:///worker');
 // Verify that import.meta.main is true here.
 ok(import.meta.main);
 
+// When running in nodejs_compat_v2 mode, the globalThis.Buffer
+// and globalThis.process properties are resolved from the module
+// registry. Let's make sure we get good values here.
+strictEqual(typeof globalThis.Buffer, 'function');
+strictEqual(globalThis.Buffer, Buffer);
+ok(globalThis.process);
+strictEqual(typeof globalThis.process, 'object');
+
+// Our internal implementation of console.log depend on the module registry
+// to get the node-internal:internal_inspect module. This console.log makes
+// sure that works correctly without crashing when using the new module
+// registry implementation.
+console.log(import.meta);
+
 // Verify that import.meta.resolve provides correct results here.
 // The input should be interpreted as a URL and normalized according
 // to the rules in the WHATWG URL specification.
@@ -46,6 +60,11 @@ strictEqual(Buffer.from(data.default).toString(), 'abcdef');
 import * as json from 'json';
 deepStrictEqual(json.default, { foo: 1 });
 
+// Synchronously resolved promises can be awaited.
+await Promise.resolve();
+
+// These dynamics imports can be top-level awaited because they
+// are immediately rejected with errors.
 await rejects(import('invalid-json'), {
   message: /Unexpected non-whitespace character after JSON/,
 });

src/workerd/io/worker.c++

@@ -799,6 +799,9 @@ struct Worker::Script::Impl {
   using DynamicImportHandler = kj::Function<jsg::Value()>;
 
   void configureDynamicImports(jsg::Lock& js, jsg::ModuleRegistry& modules) {
+    // This is only used with the original module registry implementation.
+    KJ_ASSERT(!FeatureFlags::get(js).getNewModuleRegistry(),
+        "legacy dynamic imports must not be used with the new module registry");
     static auto constexpr handleDynamicImport =
         [](kj::Own<const Worker> worker, DynamicImportHandler handler,
             kj::Maybe<jsg::Ref<jsg::AsyncContextFrame>> asyncContext)
@@ -1266,9 +1269,9 @@ Worker::Script::Script(kj::Own<const Isolate> isolateParam,
         context = mContext.getHandle(lock);
         recordedLock.setupContext(context);
       } else {
-        // Although we're going to compile a script independent of context, V8 requires that there be
-        // an active context, otherwise it will segfault, I guess. So we create a dummy context.
-        // (Undocumented, as usual.)
+        // Although we're going to compile a script independent of context, V8 requires that
+        // there be an active context, otherwise it will segfault, I guess. So we create a
+        // dummy context. (Undocumented, as usual.)
         context =
             v8::Context::New(lock.v8Isolate, nullptr, v8::ObjectTemplate::New(lock.v8Isolate));
         // We need to set the highest used index in every context we create to be a nullptr
@@ -1284,32 +1287,32 @@ Worker::Script::Script(kj::Own<const Isolate> isolateParam,
         if (logNewScript) {
           // HACK: Log a message indicating that a new script was loaded. This is used only when the
           //   inspector is enabled. We want to do this immediately after the context is created,
-          //   before the user gets a chance to modify the behavior of the console, which if they did,
-          //   we'd then need to be more careful to apply time limits and such.
+          //   before the user gets a chance to modify the behavior of the console, which if they
+          //   did, we'd then need to be more careful to apply time limits and such.
           lockedWorkerIsolate.logMessage(lock, static_cast<uint16_t>(cdp::LogType::WARNING),
               "Script modified; context reset.");
         }
 
-        // We need to register this context with the inspector, otherwise errors won't be reported. But
-        // we want it to be un-registered as soon as the script has been compiled, otherwise the
-        // inspector will end up with multiple contexts active which is very confusing for the user
-        // (since they'll have to select from the drop-down which context to use).
+        // We need to register this context with the inspector, otherwise errors won't be
+        // reported. But we want it to be un-registered as soon as the script has been
+        // compiled, otherwise the inspector will end up with multiple contexts active which
+        // is very confusing for the user (since they'll have to select from the drop-down
+        // which context to use).
         //
         // (For modules, the context was already registered by `setupContext()`, above.
         KJ_IF_SOME(i, isolate->impl->inspector) {
-          if (!source.is<ModulesSource>()) {
+          if (!modular) {
             i.get()->contextCreated(
                 v8_inspector::V8ContextInfo(context, 1, jsg::toInspectorStringView("Compiler")));
           }
         } else {
         }  // Here to squash a compiler warning
         KJ_DEFER({
-          if (!source.is<ModulesSource>()) {
+          if (!modular) {
             KJ_IF_SOME(i, isolate->impl->inspector) {
               i.get()->contextDestroyed(context);
             } else {
-              // Else block to avoid dangling else clang warning.
-            }
+            }  // Here to squash a compiler warning
           }
         });
 
@@ -1320,23 +1323,26 @@ Worker::Script::Script(kj::Own<const Isolate> isolateParam,
           try {
             KJ_SWITCH_ONEOF(source) {
               KJ_CASE_ONEOF(script, ScriptSource) {
+                // This path is used for the older, service worker syntax workers.
+
                 impl->globals =
                     script.compileGlobals(lock, isolate->getApi(), isolate->getApi().getObserver());
 
                 {
-                  // It's unclear to me if CompileUnboundScript() can get trapped in any infinite loops or
-                  // excessively-expensive computation requiring a time limit. We'll go ahead and apply a time
-                  // limit just to be safe. Don't add it to the rollover bank, though.
+                  // It's unclear to me if CompileUnboundScript() can get trapped in any
+                  // infinite loops or excessively-expensive computation requiring a time
+                  // limit. We'll go ahead and apply a time limit just to be safe. Don't
+                  // add it to the rollover bank, though.
                   auto limitScope =
                       isolate->getLimitEnforcer().enterStartupJs(lock, limitErrorOrTime);
                   impl->unboundScriptOrMainModule =
                       jsg::NonModuleScript::compile(lock, script.mainScript, script.mainScriptName);
                 }
-
-                break;
               }
 
               KJ_CASE_ONEOF(modulesSource, ModulesSource) {
+                // This path is used for the new ESM worker syntax.
+
                 this->isPython = modulesSource.isPython;
                 if (!isolate->getApi().getFeatureFlags().getNewModuleRegistry()) {
                   kj::Own<void> limitScope;
@@ -1351,7 +1357,6 @@ Worker::Script::Script(kj::Own<const Isolate> isolateParam,
                   modulesSource.compileModules(lock, isolate->getApi());
                 }
                 impl->unboundScriptOrMainModule = kj::Path::parse(modulesSource.mainModule);
-                break;
               }
             }
 
@@ -1477,21 +1482,6 @@ void Worker::setupContext(
 // =======================================================================================
 
 namespace {
-
-jsg::JsObject resolveNodeInspectModule(jsg::Lock& js) {
-  static constexpr auto kSpecifier = "node-internal:internal_inspect"_kj;
-  if (FeatureFlags::get(js).getNewModuleRegistry()) {
-    return KJ_ASSERT_NONNULL(
-        jsg::modules::ModuleRegistry::tryResolveModuleNamespace(js, kSpecifier));
-  }
-
-  // Use the original module registry implementation
-  auto registry = jsg::ModuleRegistry::from(js);
-  KJ_ASSERT(registry != nullptr);
-  auto inspectModule = registry->resolveInternalImport(js, kSpecifier);
-  return jsg::JsObject(inspectModule.getHandle(js).As<v8::Object>());
-}
-
 kj::Maybe<jsg::JsObject> tryResolveMainModule(jsg::Lock& js,
     const kj::Path& mainModule,
     jsg::JsContext<api::ServiceWorkerGlobalScope>& jsContext,
@@ -1867,7 +1857,8 @@ void Worker::handleLog(jsg::Lock& js,
     // not even evaluate its arguments, so `message()` will not be called at all.
     KJ_LOG(INFO, "console.log()", message());
   } else {
-    // Write to stdio if allowed by console mode
+    // Write to stdio if allowed by console mode. This is making use of our internal
+    // built-in implementation of the node:util inspect API.
     static const ColorMode COLOR_MODE = permitsColor();
 #if _WIN32
     static bool STDOUT_TTY = _isatty(_fileno(stdout));
@@ -1884,7 +1875,8 @@ void Worker::handleLog(jsg::Lock& js,
     auto colors =
         COLOR_MODE == ColorMode::ENABLED || (COLOR_MODE == ColorMode::ENABLED_IF_TTY && tty);
 
-    auto inspectModule = resolveNodeInspectModule(js);
+    static constexpr auto kSpecifier = "node-internal:internal_inspect"_kj;
+    auto inspectModule = KJ_ASSERT_NONNULL(js.resolveInternalModule(kSpecifier));
     v8::Local<v8::Value> formatLogVal = inspectModule.get(js, "formatLog"_kj);
     KJ_ASSERT(formatLogVal->IsFunction());
     auto formatLog = formatLogVal.As<v8::Function>();

src/workerd/jsg/BUILD.bazel

@@ -8,12 +8,9 @@ exports_files(["modules.capnp"])
 
 wd_cc_library(
     name = "jsg",
-    srcs = [
-        "modules-new.c++",
-    ],
+    srcs = [],
     hdrs = [
         "jsg-test.h",
-        "modules-new.h",
         "type-wrapper.h",
     ],
     local_defines = ["JSG_IMPLEMENTATION"],
@@ -86,6 +83,7 @@ wd_cc_library(
         "jsg.c++",
         "jsvalue.c++",
         "modules.c++",
+        "modules-new.c++",
         "promise.c++",
         "resource.c++",
         "ser.c++",
@@ -103,6 +101,7 @@ wd_cc_library(
         "jsg.h",
         "jsvalue.h",
         "modules.h",
+        "modules-new.h",
         "promise.h",
         "resource.h",
         "ser.h",
@@ -121,6 +120,7 @@ wd_cc_library(
         ":meta",
         ":modules_capnp",
         ":observer",
+        ":url",
         "//src/workerd/util",
         "//src/workerd/util:sentry",
         "//src/workerd/util:thread-scopes",

src/workerd/jsg/jsg.c++

@@ -6,6 +6,8 @@
 
 #include "setup.h"
 
+#include <workerd/jsg/modules-new.h>
+#include <workerd/jsg/modules.h>
 #include <workerd/jsg/util.h>
 #include <workerd/util/thread-scopes.h>
 
@@ -289,6 +291,37 @@ JsSymbol Lock::symbolAsyncDispose() {
   return IsolateBase::from(v8Isolate).getSymbolAsyncDispose();
 }
 
+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_ONLY);
+  }
+
+  // Use the original module registry implementation
+  auto registry = ModuleRegistry::from(*this);
+  KJ_ASSERT(registry != nullptr);
+  auto module = registry->resolveInternalImport(*this, specifier);
+  return jsg::JsObject(module.getHandle(*this).As<v8::Object>());
+}
+
+kj::Maybe<JsObject> Lock::resolveModule(kj::StringPtr specifier) {
+  auto& isolate = IsolateBase::from(v8Isolate);
+  if (isolate.isUsingNewModuleRegistry()) {
+    return jsg::modules::ModuleRegistry::tryResolveModuleNamespace(
+        *this, specifier, jsg::modules::ResolveContext::Type::BUNDLE);
+  }
+
+  auto moduleRegistry = jsg::ModuleRegistry::from(*this);
+  if (moduleRegistry == nullptr) return kj::none;
+  auto spec = kj::Path::parse(specifier);
+  auto& info = JSG_REQUIRE_NONNULL(
+      moduleRegistry->resolve(*this, spec), Error, kj::str("No such module: ", specifier));
+  auto module = info.module.getHandle(*this);
+  jsg::instantiateModule(*this, module);
+  return JsObject(module->GetModuleNamespace().As<v8::Object>());
+}
+
 void ExternalMemoryAdjustment::maybeDeferAdjustment(
     v8::ExternalMemoryAccounter& externalMemoryAccounter, v8::Isolate* isolate, size_t amount) {
   if (isolate == nullptr) return;

src/workerd/jsg/jsg.h

@@ -2718,6 +2718,14 @@ class Lock {
   // Retrieve the worker environment.
   virtual kj::Maybe<Value> getWorkerEnv() = 0;
 
+  // Resolve an internalk module namespace from the given specifier.
+  // This variation can be used only for internal built-ins.
+  kj::Maybe<JsObject> resolveInternalModule(kj::StringPtr specifier);
+
+  // Resolve a module namespace from the given specifier.
+  // This variation includes modules from the worker bundle.
+  kj::Maybe<JsObject> resolveModule(kj::StringPtr specifier);
+
  private:
   // Mark the jsg::Lock as being disallowed from being passed as a parameter into
   // a kj promise coroutine. Note that this only blocks directly passing the Lock