feat: Add ability to generate a generic Rpc interface and client implementation

[corwinsheahan-wf]

This PR introduces a way to generate a generic Rpc interface, as well as client implementations to match. The inspiration for this change comes from this comment.

This change allows the implementer of the Rpc interface to serialize the data sent over the wire however they wish.

Summary of changes:

• When ts_proto_opts=outputClientImpl="generic" is set, the plugin outputs an Rpc interface that looks like this for the basic unary request (other methods on Rpc are similarly updated):

interface Rpc {
  request<Req, Res>(
    service: string,
    method: string,
    request: Req,
    reqType: MessageType,
    respType: MessageType,
  ): Promise<Res>;
}

• ts_proto_opts=outputClientImpl="generic" sets outputTypeRegistry=true, since the MessageType gives us a super interface for the serialization instances.
• When ts_proto_opts=outputClientImpl="generic" is set, outputs a generated client implementation which uses the generic Rpc interface and support the existing options of the client implementation (rpc before/after, handleError, asyncIterable, batching).
• Adds tests
• Existing client impl generations are left unchanged.

[corwinsheahan-wf]

@stephenh Any chance you would be open to reviewing this change? Happy to make any adjustments necessary.

[corwinsheahan-wf]

@stephenh Sorry to keep prodding, but this is a change that we would still really like to contribute if you're open to it. LMK if there's anything we can do to make that possible.

Thanks so much!

[stephenh]

Hi @corwinsheahan-wf apologies for not replying sooner; I'd skimmed the PR a few times and couldn't quite succinctly articulate my thoughts/am not super-close to the use case, so just had it easy to keep procrastinating writing up a reply.

I think my two main questions, just to understand better, are looking at this interface:

  request<Req, Res>(
    service: string,
    method: string,
    request: Req,
    reqType: MessageType,
    respType: MessageType,
  ): Promise<Res>;

1. Do we need the Req and Res generics?

Given they a) are just typing the request: ... and Promise<Res> (i.e. not being used to infer/type other parameters), b) have no constraints on them/between them (i.e. I could call request<string, number> and it would be fine, vs. something fancy that ensures Req & Res are "the same pair" of req/res types), and c) the request method is only ever called from "trusted" / generated code, I'm wondering if we can just remove/skip them?

I.e. maybe just:

  request(
    service: string,
    method: string,
    request: object,
    reqType: MessageType,
    respType: MessageType,
  ): Promise<object>;

Hm, I suppose the Promise<object> would be annoying.

I dunno, I am probably too tempted to do fancy things, but I'd try something thing:

  request<ReqType extends MessageType, ResType extends MessageType>(
    service: string,
    method: string,
    request: ReturnType<ReqType["create"]>,
    reqType: ReqType,
    respType: ResType,
  ): Promise<ReturnType<ResType["create"]>>;

After adding a create method to MessageType:

export interface MessageType<Message extends UnknownMessage = UnknownMessage> {
  $type: Message["$type"];
  create(object: any): Message;

And then letting the generics get inferred:

    const response = this.rpc.request(this.service, "Unary", request, GetBasicRequest, GetBasicResponse);

I'd also probably reorder the parameters, just to have "the most static" ones come first:

  request<ReqType extends MessageType, ResType extends MessageType>(
    reqType: ReqType,
    respType: ResType,
    service: string,
    method: string,
    request: ReturnType<ReqType["create"]>,
  ): Promise<ReturnType<ResType["create"]>>;

Does this updated request typing make sense/sound okay?

2. You'd mentioned this generic client impl should default-on the type registry--I guess that's fine (it does work well to get the MessageType interface itself, which per ^ we can use for some generic type inference niceties), but I'm just curious, what does your typical Rpc.request implementation look like?

It makes sense there is some impl-specific details in there, but specifically I'd assumed that typically it would just end up calling reqType.encode or reqType.toJson, and then resType.decodeorreqType.fromJson`, depending on the wire format (binary or JSON), and defer to the internal serde logic of the encode/decode/toJson/fromJson methods.

...oh wait, I was thinking of the outputSchema, which re-creates/encodes a whole bunch of runtime metadata for the schema, and not just typeRegistry.

Okay, that makes more sense then!

[corwinsheahan-wf]

No worries @stephenh, I appreciate the response! Apologies for my own delay in getting back to you, I've been out sick.

1. Do we need the Req and Res generics?
   ...
   Does this updated request typing make sense/sound okay?

At first pass it looks like an improvement for sure 👍. I'll work on the implementation and raise any issues should they come up.

It makes sense there is some impl-specific details in there, but specifically I'd assumed that typically it would just end up calling reqType.encode or reqType.toJson, and then resType.decodeorreqType.fromJson`, depending on the wire format (binary or JSON), and defer to the internal serde logic of the encode/decode/toJson/fromJson methods.

...oh wait, I was thinking of the outputSchema, which re-creates/encodes a whole bunch of runtime metadata for the schema, and not just typeRegistry.

Okay, that makes more sense then!

Yep, our impl of request just calls to/fromJson on the req/resp respectively.

As far as our use case, it's a bit unique. We are using protobuf to define interactions between microfrontends, so we're technically not even leaving the browser for our request implementation, just sending the request to another microfrontend. While the use case is fairly unique, this generic client implementation shouldn't share any of that uniqueness.

[corwinsheahan-wf]

@stephenh I took a look at what an implementation of the below proposed typing of Rpc.request would look like to implement. For reference, here's the generated Rpc.request method definition:

  request<ReqType extends MessageType, ResType extends MessageType>(
    reqType: ReqType,
    respType: ResType,
    service: string,
    method: string,
    request: ReturnType<ReqType["create"]>,
  ): Promise<ReturnType<ResType["create"]>>;

Unfortunately I'm not able to get something that satisfies the typing on the return type in the implementation.

Here is a simple example:

We can work around this by casting the return instance with as ReturnType<RespType["create"]> or just ignoring the type error, but that doesn't feel great to require implementers to do as a default.

Another option is to use UnknownMessage as the request/response types:

interface Rpc {
  request<ReqType extends MessageType, ResType extends MessageType>(
    service: string,
    method: string,
    request: UnknownMessage,
    reqType: ReqType,
    respType: ResType,
  ): Promise<UnknownMessage>;
}

This allows the ReqType instance methods to work on the request without casting (i.e. JSON.stringify(reqType.toJSON(request)); still type checks if request is UnknownMessage, where it fails type checking if request is object). Granted, we could still just have ReturnType<ReqType["create"]> as the type here, but that is a bit verbose and I'm not sure it's actually offering any additional type checking advantages over UnknownMessage.

For the response, however, UnknownMessage doesn't give us much. We still have to cast in the generated client's method implementations, for example:

  // For service method `foo` which takes a `FooRequest` and returns a `FooResponse`:
  async foo(request: FooRequest): Promise<FooResponse> {
    return await this.rpc.request(
      this.service,
      "Foo",
      request,
      FooRequest,
      FooResponse,
    ) as FooResponse;
  }

Without the cast, we get a type error if UnknownMessage doesn't have all the methods of FooResponse (which it usually won't).

Do you have an opinion on an approach? I think I lean towards typing the request/response as UnknownMessage and generating the cast as the cast should be safe since it's all generated, and having UnknownMessage on the return type is a bit more semantic than just object.

[corwinsheahan-wf]

@stephenh Please let me know your thoughts and any changes we can make to get this PR through. Thanks!

[corwinsheahan-wf]

@stephenh Is there anything we can do to move this along? Sorry to keep pestering 😬