Merge pull request #2546 from cloudflare/jsnell/moar-new-module-regis…

…try-fun

src/workerd/io/worker.c++

@@ -24,6 +24,7 @@
 #include <workerd/jsg/util.h>
 #include <workerd/io/cdp.capnp.h>
 #include <workerd/io/compatibility-date.h>
+#include <workerd/io/features.h>
 #include <capnp/message.h>
 #include <capnp/compat/json.h>
 #include <kj/compat/gzip.h>
@@ -1361,6 +1362,21 @@ void setWebAssemblyModuleHasInstance(jsg::Lock& lock, v8::Local<v8::Context> con
 // =======================================================================================
 
 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,
@@ -1590,8 +1606,7 @@ Worker::Worker(kj::Own<const Script> scriptParam,
                 }
               }
             } else {
-            }  // Added to suppress a compiler warning
-
+            }
             startupMetrics->done();
           } catch (const kj::Exception& e) {
             lock.throwException(kj::cp(e));
@@ -1742,15 +1757,14 @@ void Worker::handleLog(jsg::Lock& js,
     auto colors =
         COLOR_MODE == ColorMode::ENABLED || (COLOR_MODE == ColorMode::ENABLED_IF_TTY && tty);
 
-    auto registry = jsg::ModuleRegistry::from(js);
-    auto inspectModule = registry->resolveInternalImport(js, "node-internal:internal_inspect"_kj);
-    auto inspectModuleHandle = inspectModule.getHandle(js).As<v8::Object>();
-    auto formatLog = js.v8Get(inspectModuleHandle, "formatLog"_kj).As<v8::Function>();
+    auto inspectModule = resolveNodeInspectModule(js);
+    v8::Local<v8::Value> formatLogVal = inspectModule.get(js, "formatLog"_kj);
+    KJ_ASSERT(formatLogVal->IsFunction());
+    auto formatLog = formatLogVal.As<v8::Function>();
 
-    auto recv = js.v8Undefined();
     args[length] = v8::Boolean::New(js.v8Isolate, colors);
-    auto formatted =
-        js.toString(jsg::check(formatLog->Call(context, recv, length + 1, args.data())));
+    auto formatted = js.toString(
+        jsg::check(formatLog->Call(context, js.v8Undefined(), length + 1, args.data())));
     fprintf(fd, "%s\n", formatted.cStr());
     fflush(fd);
   }

src/workerd/jsg/modules-new-test.c++

@@ -1129,7 +1129,7 @@ KJ_TEST("Previously resolved modules not found with incompatible resolve context
 
 // ======================================================================================
 
-KJ_TEST("Awaiting top-level dynamic import in synchronous require fails as expected") {
+KJ_TEST("Awaiting top-level dynamic import in synchronous require works as expected") {
   PREAMBLE([&](Lock& js) {
     ResolveObserverImpl observer;
     CompilationObserver compilationObserver;
@@ -1145,17 +1145,7 @@ KJ_TEST("Awaiting top-level dynamic import in synchronous require fails as expec
 
     auto attached = registry->attachToIsolate(js, compilationObserver);
 
-    js.tryCatch([&] {
-      // This is a synchronous resolve that should fail.
-      ModuleRegistry::resolve(js, "file:///foo", "default"_kjc);
-      KJ_FAIL_ASSERT("Should have failed");
-    }, [&](Value exception) {
-      auto str = kj::str(exception.getHandle(js));
-      KJ_ASSERT(str ==
-          "Error: The module evaluation did not complete synchronously. "
-          "This is not permitted for synchronous require(...). "
-          "Use await import(...) instead.");
-    });
+    ModuleRegistry::resolve(js, "file:///foo", "default"_kjc);
   });
 }
 
@@ -1373,10 +1363,8 @@ KJ_TEST("Recursively require ESM from CJS required from ESM fails as expected (d
       JSG_FAIL_REQUIRE(Error, "Should have failed");
     }, [&](Value exception) {
       auto str = kj::str(exception.getHandle(js));
-      KJ_ASSERT(str ==
-          "Error: The module evaluation did not complete synchronously. "
-          "This is not permitted for synchronous require(...). "
-          "Use await import(...) instead.");
+      KJ_ASSERT(
+          str == "TypeError: Circular module dependency with synchronous require: file:///bar");
     });
   });
 }
@@ -1816,40 +1804,28 @@ KJ_TEST("Aliased modules (import maps) work") {
 
 // ======================================================================================
 
-// TODO(soon): Temporarily disabling this test. TC-39 recently changed the syntax
-// for import attributes to move away from the "assert". Prior to v8 12.6 the
-// assert keyword was still supporteed, with 12.6 it is not. Since we currently
-// update the v8 version in the internal repo separately than we do the public
-// workerd repo, there's a small period of time in which the versions will be out
-// of sync. If we modify this test to work for 12.6, it will fail will 12.5. So since
-// the test is not yet critical, we'll disable it for now.
-//
-// If/when we decide to support import attributes, this test will need to be updated.
-// KJ_TEST("Import attributes are currently unsupported") {
-//   ResolveObserver resolveObserver;
-//   CompilationObserver compilationObserver;
-//   ModuleBundle::BundleBuilder builder;
-
-//   // TODO(soon): The import attribute spec has been updated to replace the "assert"
-//   // with the "with" keyword. V8 has not yet picked up this change. Once it does,
-//   // this test will likely need to be changed.
-//   auto foo = kj::str("import abc from 'foo' assert { type: 'json' };");
-//   builder.addEsmModule("foo", foo.slice(0, foo.size()).attach(kj::mv(foo)));
-
-//   auto registry = ModuleRegistry::Builder(resolveObserver).add(builder.finish()).finish();
-
-//   PREAMBLE([&](Lock& js) {
-//     auto attached = registry->attachToIsolate(js, compilationObserver);
-
-//     js.tryCatch([&] {
-//       ModuleRegistry::resolve(js, "foo", "default"_kjc);
-//       JSG_FAIL_REQUIRE(Error, "Should have thrown");
-//     }, [&](Value exception) {
-//       auto str = kj::str(exception.getHandle(js));
-//       KJ_ASSERT(str == "TypeError: Import attributes are not supported");
-//     });
-//   });
-// }
+KJ_TEST("Import attributes are currently unsupported") {
+  ResolveObserver resolveObserver;
+  CompilationObserver compilationObserver;
+  ModuleBundle::BundleBuilder builder;
+
+  auto foo = kj::str("import abc from 'foo' with { type: 'json' };");
+  builder.addEsmModule("foo", foo.slice(0, foo.size()).attach(kj::mv(foo)));
+
+  auto registry = ModuleRegistry::Builder(resolveObserver).add(builder.finish()).finish();
+
+  PREAMBLE([&](Lock& js) {
+    auto attached = registry->attachToIsolate(js, compilationObserver);
+
+    js.tryCatch([&] {
+      ModuleRegistry::resolve(js, "foo", "default"_kjc);
+      JSG_FAIL_REQUIRE(Error, "Should have thrown");
+    }, [&](Value exception) {
+      auto str = kj::str(exception.getHandle(js));
+      KJ_ASSERT(str == "TypeError: Import attributes are not supported");
+    });
+  });
+}
 
 // ======================================================================================
 KJ_TEST("Using a deferred eval callback works") {

src/workerd/jsg/modules-new.c++

@@ -392,6 +392,7 @@ public:
       // Evaluate the module and grab the default export from the module namespace.
       auto promise =
           check(entry.module.evaluate(js, module, observer, maybeEvalCallback)).As<v8::Promise>();
+      js.runMicrotasks();
 
       switch (promise->State()) {
         case v8::Promise::kFulfilled: {

src/workerd/server/workerd-api.c++

@@ -882,6 +882,44 @@ kj::Own<jsg::modules::ModuleRegistry> WorkerdApi::initializeBundleModuleRegistry
     const PythonConfig& pythonConfig) {
   jsg::modules::ModuleRegistry::Builder builder(
       observer, jsg::modules::ModuleRegistry::Builder::Options::ALLOW_FALLBACK);
+  builder.setEvalCallback([](jsg::Lock& js, const auto& module, v8::Local<v8::Module> v8Module,
+                              const auto& observer) -> jsg::Promise<jsg::Value> {
+    static constexpr auto handleDynamicImport =
+        [](kj::Own<const Worker> worker, const auto& module, jsg::V8Ref<v8::Module> v8Module,
+            const auto& observer, kj::Maybe<jsg::Ref<jsg::AsyncContextFrame>> asyncContext)
+        -> kj::Promise<jsg::Promise<jsg::Value>> {
+      co_await kj::yield();
+      KJ_ASSERT(!IoContext::hasCurrent());
+      auto asyncLock = co_await worker->takeAsyncLockWithoutRequest(nullptr);
+
+      co_return worker->runInLockScope(asyncLock, [&](Worker::Lock& lock) {
+        return JSG_WITHIN_CONTEXT_SCOPE(lock, lock.getContext(), [&](jsg::Lock& js) {
+          jsg::AsyncContextFrame::Scope asyncContextScope(js, asyncContext);
+          return js.tryCatch([&] {
+            return js.toPromise(jsg::check(v8Module.getHandle(js)->Evaluate(js.v8Context())));
+          }, [&](jsg::Value&& exception) {
+            return js.rejectedPromise<jsg::Value>(kj::mv(exception));
+          });
+        });
+      });
+    };
+
+    // If there is an active IoContext, then we want to defer evaluation of the
+    // module to escape the current IoContext.
+    if (IoContext::hasCurrent()) {
+      auto& context = IoContext::current();
+      return context.awaitIo(js,
+          handleDynamicImport(kj::atomicAddRef(context.getWorker()), module, js.v8Ref(v8Module),
+              observer, jsg::AsyncContextFrame::currentRef(js)),
+          [](jsg::Lock& js, jsg::Promise<jsg::Value>&& result) { return kj::mv(result); });
+    }
+
+    // If there is no active IoContext at this point, then we can evaluate the module
+    // immediately.
+    return js.tryCatch([&]() -> jsg::Promise<jsg::Value> {
+      return js.toPromise(jsg::check(v8Module->Evaluate(js.v8Context())));
+    }, [&](jsg::Value&& exception) { return js.rejectedPromise<jsg::Value>(kj::mv(exception)); });
+  });
 
   api::registerBuiltinModules<JsgWorkerdIsolate_TypeWrapper>(builder, featureFlags);