Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JSRPC: Service bindings and stateless class syntax #1658

Merged
merged 18 commits into from
Feb 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/cloudflare/internal/workers.d.ts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually kinda think the internal file could still be entrypoints, and the public cloudflare:workers file could export a bunch of stuff from a few cloudflare-internal files?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe but it doesn't matter, the file is internal and ideally I'd like to remove it entirely when built-in modules (defined in C++) start supporting named exports properly.

Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
export class DurableObject {
public constructor(ctx: unknown, env: unknown);

public ctx: unknown;
public env: unknown;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these readonly?

}

export class WorkerEntrypoint {
public constructor(ctx: unknown, env: unknown);

public ctx: unknown;
public env: unknown;
}
11 changes: 11 additions & 0 deletions src/cloudflare/workers.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Copyright (c) 2024 Cloudflare, Inc.
// Licensed under the Apache 2.0 license found in the LICENSE file or at:
// https://opensource.org/licenses/Apache-2.0

// TODO(cleanup): C++ built-in modules do not yet support named exports, so we must define this
// wrapper module that simply re-exports the classes from the built-in module.

import entrypoints from 'cloudflare-internal:workers';

export const WorkerEntrypoint = entrypoints.WorkerEntrypoint;
export const DurableObject = entrypoints.DurableObject;
9 changes: 9 additions & 0 deletions src/workerd/api/global-scope.h
Original file line number Diff line number Diff line change
Expand Up @@ -255,12 +255,21 @@ struct ExportedHandler {
// Values to pass for `env` and `ctx` when calling handlers. Note these have to be the last members
// so that they don't interfere with `JSG_STRUCT`'s machinations.

// env and ctx values that need to be passed to the handler function. If the ExportedHandler
// represents a class instance (e.g. Durable Object instance), then `env` is is the JS value
// `undefined` and `ctx` is `kj::none`.
// TODO(cleanup): Why isn't `env` a `jsg::Optional` too? Or maybe the pair should be wrapped in
// a struct that is `Maybe`?
jsg::Value env = nullptr;
jsg::Optional<jsg::Ref<ExecutionContext>> ctx = kj::none;
// TODO(cleanup): These are shoved here as a bit of a hack. At present, this is convenient and
// works for all use cases. If we have bindings or things on ctx that vary on a per-request basis,
// this won't work as well, I guess, but we can cross that bridge when we come to it.

// If true, this is a Durable Object class that failed to extend `DurableObject`. We will not
// permit RPC to this class.
bool missingSuperclass = false;

jsg::Optional<jsg::Ref<ExecutionContext>> getCtx() {
return ctx.map([&](jsg::Ref<ExecutionContext>& p) { return p.addRef(); });
}
Expand Down
8 changes: 8 additions & 0 deletions src/workerd/api/http.h
Original file line number Diff line number Diff line change
Expand Up @@ -560,6 +560,9 @@ class Fetcher: public jsg::Object {
const v8::FunctionCallbackInfo<v8::Value>& info)>;

kj::Maybe<RpcFunction> getRpcMethod(jsg::Lock& js, kj::StringPtr name);
kj::Maybe<RpcFunction> getRpcMethodForTestOnly(jsg::Lock& js, kj::String name) {
return getRpcMethod(js, name);
}

JSG_RESOURCE_TYPE(Fetcher, CompatibilityFlags::Reader flags) {
// WARNING: New JSG_METHODs on Fetcher must be gated via compatibility flag to prevent
Expand Down Expand Up @@ -587,6 +590,11 @@ class Fetcher: public jsg::Object {

if (flags.getWorkerdExperimental()) {
JSG_WILDCARD_PROPERTY(getRpcMethod);

// We export a copy of getRpcMethod for use in tests only which allows the caller to provide
// an arbitrary string as the method name. This allows invoking methods that would normally
// be shadowed by non-wildcard methods.
JSG_METHOD(getRpcMethodForTestOnly);
}

JSG_TS_OVERRIDE({
Expand Down
2 changes: 2 additions & 0 deletions src/workerd/api/modules.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <workerd/api/rtti.h>
#include <workerd/api/sockets.h>
#include <workerd/api/unsafe.h>
#include <workerd/api/worker-rpc.h>
#include <workerd/io/worker.h>
#include <cloudflare/cloudflare.capnp.h>

Expand All @@ -28,6 +29,7 @@ void registerModules(Registry& registry, auto featureFlags) {
}
registerSocketsModule(registry, featureFlags);
registry.addBuiltinBundle(CLOUDFLARE_BUNDLE);
registerRpcModules(registry, featureFlags);
}

} // namespace workerd::api
38 changes: 0 additions & 38 deletions src/workerd/api/tests/js-rpc-disabled.js

This file was deleted.

24 changes: 24 additions & 0 deletions src/workerd/api/tests/js-rpc-flag.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Copyright (c) 2023 Cloudflare, Inc.
// Licensed under the Apache 2.0 license found in the LICENSE file or at:
// https://opensource.org/licenses/Apache-2.0

import assert from 'node:assert';

// This class does not extend `DurableObject`, but because we have the `js_rpc` compat flag on,
// it'll still accept RPC.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You deleted the "js-rpc-disabled" test, should something like that still exist?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of this commit, the main js-rpc-test no longer has the flag set, and tests all the behavior around that, making js-rpc-disabled redundant. I did however introduce js-rpc-flag which is the opposite test: it tests behavior when the flag is enabled.

export class DurableObjectExample {
constructor() {}

foo() {
return 123;
}
}

export default {
async test(ctrl, env, ctx) {
let id = env.ns.idFromName("foo");
let obj = env.ns.get(id);
assert.strictEqual(await obj.foo(), 123);
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ const config :Workerd.Config = (

const mainWorker :Workerd.Worker = (
compatibilityDate = "2022-09-16",
compatibilityFlags = ["experimental", "nodejs_compat"],
compatibilityFlags = ["experimental", "nodejs_compat", "js_rpc"],

modules = [
(name = "worker", esModule = embed "js-rpc-disabled.js"),
(name = "worker", esModule = embed "js-rpc-flag.js"),
],

durableObjectNamespaces = [
Expand Down
Loading
Loading