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

Server streaming method outputs observable interface but promise client implementation #311

Open
protango opened this issue Jun 7, 2021 · 2 comments

Comments

@protango
Copy link
Contributor

protango commented Jun 7, 2021

Consider the following proto

service MathService {
    rpc Add(MathServiceAddRequest) returns (stream MathServiceAddResponse) {}
}

Running ts-proto on this will generate the following:

export interface MathService {
  Add(request: MathServiceAddRequest): Observable<MathServiceAddResponse>;
}
export class MathServiceClientImpl implements MathService {
  // ...
  Add(request: MathServiceAddRequest): Promise<MathServiceAddResponse> {
    // ...
  }
}

Notice that the MathServiceClientImpl.Add method returns a Promise, but the interface specifies it should be an Observable, thus the following error is generated by typescript:

error TS2416: Property 'Add' in type 'MathServiceClientImpl' is not assignable to the same property in base type 'MathService'.
@stephenh
Copy link
Owner

stephenh commented Jun 7, 2021

Huh, thanks for the report; I kinda thought this code should already handle that:

options.returnObservable || methodDesc.serverStreaming

function generateRpcMethod(ctx: Context, serviceDesc: ServiceDescriptorProto, methodDesc: MethodDescriptorProto) {
  const { options, utils } = ctx;
  const inputType = requestType(ctx, methodDesc);
  const partialInputType = code`${utils.DeepPartial}<${inputType}>`;
  const returns =
    options.returnObservable || methodDesc.serverStreaming
      ? responseObservable(ctx, methodDesc)
      : responsePromise(ctx, methodDesc);

And there's an integration test that I'm pretty sure is basically the same proto here:

https://github.com/stephenh/ts-proto/blob/main/integration/grpc-web/example.ts#L750

Can you tell what's different in your setup vs. that integration test?

@Jille
Copy link
Contributor

Jille commented Oct 26, 2021

generate-services.ts (not grpc-web) had a bug here where it missed the check for options.returnObservable || methodDesc.serverStreaming.

I fixed it with the first commit of #373.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants