Skip to content

Commit 9dfd653

Browse files
jasnelljasnell
committed
Fixup return value handling
1 parent 65eda74 commit 9dfd653

5 files changed

Lines changed: 27 additions & 23 deletions

File tree

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,12 +32,12 @@ jsg::JsValue ModuleUtil::createRequire(jsg::Lock& js, kj::String path) {
3232
specifier = kj::mv(nodeSpec);
3333
}
3434
}
35-
KJ_IF_SOME(ns,
35+
KJ_IF_SOME(val,
3636
jsg::modules::ModuleRegistry::tryResolveModuleNamespace(js, specifier,
3737
jsg::modules::ResolveContext::Type::BUNDLE,
3838
jsg::modules::ResolveContext::Source::REQUIRE, referrer,
3939
jsg::modules::UnwrapDefault::YES)) {
40-
return ns;
40+
return val;
4141
}
4242
JSG_FAIL_REQUIRE(Error, kj::str("Module not found: ", specifier));
4343
}));

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -358,7 +358,8 @@ kj::Maybe<JsObject> Lock::resolveInternalModule(kj::StringPtr specifier) {
358358
auto& isolate = IsolateBase::from(v8Isolate);
359359
if (isolate.isUsingNewModuleRegistry()) {
360360
return jsg::modules::ModuleRegistry::tryResolveModuleNamespace(
361-
*this, specifier, jsg::modules::ResolveContext::Type::BUILTIN);
361+
*this, specifier, jsg::modules::ResolveContext::Type::BUILTIN)
362+
.map([](JsValue val) { return KJ_ASSERT_NONNULL(val.tryCast<JsObject>()); });
362363
}
363364

364365
// Use the original module registry implementation
@@ -372,14 +373,16 @@ kj::Maybe<JsObject> Lock::resolvePublicBuiltinModule(kj::StringPtr specifier) {
372373
auto& isolate = IsolateBase::from(v8Isolate);
373374
KJ_ASSERT(isolate.isUsingNewModuleRegistry());
374375
return jsg::modules::ModuleRegistry::tryResolveModuleNamespace(
375-
*this, specifier, jsg::modules::ResolveContext::Type::PUBLIC_BUILTIN);
376+
*this, specifier, jsg::modules::ResolveContext::Type::PUBLIC_BUILTIN)
377+
.map([](JsValue val) { return KJ_ASSERT_NONNULL(val.tryCast<JsObject>()); });
376378
}
377379

378380
kj::Maybe<JsObject> Lock::resolveModule(kj::StringPtr specifier, RequireEsm requireEsm) {
379381
auto& isolate = IsolateBase::from(v8Isolate);
380382
if (isolate.isUsingNewModuleRegistry()) {
381383
return jsg::modules::ModuleRegistry::tryResolveModuleNamespace(
382-
*this, specifier, jsg::modules::ResolveContext::Type::BUNDLE);
384+
*this, specifier, jsg::modules::ResolveContext::Type::BUNDLE)
385+
.map([](JsValue val) { return KJ_ASSERT_NONNULL(val.tryCast<JsObject>()); });
383386
}
384387

385388
auto moduleRegistry = jsg::ModuleRegistry::from(*this);

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

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1480,9 +1480,10 @@ KJ_TEST("UNWRAP_DEFAULT returns namespace for bundle ESM, default for others") {
14801480
// Bundle ESM without __cjsUnwrapDefault: tryResolveModuleNamespace with UnwrapDefault::YES
14811481
// returns the full namespace (has "default", "name" properties).
14821482
js.tryCatch([&] {
1483-
auto ns = KJ_ASSERT_NONNULL(ModuleRegistry::tryResolveModuleNamespace(js, "file:///esm-mod",
1483+
auto val = KJ_ASSERT_NONNULL(ModuleRegistry::tryResolveModuleNamespace(js, "file:///esm-mod",
14841484
ResolveContext::Type::BUNDLE, ResolveContext::Source::REQUIRE, kj::none,
14851485
modules::UnwrapDefault::YES));
1486+
auto ns = KJ_ASSERT_NONNULL(val.tryCast<JsObject>());
14861487
// The namespace should have both "default" and "name" properties.
14871488
auto nameVal = ns.get(js, "name");
14881489
KJ_ASSERT(!nameVal.isUndefined());
@@ -1496,10 +1497,7 @@ KJ_TEST("UNWRAP_DEFAULT returns namespace for bundle ESM, default for others") {
14961497
auto result = KJ_ASSERT_NONNULL(ModuleRegistry::tryResolveModuleNamespace(js,
14971498
"file:///esm-cjs", ResolveContext::Type::BUNDLE, ResolveContext::Source::REQUIRE,
14981499
kj::none, modules::UnwrapDefault::YES));
1499-
// Should be the unwrapped default, not the namespace.
1500-
// The default is a string, so ToObject wraps it. Check via valueOf().
1501-
auto val = JsValue(result);
1502-
KJ_ASSERT(kj::str(val) == "unwrapped");
1500+
KJ_ASSERT(kj::str(result) == "unwrapped");
15031501
}, [&](Value exception) { js.throwException(kj::mv(exception)); });
15041502

15051503
// JSON synthetic module: returns the parsed value (the default export).
@@ -1508,23 +1506,24 @@ KJ_TEST("UNWRAP_DEFAULT returns namespace for bundle ESM, default for others") {
15081506
"file:///data.json", ResolveContext::Type::BUNDLE, ResolveContext::Source::REQUIRE,
15091507
kj::none, modules::UnwrapDefault::YES));
15101508
// Should be the parsed JSON object, not the namespace.
1511-
KJ_ASSERT(kj::str(result.get(js, "key")) == "value");
1509+
auto obj = KJ_ASSERT_NONNULL(result.tryCast<JsObject>());
1510+
KJ_ASSERT(kj::str(obj.get(js, "key")) == "value");
15121511
}, [&](Value exception) { js.throwException(kj::mv(exception)); });
15131512

15141513
// Text synthetic module: returns the string value (the default export).
15151514
js.tryCatch([&] {
15161515
auto result = KJ_ASSERT_NONNULL(ModuleRegistry::tryResolveModuleNamespace(js,
15171516
"file:///data.txt", ResolveContext::Type::BUNDLE, ResolveContext::Source::REQUIRE,
15181517
kj::none, modules::UnwrapDefault::YES));
1519-
// The default is a string, wrapped in a String object by ToObject.
1520-
auto val = JsValue(result);
1521-
KJ_ASSERT(kj::str(val) == "hello world");
1518+
// The default is a string — JsValue directly.
1519+
KJ_ASSERT(kj::str(result) == "hello world");
15221520
}, [&](Value exception) { js.throwException(kj::mv(exception)); });
15231521

15241522
// Without UnwrapDefault, all modules return the namespace.
15251523
js.tryCatch([&] {
1526-
auto ns = KJ_ASSERT_NONNULL(ModuleRegistry::tryResolveModuleNamespace(
1524+
auto val = KJ_ASSERT_NONNULL(ModuleRegistry::tryResolveModuleNamespace(
15271525
js, "file:///data.json", ResolveContext::Type::BUNDLE, ResolveContext::Source::REQUIRE));
1526+
auto ns = KJ_ASSERT_NONNULL(val.tryCast<JsObject>());
15281527
// Should have a "default" property (it's the namespace).
15291528
KJ_ASSERT(!ns.get(js, "default").isUndefined());
15301529
}, [&](Value exception) { js.throwException(kj::mv(exception)); });

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

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -575,7 +575,7 @@ class IsolateModuleRegistry final {
575575
// like the CommonJS require. Returns the instantiated/evaluated module namespace.
576576
// If an empty v8::MaybeLocal is returned and the default option is given, then an
577577
// exception has been scheduled.
578-
v8::MaybeLocal<v8::Object> require(
578+
v8::MaybeLocal<v8::Value> require(
579579
Lock& js, const ResolveContext& context, RequireOption option = RequireOption::DEFAULT) {
580580
// Returns either the module namespace or, when UNWRAP_DEFAULT is set and
581581
// the module is not ESM, the default export from the namespace. This matches
@@ -708,7 +708,7 @@ class IsolateModuleRegistry final {
708708
KJ_UNREACHABLE;
709709
};
710710

711-
return js.tryCatch([&]() -> v8::MaybeLocal<v8::Object> {
711+
return js.tryCatch([&]() -> v8::MaybeLocal<v8::Value> {
712712
KJ_IF_SOME(processUrl, maybeRedirectNodeProcess(js, context.normalizedSpecifier.getHref())) {
713713
ResolveContext newContext{
714714
.type = ResolveContext::Type::BUILTIN_ONLY,
@@ -1161,12 +1161,13 @@ v8::MaybeLocal<std::conditional_t<IsSourcePhase, v8::Object, v8::Module>> resolv
11611161
// with the compiled record, so that we can just directly read `sourceObject_` off of
11621162
// entry.module instead.
11631163
if (entry.module.isWasm()) {
1164-
v8::Local<v8::Object> moduleNamespace;
1164+
v8::Local<v8::Value> moduleNamespace;
11651165
if (registry
11661166
.require(js, resolveContext, IsolateModuleRegistry::RequireOption::RETURN_EMPTY)
11671167
.ToLocal(&moduleNamespace)) {
11681168
v8::Local<v8::Value> defaultExport;
1169-
if (moduleNamespace->Get(js.v8Context(), js.strIntern("default"_kj))
1169+
if (moduleNamespace.As<v8::Object>()
1170+
->Get(js.v8Context(), js.strIntern("default"_kj))
11701171
.ToLocal(&defaultExport)) {
11711172
if (defaultExport->IsWasmModuleObject()) {
11721173
return defaultExport.As<v8::Object>();
@@ -1801,7 +1802,7 @@ kj::Maybe<const Module&> ModuleRegistry::lookup(const ResolveContext& context) c
18011802
return kj::none;
18021803
}
18031804

1804-
kj::Maybe<JsObject> ModuleRegistry::tryResolveModuleNamespace(Lock& js,
1805+
kj::Maybe<JsValue> ModuleRegistry::tryResolveModuleNamespace(Lock& js,
18051806
kj::StringPtr specifier,
18061807
ResolveContext::Type type,
18071808
ResolveContext::Source source,
@@ -1839,7 +1840,7 @@ kj::Maybe<JsObject> ModuleRegistry::tryResolveModuleNamespace(Lock& js,
18391840
throw JsExceptionThrown();
18401841
}
18411842
if (ns.IsEmpty()) return kj::none;
1842-
return JsObject(check(ns));
1843+
return JsValue(check(ns));
18431844
}
18441845

18451846
JsValue ModuleRegistry::resolve(Lock& js,
@@ -1848,7 +1849,8 @@ JsValue ModuleRegistry::resolve(Lock& js,
18481849
ResolveContext::Type type,
18491850
ResolveContext::Source source,
18501851
kj::Maybe<const Url&> maybeReferrer) {
1851-
KJ_IF_SOME(ns, tryResolveModuleNamespace(js, specifier, type, source, maybeReferrer)) {
1852+
KJ_IF_SOME(val, tryResolveModuleNamespace(js, specifier, type, source, maybeReferrer)) {
1853+
auto ns = KJ_ASSERT_NONNULL(val.tryCast<JsObject>());
18521854
return ns.get(js, exportName);
18531855
}
18541856
JSG_FAIL_REQUIRE(Error, kj::str("Module not found: ", specifier));

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -731,7 +731,7 @@ class ModuleRegistry final: public kj::AtomicRefcounted, public ModuleRegistryBa
731731
// JsExceptionThrown exception if an error occurs while the module is being evaluated.
732732
// Modules resolved with this method must be capable of fully evaluating within one
733733
// drain of the microtask queue.
734-
static kj::Maybe<JsObject> tryResolveModuleNamespace(Lock& js,
734+
static kj::Maybe<JsValue> tryResolveModuleNamespace(Lock& js,
735735
kj::StringPtr specifier,
736736
ResolveContext::Type type = ResolveContext::Type::BUNDLE,
737737
ResolveContext::Source source = ResolveContext::Source::INTERNAL,

0 commit comments

Comments
 (0)