cloudflare / workerd

The JavaScript / Wasm runtime that powers Cloudflare Workers
https://blog.cloudflare.com/workerd-open-source-workers-runtime/
Apache License 2.0
6.06k stars 287 forks source link

rpc can only return `type` not `interface`/`class` #2003

Open helloimalastair opened 5 months ago

helloimalastair commented 5 months ago

Repro

When calling a DO RPC method, return type collapses to Promise<undefined>/never if using an interface. When using a type, return values are as expected.

helloimalastair commented 5 months ago

Just ran into it not working with WebSockets either, which are classes, though not sure you can move a WebSocket anyway...

kentonv commented 5 months ago

As discussed, it's expected (and documented) that class instances are only supported if they extend RpcTarget or are instances of one of the specific API types that are documented to work. WebSocket is not currently supported.

That said, it sounds like there is arguably a bug in the TypeScript types. It sounds like the types treat interface as strictly referring to class instances, but actually an interface can be satisfied by a plain object too.

@mrbbot Is there any way for us to more specifically filter out class instances using the TypeScript type system?

kentonv commented 5 months ago

Err, apparently @mrbbot recently left the company. Sorry for the spurious ping @mrbbot, hope things go well at whatever's next for you.

I'm not sure who that leaves as the typescript wizard now. @GregBrimble any thoughts?

katis commented 4 months ago

Recursively mapping the return type to a type alias helped me work around this issue with some 3rd party types

type InterfaceToType<T> = T extends object ? { [P in keyof T]: InterfaceToType<T[P]> } : T

class Foo extends RpcTarget {
    async bar(): Promise<InterfaceToType<InterfaceOfThirdParty>> {
            // ...
        }
}
GregBrimble commented 4 months ago

@penalosa would be my next go-to!

ngbrown commented 1 month ago

I had a hell of a time trying to get typings working between my Worker RPC service class extending WorkerEntrypoint<Env> and the consuming Pages function.

They are setup as two separate components in a mono repo with separate tsconfig.json files. I tried using "composite": true and "references": [] in my tsconfig.json files. I tried not using them. Nothing seemed to work. The branded types seem to have a really hard time transferring across TypeScript contexts. If the component with the worker providing the service emits typescript definitions ("emitDeclarationOnly": true), the output .d.ts file includes a copy of the class WorkerEntrypoint, without the branding. So maybe that is what is happening in memory when TypeScript analyzes the whole solution.

I also tried putting /// <reference types="@cloudflare/workers-types" preserve="true" /> at the top of various files. That also didn't help.

The output types the functions use are all type definitions, after I converted from interface... I saw that in the linked pull request, but I'm still getting the same error with just type definitions. The RPC functions are declared like this: async getUser(id: string): Promise<AppUser | null>, with the type specified explicitly. Removing the explicit type didn't necessarily help either because they were specifically specified within the function at the database call.

Anyways, in the end, I just wanted my types to resolve to something besides never and null. I also wanted the [Symbol.dispose] on them to be correctly in place. So I finally patched the @cloudflare/workers-types package to remove the never option.

diff --git a/node_modules/@cloudflare/workers-types/2023-07-01/index.ts b/node_modules/@cloudflare/workers-types/2023-07-01/index.ts
index 19d049f..8b653d8 100644
--- a/node_modules/@cloudflare/workers-types/2023-07-01/index.ts
+++ b/node_modules/@cloudflare/workers-types/2023-07-01/index.ts
@@ -5000,9 +5000,7 @@ export declare namespace Rpc {
   // Intersecting with `(Maybe)Provider` allows pipelining.
   type Result<R> = R extends Stubable
     ? Promise<Stub<R>> & Provider<R>
-    : R extends Serializable
-      ? Promise<Stubify<R> & MaybeDisposable<R>> & MaybeProvider<R>
-      : never;
+    : Promise<Stubify<R> & MaybeDisposable<R>> & MaybeProvider<R>;
   // Type for method or property on an RPC interface.
   // For methods, unwrap `Stub`s in parameters, and rewrite returns to be `Result`s.
   // Unwrapping `Stub`s allows calling with `Stubable` arguments.

That helped somewhat, but seemingly identical functions are responding different to using user = ... (or user?.[Symbol.dispose]();). Evidently, with both the index.ts file and index.d.ts file in the library, TypeScript seems to use one file sometimes and the other file other times. So I actually had to patch the .d.ts file too.

If you are wondering why it's a dated version of the types, I tried without the date path and with the date path, and neither had any difference. The templates default to no date, but the @cloudflare/workers-types readme says that the types are then the oldest version. I'm not sure what's up with that.

I don't think it's worth it to shut down the typing system by forcing a never output type if all the stars don't align perfectly. TypeScript works best as a hinting system, not as a fool-proof compiler.

jfsiii commented 1 month ago

I reached out to @andarist for help on Twitter. Here's a paste of the thread:


Mateusz Burzyński @AndaristRake Jun 22 is the problem here with bazBumOptional at the bottom?

https://tsplay.dev/mqgJQw

John Schulz @JFSIII Jun 22 In that example (nice one, btw, thanks) the issue is with bazBum vs fooBar.

I made some small tweaks in DO https://tsplay.dev/w2oG4N

await stub.fooBar() works as expected await stub.bazBum() has type never & gives "'await' has no effect ..."

Interesting that | undefined fixes

Mateusz Burzyński @AndaristRake Jun 23 it's nice that today Serializable is a union but this creates a problem here, interfaces don't have "inferable indexes" so checking if they extend Serializable fails for them (as u already know)

Mateusz Burzyński @AndaristRake Jun 23 one way to fix this would be to make Serializable a conditional type, this was in a fallback branch u could check smth like:

T[keyof T] extends Serializable ? ... : never

Mateusz Burzyński @AndaristRake Jun 23 this would check if its members are serializable, not if the whole type is serializable. First u'd have to check if it doesn't have any symbol members as those are not serialiable. But other than that, it should work, i think

Mateusz Burzyński @AndaristRake Jun 23 And it would be pretty close to what Serializable is doing today already


I think https://github.com/cloudflare/workerd/pull/2040 tries the T[keyof T] extends Serializable ? ... : never but I'm not entirely sure.

I mainly wanted to bring andarist's comments/suggestions to the relevant place.

Also, we now have TS Playgrounds to use for this!

jfsiii commented 6 days ago

In my experience, limiting to type is not enough. One must also avoid unknown as well.

Is there any progress on a fix or other workaround for this? This is a really painful experience and makes it difficult (arguably unfeasible) to advocate for using this otherwise incredible feature.