Fix returning interface types from rpc methods

[alpertuna]

Fixes #2003

interface does not extend Serializable but type does.

interface R1 {
  field: string;
}
type R2 = {
  field: string;
}

R1 extends Serializable fails. R2 extends Serializable ok.

After the patch both interface and type match Serializable.

This fix is a starting point for the issue, there may be a better solution.

[github-actions[bot]]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[alpertuna]

Better fix may be done like this;

Here is Serializable type https://github.com/cloudflare/workerd/blob/4098ac7c038d5097c470b3b5413b598b52730b98/types/defines/rpc.d.ts#L30-L58

This line does not match interfaces

    | { [key: string | number]: Serializable }

To make it work, Serializable should take the interface as a generic type like this;

type Serializable<T> =
...
    | { [K in keyof T]: Serializable<T[K]> }
...

But this solution make things more complicated.

[kentonv]

The trouble is, we don't want user-defined classes to be considered serializable unless they extend RpcTarget.

Does this change keep that property, or will it make all user-defined classes be considered serializable?

[alpertuna]

@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. So the real responsible part is the business logic that serializes.

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) As you said We don't want user-defined classes I just added the constraint, so developers use RpcTarget for advanced usage. But I'm not sure if this constraint should be there.

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 : )

[alpertuna]

I read the documentation about RpcTarget, and my fix conflicted with the documentation since using callback as rpc return type is valid usage. I just reverted the function check in case of that you may use this pull request.

Now all the fix does is let class instances and interfaces as return types of an rpc method.

[kentonv]

Can someone on the team who knows typescript better than me review this? @petebacondarwin @penalosa ?

[RamIdeas]

LGTM

Just needed to use the generic within the recursive parts of the type too

[RamIdeas]

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 https://github.com/microsoft/TypeScript/issues/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

[alpertuna]

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.)

[kentonv]

Regardless of what the type system says, attempting to serialize an instance of a class (that does not extend RpcTarget) will fail at runtime. This is an intentional design choice which we won't be changing.

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.

[alpertuna]

If class instances cause failure at runtime, I respect and agree that types should not let this usage.

Btw I missed fixing other Structured cloneable composites types, that's my fault and a proper fix should be like this;

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 Serializable type is getting spagettier and may be harder to maintain future improvements, so I'm not confident with this fix.

[RamIdeas]

Regardless of what the type system says, attempting to serialize an instance of a class (that does not extend RpcTarget) will fail at runtime. This is an intentional design choice which we won't be changing.

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.

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.

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.

[jfsiii]

Linking this comment for visibility

[kentonv]

Sorry I lost track of this.

Honestly I don't understand how this works. It might be nice to add a comment, unless this should just be obvious to typescript wizards.

Otherwise @penalosa if you could get the test passing (looks like CI doesn't like it right now) and click your approval then I'll be happy to merge.

[alpertuna]

Some little further fixes need to be done before merging. Let me do it.

[alpertuna]

Here are further main fixes:

• Fixed Serializable to properly resolve structured composite types.

-    | Map<Serializable<T>, Serializable<T>>
-    | Set<Serializable<T>>
-    | ReadonlyArray<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>

• I realized that when using interfaces, callbacks were not being stubified (would not return a promise). I fixed this in the Stubify<T> type with a small change:

- : T extends { [key: string | number]: unknown } ? { [K in keyof T]: Stubify<T[K]> }
+ : T extends { [key: string | number]: any } ? { [K in keyof T]: Stubify<T[K]> }

I also added a comment at the top of the Serializable type.

See the commit: https://github.com/cloudflare/workerd/pull/2040/commits/09429df522cb1d43c5ead365035b959e9d035c6b

[kentonv]

Thanks so much @alpertuna!

This is looking pretty good to me but I'd appreciate if @penalosa could sign off before we merge.

[kentonv]

Doh, there's now a merge conflict...

[jasnell]

@alpertuna ... rather than using a merge commit here, can I ask you to rebase the PR on main?

[alpertuna]

@alpertuna ... rather than using a merge commit here, can I ask you to rebase the PR on main?

@jasnell Sorry about that. Everything should be fine now.