From 3a54618cee8c23acd8e58ddf76f7b1985cc69b46 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 8 Nov 2024 06:00:20 -0800 Subject: [PATCH 1/3] Disable top-level await in require with a compat flag --- src/workerd/api/global-scope.c++ | 9 +- src/workerd/api/node/module.c++ | 26 +++-- .../node/tests/module-create-require-test.js | 9 +- .../tests/module-create-require-test.wd-test | 6 +- src/workerd/api/node/util.c++ | 7 -- .../api/tests/new-module-registry-test.js | 2 +- src/workerd/io/compatibility-date.capnp | 8 ++ src/workerd/io/worker.c++ | 3 + src/workerd/jsg/jsg.c++ | 4 + src/workerd/jsg/jsg.h | 1 + src/workerd/jsg/modules.c++ | 107 ++++++++++++------ src/workerd/jsg/modules.h | 9 +- src/workerd/jsg/setup.h | 9 ++ 13 files changed, 132 insertions(+), 68 deletions(-) diff --git a/src/workerd/api/global-scope.c++ b/src/workerd/api/global-scope.c++ index 44ca2649cce..e3072b957cd 100644 --- a/src/workerd/api/global-scope.c++ +++ b/src/workerd/api/global-scope.c++ @@ -849,15 +849,8 @@ jsg::JsValue resolveFromRegistry(jsg::Lock& js, kj::StringPtr 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); - auto handle = jsg::check(module->Evaluate(js.v8Context())); - KJ_ASSERT(handle->IsPromise()); - auto prom = handle.As(); - KJ_ASSERT(prom->State() != v8::Promise::PromiseState::kPending); - if (module->GetStatus() == v8::Module::kErrored) { - jsg::throwTunneledException(js.v8Isolate, module->GetException()); - } + jsg::instantiateModule(js, module); return jsg::JsValue(js.v8Get(module->GetModuleNamespace().As(), "default"_kj)); } } // namespace diff --git a/src/workerd/api/node/module.c++ b/src/workerd/api/node/module.c++ index 16c4fbfd697..6f2482d9599 100644 --- a/src/workerd/api/node/module.c++ +++ b/src/workerd/api/node/module.c++ @@ -3,6 +3,7 @@ // https://opensource.org/licenses/Apache-2.0 #include "module.h" +#include #include namespace workerd::api::node { @@ -84,19 +85,22 @@ jsg::JsValue ModuleUtil::createRequire(jsg::Lock& js, kj::String path) { auto module = info.module.getHandle(js); - jsg::instantiateModule(js, module); - auto handle = jsg::check(module->Evaluate(js.v8Context())); - KJ_ASSERT(handle->IsPromise()); - auto prom = handle.As(); - if (prom->State() == v8::Promise::PromiseState::kPending) { - js.runMicrotasks(); - } - JSG_REQUIRE(prom->State() != v8::Promise::PromiseState::kPending, Error, - "Module evaluation did not complete synchronously."); - if (module->GetStatus() == v8::Module::kErrored) { - jsg::throwTunneledException(js.v8Isolate, module->GetException()); + auto features = FeatureFlags::get(js); + jsg::InstantiateModuleOptions options = jsg::InstantiateModuleOptions::DEFAULT; + if (features.getNoTopLevelAwaitInRequire()) { + options = jsg::InstantiateModuleOptions::NO_TOP_LEVEL_AWAIT; + + // If the module was already evaluated, let's check if it is async. + // If it is, we will throw an error. This case can happen if a previous + // attempt to require the module failed because the module was async. + if (module->GetStatus() == v8::Module::kEvaluated) { + JSG_REQUIRE(!module->IsGraphAsync(), Error, + "Top-level await in module is not permitted at this time."); + } } + jsg::instantiateModule(js, module, options); + if (isEsm) { // If the import is an esm module, we will return the namespace object. jsg::JsObject obj(module->GetModuleNamespace().As()); diff --git a/src/workerd/api/node/tests/module-create-require-test.js b/src/workerd/api/node/tests/module-create-require-test.js index d2579495150..724a48465c0 100644 --- a/src/workerd/api/node/tests/module-create-require-test.js +++ b/src/workerd/api/node/tests/module-create-require-test.js @@ -24,7 +24,14 @@ export const doTheTest = { strictEqual(assert, required); throws(() => require('invalid'), { - message: 'Module evaluation did not complete synchronously.', + message: 'Top-level await in module is not permitted at this time.', + }); + // Trying to require the module again should throw the same error. + throws(() => require('invalid'), { + message: 'Top-level await in module is not permitted at this time.', + }); + throws(() => require('invalid2'), { + message: 'Top-level await in module is not permitted at this time.', }); throws(() => require('does not exist')); diff --git a/src/workerd/api/node/tests/module-create-require-test.wd-test b/src/workerd/api/node/tests/module-create-require-test.wd-test index cc97fcb5074..c550ff58d5e 100644 --- a/src/workerd/api/node/tests/module-create-require-test.wd-test +++ b/src/workerd/api/node/tests/module-create-require-test.wd-test @@ -10,10 +10,12 @@ const unitTests :Workerd.Config = ( (name = "bar", esModule = "export default 2; export const __cjsUnwrapDefault = true;"), (name = "baz", commonJsModule = "module.exports = 3;"), (name = "worker/qux", text = "4"), - (name = "invalid", esModule = "const p = new Promise(() => {}); await p;"), + (name = "invalid", esModule = "await new Promise(() => {});"), + (name = "invalid2", commonJsModule = "require('invalid3');"), + (name = "invalid3", esModule = "await new Promise(() => {});"), ], compatibilityDate = "2024-08-01", - compatibilityFlags = ["nodejs_compat_v2"] + compatibilityFlags = ["disable_top_level_await_in_require", "nodejs_compat_v2"] ) ), ], diff --git a/src/workerd/api/node/util.c++ b/src/workerd/api/node/util.c++ index 5fe83665f23..0b526cb2bc4 100644 --- a/src/workerd/api/node/util.c++ +++ b/src/workerd/api/node/util.c++ @@ -217,13 +217,6 @@ jsg::JsValue UtilModule::getBuiltinModule(jsg::Lock& js, kj::String specifier) { jsg::ModuleRegistry::ResolveMethod::IMPORT, rawSpecifier.asPtr())) { auto module = info.module.getHandle(js); jsg::instantiateModule(js, module); - auto handle = jsg::check(module->Evaluate(js.v8Context())); - KJ_ASSERT(handle->IsPromise()); - auto prom = handle.As(); - KJ_ASSERT(prom->State() != v8::Promise::PromiseState::kPending); - if (module->GetStatus() == v8::Module::kErrored) { - jsg::throwTunneledException(js.v8Isolate, module->GetException()); - } // For Node.js modules, we want to grab the default export and return that. // For other built-ins, we'll return the module namespace instead. Can be diff --git a/src/workerd/api/tests/new-module-registry-test.js b/src/workerd/api/tests/new-module-registry-test.js index d271a41b6e4..6013b7885fb 100644 --- a/src/workerd/api/tests/new-module-registry-test.js +++ b/src/workerd/api/tests/new-module-registry-test.js @@ -8,7 +8,7 @@ import { import { foo, default as def } from 'foo'; import { default as fs } from 'node:fs'; import { Buffer } from 'buffer'; -const { foo: foo2, default: def2 } = await import('bar'); +import { foo as foo2, default as def2 } from 'bar'; // Verify that import.meta.url is correct here. strictEqual(import.meta.url, 'file:///worker'); diff --git a/src/workerd/io/compatibility-date.capnp b/src/workerd/io/compatibility-date.capnp index acc404c8f49..a7d16d4ad57 100644 --- a/src/workerd/io/compatibility-date.capnp +++ b/src/workerd/io/compatibility-date.capnp @@ -657,4 +657,12 @@ struct CompatibilityFlags @0x8f8c1b68151b6cef { # # This is a compat flag so that we can opt in our test workers into it before rolling out to # everyone. + + noTopLevelAwaitInRequire @67 :Bool + $compatEnableFlag("disable_top_level_await_in_require") + $compatDisableFlag("enable_top_level_await_in_require") + $compatEnableDate("2024-12-02"); + # When enabled, use of top-level await syntax in require() calls will be disallowed. + # The ecosystem and runtimes are moving to a state where top level await in modules + # is being strongly discouraged. } diff --git a/src/workerd/io/worker.c++ b/src/workerd/io/worker.c++ index b4434763e2a..26db2700414 100644 --- a/src/workerd/io/worker.c++ +++ b/src/workerd/io/worker.c++ @@ -999,6 +999,9 @@ Worker::Isolate::Isolate(kj::Own apiParam, if (features.getNodeJsCompatV2()) { lock->setNodeJsCompatEnabled(); } + if (features.getNoTopLevelAwaitInRequire()) { + lock->disableTopLevelAwait(); + } if (impl->inspector != kj::none || ::kj::_::Debug::shouldLog(::kj::LogSeverity::INFO)) { lock->setLoggerCallback([this](jsg::Lock& js, kj::StringPtr message) { diff --git a/src/workerd/jsg/jsg.c++ b/src/workerd/jsg/jsg.c++ index 05e346f2af0..5c625794a77 100644 --- a/src/workerd/jsg/jsg.c++ +++ b/src/workerd/jsg/jsg.c++ @@ -195,6 +195,10 @@ void Lock::setNodeJsCompatEnabled() { IsolateBase::from(v8Isolate).setNodeJsCompatEnabled({}, true); } +void Lock::disableTopLevelAwait() { + IsolateBase::from(v8Isolate).disableTopLevelAwait(); +} + void Lock::setToStringTag() { IsolateBase::from(v8Isolate).enableSetToStringTag(); } diff --git a/src/workerd/jsg/jsg.h b/src/workerd/jsg/jsg.h index 3190cdfd966..4945a950b6e 100644 --- a/src/workerd/jsg/jsg.h +++ b/src/workerd/jsg/jsg.h @@ -2516,6 +2516,7 @@ class Lock { void setNodeJsCompatEnabled(); void setToStringTag(); + void disableTopLevelAwait(); using Logger = void(Lock&, kj::StringPtr); void setLoggerCallback(kj::Function&& logger); diff --git a/src/workerd/jsg/modules.c++ b/src/workerd/jsg/modules.c++ index 4107313bb88..0131b16e6b2 100644 --- a/src/workerd/jsg/modules.c++ +++ b/src/workerd/jsg/modules.c++ @@ -272,24 +272,32 @@ v8::Local CommonJsModuleContext::require(jsg::Lock& js, kj::String sp // Adding imported from suffix here not necessary like it is for resolveCallback, since we have a // js stack that will include the parent module's name and location of the failed require(). - JSG_REQUIRE_NONNULL( - info.maybeSynthetic, TypeError, "Cannot use require() to import an ES Module."); - auto module = info.module.getHandle(js); - check(module->InstantiateModule(js.v8Context(), &resolveCallback)); - auto handle = check(module->Evaluate(js.v8Context())); - KJ_ASSERT(handle->IsPromise()); - auto prom = handle.As(); - - // This assert should always pass since evaluateSyntheticModuleCallback() for CommonJS - // modules (below) always returns an already-resolved promise. - KJ_ASSERT(prom->State() != v8::Promise::PromiseState::kPending); + JSG_REQUIRE(module->GetStatus() != v8::Module::Status::kEvaluating && + module->GetStatus() != v8::Module::Status::kInstantiating, + Error, + "Module cannot be synchronously required while it is being instantiated or evaluated. " + "This error typically means that a CommonJS or NodeJS-Compat type module has a circular " + "dependency on itself, and that a synchronous require() is being called while the module " + "is being loaded."); - if (module->GetStatus() == v8::Module::kErrored) { - throwTunneledException(js.v8Isolate, module->GetException()); + auto& isolateBase = IsolateBase::from(js.v8Isolate); + jsg::InstantiateModuleOptions options = jsg::InstantiateModuleOptions::DEFAULT; + if (!isolateBase.isTopLevelAwaitEnabled()) { + options = jsg::InstantiateModuleOptions::NO_TOP_LEVEL_AWAIT; + + // If the module was already evaluated, let's check if it is async. + // If it is, we will throw an error. This case can happen if a previous + // attempt to require the module failed because the module was async. + if (module->GetStatus() == v8::Module::kEvaluated) { + JSG_REQUIRE(!module->IsGraphAsync(), Error, + "Top-level await in module is not permitted at this time."); + } } + jsg::instantiateModule(js, module, options); + // Originally, This returned an object like `{default: module.exports}` when we really // intended to return the module exports raw. We should be extracting `default` here. // Unfortunately, there is a user depending on the wrong behavior in production, so we @@ -322,32 +330,47 @@ NonModuleScript NonModuleScript::compile(kj::StringPtr code, jsg::Lock& js, kj:: return NonModuleScript(js, check(v8::ScriptCompiler::CompileUnboundScript(isolate, &source))); } -void instantiateModule(jsg::Lock& js, v8::Local& module) { +void instantiateModule( + jsg::Lock& js, v8::Local& module, InstantiateModuleOptions options) { KJ_ASSERT(!module.IsEmpty()); auto isolate = js.v8Isolate; auto context = js.v8Context(); auto status = module->GetStatus(); - // Nothing to do if the module is already instantiated, evaluated, or errored. - if (status == v8::Module::Status::kInstantiated || status == v8::Module::Status::kEvaluated || - status == v8::Module::Status::kErrored) - return; - JSG_REQUIRE(status == v8::Module::Status::kUninstantiated, Error, - "Module cannot be synchronously required while it is being instantiated or evaluated. " - "This error typically means that a CommonJS or NodeJS-Compat type module has a circular " - "dependency on itself, and that a synchronous require() is being called while the module " - "is being loaded."); + // If the previous instantiation failed, throw the exception. + if (status == v8::Module::Status::kErrored) { + isolate->ThrowException(module->GetException()); + throw jsg::JsExceptionThrown(); + } + + // Nothing to do if the module is already instantiated, evaluated. + if (status == v8::Module::Status::kEvaluated || status == v8::Module::Status::kEvaluating) return; + + if (status == v8::Module::Status::kUninstantiated) { + jsg::check(module->InstantiateModule(context, &resolveCallback)); + } - jsg::check(module->InstantiateModule(context, &resolveCallback)); auto prom = jsg::check(module->Evaluate(context)).As(); + + if (module->IsGraphAsync() && prom->State() == v8::Promise::kPending) { + // Pump the microtasks if there's an unsettled top-level await in the module. + // Because we do not support i/o in this scope, this *should* resolve in a + // single drain of the microtask queue (tho it's possible that it'll take + // multiple tasks). When the runMicrotasks() is complete, the promise should + // be settled. + JSG_REQUIRE(options != InstantiateModuleOptions::NO_TOP_LEVEL_AWAIT, Error, + "Top-level await in module is not permitted at this time."); + } + // We run microtasks to ensure that any promises that happen to be scheduled + // during the evaluation of the top level scope have a chance to be settled, + // even if those are not directly awaited. js.runMicrotasks(); switch (prom->State()) { case v8::Promise::kPending: // Let's make sure nobody is depending on pending modules that do not resolve first. - KJ_LOG(WARNING, "Async module was not immediately resolved."); - break; + JSG_FAIL_REQUIRE(Error, "Top-level await in module is unsettled."); case v8::Promise::kRejected: // Since we don't actually support I/O when instantiating a worker, we don't return the // promise from module->Evaluate, which means we lose any errors that happen during @@ -594,20 +617,30 @@ v8::Local NodeJsModuleContext::require(jsg::Lock& js, kj::String spec auto module = info.module.getHandle(js); - jsg::instantiateModule(js, module); - - auto handle = jsg::check(module->Evaluate(js.v8Context())); - KJ_ASSERT(handle->IsPromise()); - auto prom = handle.As(); - - // This assert should always pass since evaluateSyntheticModuleCallback() for CommonJS - // modules (below) always returns an already-resolved promise. - KJ_ASSERT(prom->State() != v8::Promise::PromiseState::kPending); + JSG_REQUIRE(module->GetStatus() != v8::Module::Status::kEvaluating && + module->GetStatus() != v8::Module::Status::kInstantiating, + Error, + "Module cannot be synchronously required while it is being instantiated or evaluated. " + "This error typically means that a CommonJS or NodeJS-Compat type module has a circular " + "dependency on itself, and that a synchronous require() is being called while the module " + "is being loaded."); - if (module->GetStatus() == v8::Module::kErrored) { - jsg::throwTunneledException(js.v8Isolate, module->GetException()); + auto& isolateBase = IsolateBase::from(js.v8Isolate); + jsg::InstantiateModuleOptions options = jsg::InstantiateModuleOptions::DEFAULT; + if (!isolateBase.isTopLevelAwaitEnabled()) { + options = jsg::InstantiateModuleOptions::NO_TOP_LEVEL_AWAIT; + + // If the module was already evaluated, let's check if it is async. + // If it is, we will throw an error. This case can happen if a previous + // attempt to require the module failed because the module was async. + if (module->GetStatus() == v8::Module::kEvaluated) { + JSG_REQUIRE(!module->IsGraphAsync(), Error, + "Top-level await in module is not permitted at this time."); + } } + jsg::instantiateModule(js, module, options); + return js.v8Get(module->GetModuleNamespace().As(), "default"_kj); } diff --git a/src/workerd/jsg/modules.h b/src/workerd/jsg/modules.h index 0b68aca2726..b418582b8ef 100644 --- a/src/workerd/jsg/modules.h +++ b/src/workerd/jsg/modules.h @@ -191,7 +191,14 @@ class NonModuleScript { v8::Global unboundScript; }; -void instantiateModule(jsg::Lock& js, v8::Local& module); +enum class InstantiateModuleOptions { + DEFAULT, + NO_TOP_LEVEL_AWAIT, +}; + +void instantiateModule(jsg::Lock& js, + v8::Local& module, + InstantiateModuleOptions options = InstantiateModuleOptions::DEFAULT); enum class ModuleInfoCompileOption { // The BUNDLE options tells the compile operation to treat the content as coming diff --git a/src/workerd/jsg/setup.h b/src/workerd/jsg/setup.h index c3ad32cb5d4..0586a52508a 100644 --- a/src/workerd/jsg/setup.h +++ b/src/workerd/jsg/setup.h @@ -151,6 +151,14 @@ class IsolateBase { setToStringTag = true; } + inline void disableTopLevelAwait() { + allowTopLevelAwait = false; + } + + inline bool isTopLevelAwaitEnabled() const { + return allowTopLevelAwait; + } + // The logger will be optionally set by the isolate setup logic if there is anywhere // for the log to go (for instance, if debug logging is enabled or the inspector is // being used). @@ -238,6 +246,7 @@ class IsolateBase { bool asyncContextTrackingEnabled = false; bool nodeJsCompatEnabled = false; bool setToStringTag = false; + bool allowTopLevelAwait = true; kj::Maybe> maybeLogger; kj::Maybe> maybeErrorReporter; From 00667dedb76512b4b1a433148b121fe73508a2cf Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 10 Nov 2024 06:05:47 -0800 Subject: [PATCH 2/3] Allow for circular commonjs dependencies --- src/workerd/api/node/module.c++ | 12 ++++++++++++ src/workerd/jsg/modules.c++ | 30 +++++++++++++++--------------- src/workerd/jsg/modules.h | 8 +++++--- 3 files changed, 32 insertions(+), 18 deletions(-) diff --git a/src/workerd/api/node/module.c++ b/src/workerd/api/node/module.c++ index 6f2482d9599..dd9d247119c 100644 --- a/src/workerd/api/node/module.c++ +++ b/src/workerd/api/node/module.c++ @@ -85,6 +85,18 @@ jsg::JsValue ModuleUtil::createRequire(jsg::Lock& js, kj::String path) { auto module = info.module.getHandle(js); + if (module->GetStatus() == v8::Module::Status::kEvaluating || + module->GetStatus() == v8::Module::Status::kInstantiating) { + KJ_IF_SOME(synth, info.maybeSynthetic) { + KJ_IF_SOME(cjs, synth.tryGet()) { + return cjs.moduleContext->getExports(js); + } + KJ_IF_SOME(cjs, synth.tryGet()) { + return cjs.moduleContext->getExports(js); + } + } + } + auto features = FeatureFlags::get(js); jsg::InstantiateModuleOptions options = jsg::InstantiateModuleOptions::DEFAULT; if (features.getNoTopLevelAwaitInRequire()) { diff --git a/src/workerd/jsg/modules.c++ b/src/workerd/jsg/modules.c++ index 0131b16e6b2..8cbd143bf07 100644 --- a/src/workerd/jsg/modules.c++ +++ b/src/workerd/jsg/modules.c++ @@ -274,13 +274,17 @@ v8::Local CommonJsModuleContext::require(jsg::Lock& js, kj::String sp auto module = info.module.getHandle(js); - JSG_REQUIRE(module->GetStatus() != v8::Module::Status::kEvaluating && - module->GetStatus() != v8::Module::Status::kInstantiating, - Error, - "Module cannot be synchronously required while it is being instantiated or evaluated. " - "This error typically means that a CommonJS or NodeJS-Compat type module has a circular " - "dependency on itself, and that a synchronous require() is being called while the module " - "is being loaded."); + if (module->GetStatus() == v8::Module::Status::kEvaluating || + module->GetStatus() == v8::Module::Status::kInstantiating) { + KJ_IF_SOME(synth, info.maybeSynthetic) { + KJ_IF_SOME(cjs, synth.tryGet()) { + return cjs.moduleContext->getExports(js); + } + KJ_IF_SOME(cjs, synth.tryGet()) { + return cjs.moduleContext->getExports(js); + } + } + } auto& isolateBase = IsolateBase::from(js.v8Isolate); jsg::InstantiateModuleOptions options = jsg::InstantiateModuleOptions::DEFAULT; @@ -344,7 +348,7 @@ void instantiateModule( throw jsg::JsExceptionThrown(); } - // Nothing to do if the module is already instantiated, evaluated. + // Nothing to do if the module is already evaluated. if (status == v8::Module::Status::kEvaluated || status == v8::Module::Status::kEvaluating) return; if (status == v8::Module::Status::kUninstantiated) { @@ -354,11 +358,7 @@ void instantiateModule( auto prom = jsg::check(module->Evaluate(context)).As(); if (module->IsGraphAsync() && prom->State() == v8::Promise::kPending) { - // Pump the microtasks if there's an unsettled top-level await in the module. - // Because we do not support i/o in this scope, this *should* resolve in a - // single drain of the microtask queue (tho it's possible that it'll take - // multiple tasks). When the runMicrotasks() is complete, the promise should - // be settled. + // If top level await has been disable, error. JSG_REQUIRE(options != InstantiateModuleOptions::NO_TOP_LEVEL_AWAIT, Error, "Top-level await in module is not permitted at this time."); } @@ -369,7 +369,7 @@ void instantiateModule( switch (prom->State()) { case v8::Promise::kPending: - // Let's make sure nobody is depending on pending modules that do not resolve first. + // Let's make sure nobody is depending on modules awaiting on pending promises. JSG_FAIL_REQUIRE(Error, "Top-level await in module is unsettled."); case v8::Promise::kRejected: // Since we don't actually support I/O when instantiating a worker, we don't return the @@ -507,7 +507,7 @@ v8::Local compileWasmModule( // ====================================================================================== -jsg::Ref ModuleRegistry::NodeJsModuleInfo::initModuleContext( +jsg::Ref ModuleRegistry::NodeJsModuleInfo::initModuleContext( jsg::Lock& js, kj::StringPtr name) { return jsg::alloc(js, kj::Path::parse(name)); } diff --git a/src/workerd/jsg/modules.h b/src/workerd/jsg/modules.h index b418582b8ef..3dc73b45480 100644 --- a/src/workerd/jsg/modules.h +++ b/src/workerd/jsg/modules.h @@ -88,6 +88,8 @@ class CommonJsModuleContext: public jsg::Object { // expected within the global scope of a Node.js compatible module (such as // Buffer and process). +// TODO(cleanup): There's a fair amount of duplicated code between the CommonJsModule +// and NodeJsModule types... should be deduplicated. class NodeJsModuleObject: public jsg::Object { public: NodeJsModuleObject(jsg::Lock& js, kj::String path); @@ -252,7 +254,7 @@ class ModuleRegistry { }; struct NodeJsModuleInfo { - jsg::Ref moduleContext; + jsg::Ref moduleContext; jsg::Function evalFunc; NodeJsModuleInfo(auto& lock, kj::StringPtr name, kj::StringPtr content) @@ -262,7 +264,7 @@ class ModuleRegistry { NodeJsModuleInfo(NodeJsModuleInfo&&) = default; NodeJsModuleInfo& operator=(NodeJsModuleInfo&&) = default; - static jsg::Ref initModuleContext(jsg::Lock& js, kj::StringPtr name); + static jsg::Ref initModuleContext(jsg::Lock& js, kj::StringPtr name); static v8::MaybeLocal evaluate(jsg::Lock& js, NodeJsModuleInfo& info, @@ -270,7 +272,7 @@ class ModuleRegistry { const kj::Maybe>& maybeExports); jsg::Function initEvalFunc(auto& lock, - jsg::Ref& moduleContext, + jsg::Ref& moduleContext, kj::StringPtr name, kj::StringPtr content) { v8::ScriptOrigin origin(v8StrIntern(lock.v8Isolate, name)); From e858332164080ea2b1673b35299e6dd7d2fcd84b Mon Sep 17 00:00:00 2001 From: James M Snell Date: Tue, 12 Nov 2024 07:28:26 -0800 Subject: [PATCH 3/3] Consolidate the require(...) primary implementation --- src/workerd/api/node/module.c++ | 45 ++-------- src/workerd/jsg/modules.c++ | 145 ++++++++++++++++++-------------- src/workerd/jsg/modules.h | 13 +++ 3 files changed, 98 insertions(+), 105 deletions(-) diff --git a/src/workerd/api/node/module.c++ b/src/workerd/api/node/module.c++ index dd9d247119c..dfedc885162 100644 --- a/src/workerd/api/node/module.c++ +++ b/src/workerd/api/node/module.c++ @@ -81,48 +81,13 @@ jsg::JsValue ModuleUtil::createRequire(jsg::Lock& js, kj::String path) { jsg::ModuleRegistry::ResolveMethod::REQUIRE, spec.asPtr()), Error, "No such module \"", targetPath.toString(), "\"."); - bool isEsm = info.maybeSynthetic == kj::none; - - auto module = info.module.getHandle(js); - - if (module->GetStatus() == v8::Module::Status::kEvaluating || - module->GetStatus() == v8::Module::Status::kInstantiating) { - KJ_IF_SOME(synth, info.maybeSynthetic) { - KJ_IF_SOME(cjs, synth.tryGet()) { - return cjs.moduleContext->getExports(js); - } - KJ_IF_SOME(cjs, synth.tryGet()) { - return cjs.moduleContext->getExports(js); - } - } - } - - auto features = FeatureFlags::get(js); - jsg::InstantiateModuleOptions options = jsg::InstantiateModuleOptions::DEFAULT; - if (features.getNoTopLevelAwaitInRequire()) { - options = jsg::InstantiateModuleOptions::NO_TOP_LEVEL_AWAIT; - - // If the module was already evaluated, let's check if it is async. - // If it is, we will throw an error. This case can happen if a previous - // attempt to require the module failed because the module was async. - if (module->GetStatus() == v8::Module::kEvaluated) { - JSG_REQUIRE(!module->IsGraphAsync(), Error, - "Top-level await in module is not permitted at this time."); - } - } - - jsg::instantiateModule(js, module, options); - - if (isEsm) { - // If the import is an esm module, we will return the namespace object. - jsg::JsObject obj(module->GetModuleNamespace().As()); - if (obj.get(js, "__cjsUnwrapDefault"_kj) == js.boolean(true)) { - return obj.get(js, "default"_kj); - } - return obj; + jsg::ModuleRegistry::RequireImplOptions options = + jsg::ModuleRegistry::RequireImplOptions::DEFAULT; + if (info.maybeSynthetic != kj::none) { + options = jsg::ModuleRegistry::RequireImplOptions::EXPORT_DEFAULT; } - return jsg::JsValue(js.v8Get(module->GetModuleNamespace().As(), "default"_kj)); + return jsg::ModuleRegistry::requireImpl(js, info, options); })); } diff --git a/src/workerd/jsg/modules.c++ b/src/workerd/jsg/modules.c++ index 8cbd143bf07..7a789a373c8 100644 --- a/src/workerd/jsg/modules.c++ +++ b/src/workerd/jsg/modules.c++ @@ -272,46 +272,12 @@ v8::Local CommonJsModuleContext::require(jsg::Lock& js, kj::String sp // Adding imported from suffix here not necessary like it is for resolveCallback, since we have a // js stack that will include the parent module's name and location of the failed require(). - auto module = info.module.getHandle(js); - - if (module->GetStatus() == v8::Module::Status::kEvaluating || - module->GetStatus() == v8::Module::Status::kInstantiating) { - KJ_IF_SOME(synth, info.maybeSynthetic) { - KJ_IF_SOME(cjs, synth.tryGet()) { - return cjs.moduleContext->getExports(js); - } - KJ_IF_SOME(cjs, synth.tryGet()) { - return cjs.moduleContext->getExports(js); - } - } - } - - auto& isolateBase = IsolateBase::from(js.v8Isolate); - jsg::InstantiateModuleOptions options = jsg::InstantiateModuleOptions::DEFAULT; - if (!isolateBase.isTopLevelAwaitEnabled()) { - options = jsg::InstantiateModuleOptions::NO_TOP_LEVEL_AWAIT; - - // If the module was already evaluated, let's check if it is async. - // If it is, we will throw an error. This case can happen if a previous - // attempt to require the module failed because the module was async. - if (module->GetStatus() == v8::Module::kEvaluated) { - JSG_REQUIRE(!module->IsGraphAsync(), Error, - "Top-level await in module is not permitted at this time."); - } - } - - jsg::instantiateModule(js, module, options); - - // Originally, This returned an object like `{default: module.exports}` when we really - // intended to return the module exports raw. We should be extracting `default` here. - // Unfortunately, there is a user depending on the wrong behavior in production, so we - // needed a compatibility flag to fix. + ModuleRegistry::RequireImplOptions options = ModuleRegistry::RequireImplOptions::DEFAULT; if (getCommonJsExportDefault(js.v8Isolate)) { - return check(module->GetModuleNamespace().As()->Get( - js.v8Context(), v8StrIntern(js.v8Isolate, "default"))); - } else { - return module->GetModuleNamespace(); + options = ModuleRegistry::RequireImplOptions::EXPORT_DEFAULT; } + + return ModuleRegistry::requireImpl(js, info, options); } v8::Local NonModuleScript::runAndReturn(v8::Local context) const { @@ -615,33 +581,7 @@ v8::Local NodeJsModuleContext::require(jsg::Lock& js, kj::String spec info.maybeSynthetic, TypeError, "Cannot use require() to import an ES Module."); } - auto module = info.module.getHandle(js); - - JSG_REQUIRE(module->GetStatus() != v8::Module::Status::kEvaluating && - module->GetStatus() != v8::Module::Status::kInstantiating, - Error, - "Module cannot be synchronously required while it is being instantiated or evaluated. " - "This error typically means that a CommonJS or NodeJS-Compat type module has a circular " - "dependency on itself, and that a synchronous require() is being called while the module " - "is being loaded."); - - auto& isolateBase = IsolateBase::from(js.v8Isolate); - jsg::InstantiateModuleOptions options = jsg::InstantiateModuleOptions::DEFAULT; - if (!isolateBase.isTopLevelAwaitEnabled()) { - options = jsg::InstantiateModuleOptions::NO_TOP_LEVEL_AWAIT; - - // If the module was already evaluated, let's check if it is async. - // If it is, we will throw an error. This case can happen if a previous - // attempt to require the module failed because the module was async. - if (module->GetStatus() == v8::Module::kEvaluated) { - JSG_REQUIRE(!module->IsGraphAsync(), Error, - "Top-level await in module is not permitted at this time."); - } - } - - jsg::instantiateModule(js, module, options); - - return js.v8Get(module->GetModuleNamespace().As(), "default"_kj); + return ModuleRegistry::requireImpl(js, info, ModuleRegistry::RequireImplOptions::EXPORT_DEFAULT); } v8::Local NodeJsModuleContext::getBuffer(jsg::Lock& js) { @@ -712,4 +652,79 @@ kj::Maybe> tryResolveFromFallb return kj::none; } +JsValue ModuleRegistry::requireImpl(Lock& js, ModuleInfo& info, RequireImplOptions options) { + auto module = info.module.getHandle(js); + + // If the module status is evaluating or instantiating then the module is likely + // has a circular dependency on itself. If the module is a CommonJS or NodeJS + // module, we can return the exports object directly here. + if (module->GetStatus() == v8::Module::Status::kEvaluating || + module->GetStatus() == v8::Module::Status::kInstantiating) { + KJ_IF_SOME(synth, info.maybeSynthetic) { + KJ_IF_SOME(cjs, synth.tryGet()) { + return JsValue(cjs.moduleContext->getExports(js)); + } + KJ_IF_SOME(cjs, synth.tryGet()) { + return JsValue(cjs.moduleContext->getExports(js)); + } + } + } + + // When using require(...) we previously allowed the required modules to use + // top-level await. With a compat flag we disable use of top-level await but + // ONLY when the module is synchronously required. The same module being imported + // either statically or dynamically can still use TLA. This aligns with behavior + // being implemented in other JS runtimes. + auto& isolateBase = IsolateBase::from(js.v8Isolate); + jsg::InstantiateModuleOptions opts = jsg::InstantiateModuleOptions::DEFAULT; + if (!isolateBase.isTopLevelAwaitEnabled()) { + opts = jsg::InstantiateModuleOptions::NO_TOP_LEVEL_AWAIT; + + // If the module was already evaluated, let's check if it is async. + // If it is, we will throw an error. This case can happen if a previous + // attempt to require the module failed because the module was async. + if (module->GetStatus() == v8::Module::kEvaluated) { + JSG_REQUIRE(!module->IsGraphAsync(), Error, + "Top-level await in module is not permitted at this time."); + } + } + + jsg::instantiateModule(js, module, opts); + + if (info.maybeSynthetic == kj::none) { + // If the module is an ESM and the __cjsUnwrapDefault flag is set to true, we will + // always return the default export regardless of the options. + // Otherwise fallback to the options. This is an early version of the "module.exports" + // convention that Node.js finally adopted for require(esm) that was not officially + // adopted but there are a handful of modules in the ecosystem that supported it + // early. It's trivial for us to support here so let's just do so. + JsObject obj(module->GetModuleNamespace().As()); + if (obj.get(js, "__cjsUnwrapDefault"_kj) == js.boolean(true)) { + return obj.get(js, "default"_kj); + } + // If the ES Module namespace exports a "module.exports" key then that will be the + // export that is returned by the require(...) call per Node.js' recently added + // require(esm) support. + // See: https://nodejs.org/docs/latest/api/modules.html#loading-ecmascript-modules-using-require + if (obj.has(js, "module.exports"_kj)) { + // We only want to return the value if it is explicitly specified, otherwise we'll + // always be returning undefined. + return obj.get(js, "module.exports"_kj); + } + } + + // Originally, require returned an object like `{default: module.exports}` when we really + // intended to return the module exports raw. We should be extracting `default` here. + // When Node.js recently finally adopted require(esm), they adopted the default behavior + // of exporting the module namespace, which is fun. We'll stick with our default here for + // now but users can get Node.js-like behavior by switching off the + // exportCommonJsDefaultNamespace compat flag. + if (options == RequireImplOptions::EXPORT_DEFAULT) { + return JsValue(check(module->GetModuleNamespace().As()->Get( + js.v8Context(), v8StrIntern(js.v8Isolate, "default")))); + } + + return JsValue(module->GetModuleNamespace()); +} + } // namespace workerd::jsg diff --git a/src/workerd/jsg/modules.h b/src/workerd/jsg/modules.h index 3dc73b45480..5647377e970 100644 --- a/src/workerd/jsg/modules.h +++ b/src/workerd/jsg/modules.h @@ -194,7 +194,10 @@ class NonModuleScript { }; enum class InstantiateModuleOptions { + // Allows pending top-level await in the module when evaluated. Will cause + // the microtask queue to be drained once in an attempt to resolve those. DEFAULT, + // Throws if the module evaluation results in a pending promise. NO_TOP_LEVEL_AWAIT, }; @@ -403,6 +406,16 @@ class ModuleRegistry { using DynamicImportCallback = Promise(jsg::Lock& js, kj::Function handler); virtual void setDynamicImportCallback(kj::Function func) = 0; + + enum class RequireImplOptions { + // Require returns the module namespace. + DEFAULT, + // Require returns the default export. + EXPORT_DEFAULT, + }; + + static JsValue requireImpl( + Lock& js, ModuleInfo& info, RequireImplOptions options = RequireImplOptions::DEFAULT); }; template