fix: grpc-web issue#239 where client streaming was producing incorr…

[samelie]

fixing grpc-web issue#239 where client streaming was producing incorect requestType

Based on this issue #329

Hopefully this fix isn't overly naive

Thanks for all the hardwork putting this together; it's really cool

[stephenh]

I think this fixes the compile error, but in a pretty indirect way, right? Like the *.proto file is saying "the client can submit a stream of messages" and this is basically saying "we won't do that"...which, compiles, but does it actually work?

Like looking at the grpc-web examples, if a method is stream Cred we can't just write a Cred and call it good, we need to tell the server "we could have sent more Creds but we're not", i.e. by calling finishSend:

https://github.com/improbable-eng/grpc-web/blob/master/client/grpc-web/src/client.ts#L304

So this is less of "just fix the compile error" and more that I think we need to actually support client-to-server / request streaming. Until then, ts-proto just won't work with *.proto files that use stream Cred.

Maybe we could like skip putting those methods in the output until it's supported, so that if you have a *.proto file that has those, but you don't need to call them, you'd at least get output that would let you use the other methods.

[samelie]

Yes, you are right, this does just fix the compile error. I don't have rpc services with a backend setup right now to test; though; from what improbable says , it's not supported. + cheers for the pointer re finishSend

Omitting entirely the stream Cred methods sounds like a good plan; stay tuned

[stephenh]

Fwiw @samelie I pushed a "re-codegen" commit to this PR that undoes a lot of *.bin file changes you'd had in here that come from having different versions of protoc. Ideally you shouldn't need to include those changes in the PR.

[samelie]

Hey @stephenh hopefully this approach is better. the goal of not generating rpc ChangeUserSettingsStream (stream Cred) returns (stream DashUserSettingsState) {} has been accomplished for grpc-web

Also, off topic, any thoughts on bumping rxjs to 7.3.0? Saw nestjs is on it too. v7 has native ts support and is really good

[boukeversteegh]

Hi @samelie Is this PR still under development?

[samelie]

@stephenh @boukeversteegh

I'm really not familiar with client streaming in grpc-web. I saw a post on their issues about it saying you needed the websocket transport. Makes sense.
One day I'll do an experiment (we like to say) , but I've been holding out for streamsAPI https://github.com/improbable-eng/grpc-web#client-side-streaming
All that to say I don't have an easy way to setup a websockets and write the rxjs push code we'd need.

For me, this wouldn't be an issue (am fine with it not working), however I'm in a work situation where we need to run tsc on the generated output that produces errors like these for clientStreaming

  error TS1005: '(' expected.
         Observable<WriteVerifyFileRequest>.fromPartial(request),

tsc is really annoying in the way there no way to silence errors. For me since this is all in CI, this is a problem as I can't swallow them either (bash noob?)

The easiest solution in the vein of here be dragons is just to treat clientSide in the case of grpc-web && !nestJs like a normal Promise rpc

I'd be happy to propose my forked fix for this that changes requestType in types/types.ts to

export function requestType(ctx: Context, methodDesc: MethodDescriptorProto): Code {
  let typeName = rawRequestType(ctx, methodDesc);
  if (methodDesc.clientStreaming && ctx.options.nestJs) {
    return code`${imp('Observable@rxjs')}<${typeName}>`;
  }
  return typeName;
}

Less magical solutions would be better. Perhaps some ctx option to ignore all clientStreaming?