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
Fix returning interface types from rpc methods #2040
base: main
Are you sure you want to change the base?
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
Better fix may be done like this; Here is workerd/types/defines/rpc.d.ts Lines 30 to 58 in 4098ac7
This line does not match interfaces
To make it work,
But this solution make things more complicated. |
The trouble is, we don't want user-defined classes to be considered serializable unless they extend Does this change keep that property, or will it make all user-defined classes be considered serializable? |
@kentonv I see the trouble, but the point is if developers believe that if the value is serializable and the type check fails, they will use some workarounds or will manuplate types, like here; class SomeClass {
member: number = 0;
}
type WorkAround = {
member: number
};
class SomeDurableObject {
async someRpc(): Promise<WorkAround> {
return new SomeClass();
}
}
// This will pass the check
const res = await someDoStub.someRpc();
console.log(res.member); There is no way to avoid the example above. I think that if the value is serializable, types should let it without workarounds that look unnecessary. I also improved the fix and added a new constraint; object member cannot be a function. Here is an example that shows the before and after the fix; type RpcReturnType1 = {
a: number;
sub: {
b: number;
},
}
// Before fix: Ok
// After fix: Ok
interface SomeSubInterface {
b: number;
}
interface RpcReturnType2 {
a: number;
sub: SomeSubInterface;
}
// Before fix: Fails
// After fix: Ok
type RpcReturnType3 = {
cb: () => number
}
// Before fix: Ok (functions are considered objects, that's why they pass the check)
// After fix: Fails (added new constraint in the check) Actually, it looks like returning callbacks are ok as I tested. (I didn't know rpc was capable of this) By the way, my work here is to speed up things, and it is ok if you don't like it as I understand your concerns : ) |
I read the documentation about RpcTarget, and my fix conflicted with the documentation since using callback as rpc return type is valid usage. Now all the fix does is let class instances and interfaces as return types of an rpc method. |
Can someone on the team who knows typescript better than me review this? @petebacondarwin @penalosa ? |
+ format file with prettier
LGTM Just needed to use the generic within the recursive parts of the type too |
The tests are failing because of the requirement Kenton metioned above: "we don't want user-defined classes to be considered serializable unless they extend RpcTarget" I'll take another look at this now Update: I tried: + | (T extends RpcTargetBranded ?
{
[K in keyof T]: K extends number | string
? Serializable<T[K]>
: never;
}
+ : never) But it didn't seem to change the tests outcome. Even if it worked, it would've prevented plain objects from being serializable. It looks like there's no way in typescript to determine if an object is a class/subclass vs a plain object microsoft/TypeScript#29063 I think the way forward here is to allow classes to be serializable but make clear to users that the serialized form is a plain-object, not the original class instance |
Since the returned object is stubified, return types of all methods of the object will be Promise, and that will make a visible difference that it is not the original class instance. (Unless all methods of the original class are already returning Promise.) |
Regardless of what the type system says, attempting to serialize an instance of a class (that does not extend If there's no way for TypeScript to enforce the same at type-checking time, then it seems we should make TypeScript more lenient, and just live with the fact that this problem won't be diagnosed until runtime. |
If class instances cause failure at runtime, I respect and agree that types should not let this usage. Btw I missed fixing other type Serializable<T> =
...
| Map<
T extends Map<infer U, unknown> ? Serializable<U> : never,
T extends Map<unknown, infer U> ? Serializable<U> : never
>
| Set<T extends Set<infer U> ? Serializable<U> : never>
| ReadonlyArray<T extends ReadonlyArray<infer U> ? Serializable<U> : never>
... Now my main concern is |
To clarify, I did mean to allow it in the typings, not to change the runtime behaviour to allow it. I admit though I made an assumption that, since plain objects are allowed, classes would work in some form at runtime as if they were plain objects – eg. instance properties and methods work but prototype methods would not. But I see from the docs that we have decided that behaviour is "rarely useful in practice" and to avoid the footgun.
Essentially, what I was saying. And if we deem it useful to catch these anti-patterns at dev-time, we can implement a lint rule. |
Fixes #2003
interface
does not extendSerializable
buttype
does.R1 extends Serializable
fails.R2 extends Serializable
ok.After the patch both
interface
andtype
matchSerializable
.This fix is a starting point for the issue, there may be a better solution.