Skip to content

Commit 5ecc6bf

Browse files
jasnelljasnell
authored
Merge pull request #6462 from cloudflare/jasnell/nmr-tweaks
2 parents 5555f40 + 810fab8 commit 5ecc6bf

9 files changed

Lines changed: 75 additions & 20 deletions

File tree

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

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -333,6 +333,41 @@ export const complexModuleTest = {
333333
},
334334
};
335335

336+
// Regression test: getBuiltinModule called from a function defined in eval'd
337+
// code should work. Previously failed on first invocation with "top-level await"
338+
// error due to extra microtask tick from wrapSimplePromise().
339+
export const getBuiltinModuleFromEval = {
340+
async test(_, env) {
341+
const code = `"use strict";async (exports)=>{
342+
exports.getPath = () => process.getBuiltinModule("node:path");
343+
}`;
344+
const exports = {};
345+
const fn = env.unsafe.eval(code);
346+
await fn(exports);
347+
348+
const path = exports.getPath();
349+
ok(path.join, 'node:path should have join');
350+
},
351+
};
352+
353+
// Regression test: createRequire called from a function defined in eval'd
354+
// code should work. Same root cause as getBuiltinModuleFromEval.
355+
export const createRequireFromEval = {
356+
async test(_, env) {
357+
const code = `"use strict";async (exports)=>{
358+
const { createRequire } = process.getBuiltinModule("node:module");
359+
const require = createRequire("/");
360+
exports.getUtil = () => require("node:util");
361+
}`;
362+
const exports = {};
363+
const fn = env.unsafe.eval(code);
364+
await fn(exports);
365+
366+
const util = exports.getUtil();
367+
ok(util.promisify, 'node:util should have promisify');
368+
},
369+
};
370+
336371
// TODO(now): Tests
337372
// * [x] Include tests for all known module types
338373
// * [x] ESM

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,9 @@ const unitTests :Workerd.Config = (
7777
"new_module_registry",
7878
"experimental",
7979
],
80+
bindings = [
81+
(name = "unsafe", unsafeEval = void),
82+
],
8083
),
8184
),
8285
],

‎src/workerd/io/worker-modules.h‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -95,15 +95,20 @@ static kj::Arc<jsg::modules::ModuleRegistry> newWorkerModuleRegistry(
9595

9696
// This callback is used when a module is being loaded to arrange evaluating the
9797
// module outside of the current IoContext.
98-
builder.setEvalCallback([](jsg::Lock& js, const auto& module, auto v8Module,
99-
const auto& observer) -> jsg::Promise<jsg::Value> {
100-
return js.tryOrReject<jsg::Value>([&] {
101-
// Creating the SuppressIoContextScope here ensures that the current IoContext,
102-
// if any, is moved out of the way while we are evaluating.
103-
SuppressIoContextScope suppressIoContextScope;
104-
KJ_DASSERT(!IoContext::hasCurrent(), "Module evaluation must not be in an IoContext");
105-
return jsg::check(v8Module->Evaluate(js.v8Context()));
106-
});
98+
builder.setEvalCallback(
99+
[](jsg::Lock& js, const auto& module, auto v8Module, const auto& observer) -> jsg::JsPromise {
100+
// Creating the SuppressIoContextScope here ensures that the current IoContext,
101+
// if any, is moved out of the way while we are evaluating.
102+
SuppressIoContextScope suppressIoContextScope;
103+
KJ_DASSERT(!IoContext::hasCurrent(), "Module evaluation must not be in an IoContext");
104+
JSG_TRY(js) {
105+
auto ret = jsg::check(v8Module->Evaluate(js.v8Context()));
106+
KJ_ASSERT(ret->IsPromise());
107+
return jsg::JsPromise(ret.template As<v8::Promise>());
108+
}
109+
JSG_CATCH(exception) {
110+
return js.rejectedJsPromise(jsg::JsValue(exception.getHandle(js)));
111+
}
107112
});
108113

109114
// Add the module bundles that are built into the runtime.

‎src/workerd/jsg/jsg.c++‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -372,8 +372,9 @@ kj::Maybe<JsObject> Lock::resolveInternalModule(kj::StringPtr specifier) {
372372
kj::Maybe<JsObject> Lock::resolvePublicBuiltinModule(kj::StringPtr specifier) {
373373
auto& isolate = IsolateBase::from(v8Isolate);
374374
KJ_ASSERT(isolate.isUsingNewModuleRegistry());
375-
return jsg::modules::ModuleRegistry::tryResolveModuleNamespace(
376-
*this, specifier, jsg::modules::ResolveContext::Type::PUBLIC_BUILTIN)
375+
return jsg::modules::ModuleRegistry::tryResolveModuleNamespace(*this, specifier,
376+
jsg::modules::ResolveContext::Type::PUBLIC_BUILTIN,
377+
jsg::modules::ResolveContext::Source::REQUIRE)
377378
.map([](JsValue val) { return KJ_ASSERT_NONNULL(val.tryCast<JsObject>()); });
378379
}
379380

‎src/workerd/jsg/jsg.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2563,6 +2563,7 @@ class Lock {
25632563
// Like above, but return a pure-JS promise, not a typed Promise.
25642564
JsPromise rejectedJsPromise(jsg::JsValue exception);
25652565
JsPromise rejectedJsPromise(kj::Exception&& exception, ExceptionToJsOptions options = {});
2566+
JsPromise resolvedJsPromise(jsg::JsValue value);
25662567

25672568
// Like `kj::evalNow()`, but returns a jsg::Promise for the result. Synchronous exceptions are
25682569
// caught and returned as a rejected promise.

‎src/workerd/jsg/jsvalue.c++‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -514,6 +514,14 @@ JsPromise Lock::rejectedJsPromise(kj::Exception&& exception, ExceptionToJsOption
514514
return rejectedJsPromise(exceptionToJsValue(kj::mv(exception), options).getHandle(*this));
515515
}
516516

517+
JsPromise Lock::resolvedJsPromise(jsg::JsValue value) {
518+
v8::EscapableHandleScope handleScope(v8Isolate);
519+
auto context = v8Context();
520+
auto resolver = check(v8::Promise::Resolver::New(context));
521+
check(resolver->Resolve(context, value));
522+
return JsPromise(handleScope.Escape(resolver->GetPromise()));
523+
}
524+
517525
PromiseState JsPromise::state() {
518526
switch (inner->State()) {
519527
case v8::Promise::PromiseState::kPending:

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2059,7 +2059,7 @@ KJ_TEST("Using a deferred eval callback works") {
20592059
.setEvalCallback([&called](Lock& js, const Module& module, auto v8Module,
20602060
const auto& observer) {
20612061
called = true;
2062-
return js.resolvedPromise<Value>(js.v8Ref<v8::Value>(js.num(123)));
2062+
return js.resolvedJsPromise(js.num(123));
20632063
}).finish();
20642064

20652065
PREAMBLE([&](Lock& js) {

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

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,8 @@ class EsModule final: public Module {
227227
}
228228

229229
KJ_IF_SOME(result, maybeEvaluate(js, *this, module, observer)) {
230-
return js.wrapSimplePromise(kj::mv(result));
230+
v8::Local<v8::Value> val = result;
231+
return val;
231232
}
232233

233234
return actuallyEvaluate(js, module, observer);
@@ -311,7 +312,8 @@ class SyntheticModule final: public Module {
311312
// is specified, then we defer evaluation to the given callback.
312313
if (isEval()) {
313314
KJ_IF_SOME(result, maybeEvaluate(js, *this, module, observer)) {
314-
return js.wrapSimplePromise(kj::mv(result));
315+
v8::Local<v8::Value> val = result;
316+
return val;
315317
}
316318
}
317319
return module->Evaluate(js.v8Context());
@@ -695,7 +697,7 @@ class IsolateModuleRegistry final {
695697
}
696698
}
697699
} else {
698-
KJ_ASSERT(promise->State() != v8::Promise::kPending,
700+
KJ_ASSERT(!module->IsGraphAsync() && promise->State() != v8::Promise::kPending,
699701
"Top-level await is not supported in this context, so the module promise "
700702
"should never be pending");
701703
if (promise->State() == v8::Promise::kRejected) {
@@ -1669,7 +1671,7 @@ ModuleRegistry::ModuleRegistry(ModuleRegistry::Builder* builder)
16691671
maybeEvalCallback(kj::mv(builder->maybeEvalCallback)),
16701672
schemaLoader(kj::mv(builder->schemaLoader)) {}
16711673

1672-
kj::Maybe<jsg::Promise<Value>> ModuleRegistry::evaluateImpl(jsg::Lock& js,
1674+
kj::Maybe<jsg::JsPromise> ModuleRegistry::evaluateImpl(jsg::Lock& js,
16731675
const Module& module,
16741676
v8::Local<v8::Module> v8Module,
16751677
const CompilationObserver& observer) const {
@@ -1862,7 +1864,7 @@ Module::Module(Url id, Type type, Flags flags, ContentType contentType)
18621864
flags_(flags),
18631865
contentType_(contentType) {}
18641866

1865-
kj::Maybe<jsg::Promise<Value>> Module::Evaluator::operator()(jsg::Lock& js,
1867+
kj::Maybe<jsg::JsPromise> Module::Evaluator::operator()(jsg::Lock& js,
18661868
const Module& module,
18671869
v8::Local<v8::Module> v8Module,
18681870
const CompilationObserver& observer) const {

‎src/workerd/jsg/modules-new.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -274,7 +274,7 @@ class Module {
274274
class Evaluator final {
275275
public:
276276
KJ_DISALLOW_COPY_AND_MOVE(Evaluator);
277-
kj::Maybe<jsg::Promise<Value>> operator()(jsg::Lock& js,
277+
kj::Maybe<jsg::JsPromise> operator()(jsg::Lock& js,
278278
const Module& module,
279279
v8::Local<v8::Module> v8Module,
280280
const CompilationObserver& observer) const;
@@ -666,7 +666,7 @@ class ModuleRegistry final: public kj::AtomicRefcounted, public ModuleRegistryBa
666666
// Flag::EVAL on a module is ignored. If the EvalCallback is set, then any
667667
// Modules that have the Flag::EVAL set will have their evaluation deferred
668668
// to this callback.
669-
using EvalCallback = Function<jsg::Promise<Value>(
669+
using EvalCallback = Function<jsg::JsPromise(
670670
const Module& module, v8::Local<v8::Module> v8Module, const CompilationObserver& observer)>;
671671

672672
class Builder final {
@@ -791,7 +791,7 @@ class ModuleRegistry final: public kj::AtomicRefcounted, public ModuleRegistryBa
791791
ModuleBundle& bundle,
792792
const Url& bundleBase) KJ_WARN_UNUSED_RESULT;
793793

794-
kj::Maybe<jsg::Promise<Value>> evaluateImpl(jsg::Lock& js,
794+
kj::Maybe<jsg::JsPromise> evaluateImpl(jsg::Lock& js,
795795
const Module& module,
796796
v8::Local<v8::Module> v8Module,
797797
const CompilationObserver& observer) const;

0 commit comments

Comments
 (0)