-
Notifications
You must be signed in to change notification settings - Fork 172
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
Correct generated client, bidi stream method stubs #131
Conversation
c63b3d2
to
f2c13f4
Compare
@jonny-improbable should be a simple merge I hope. |
f2c13f4
to
60d257f
Compare
I've found another problem, lets wait with merging this please. |
@jonny-improbable ok there we go. |
src/service/grpcweb.ts
Outdated
@@ -222,13 +222,13 @@ function generateTypescriptDefinition(fileDescriptor: FileDescriptorProto, expor | |||
printer.printIndentedLn(`on(type: 'end', handler: () => void): RequestStream<T>;`); | |||
printer.printIndentedLn(`on(type: 'status', handler: (status: Status) => void): RequestStream<T>;`); | |||
printer.printLn(`}`); | |||
printer.printLn(`interface BidirectionalStream<T> {`); | |||
printer.printIndentedLn(`write(message: T): BidirectionalStream<T>;`); | |||
printer.printLn(`interface BidirectionalStream<Req, Res> {`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to know opinions on the Generic type variables here, but I can't use <R, R>
, and I think <T, V>
is just unclear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe <ReqT, ResT>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep ReqT and ResT work for me
My formatter has formatted the files I've touched, I can try and revert those changes and keep the edits minimal if required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this up, some pretty silly bugs in there from me 😋
This hints that the project would benefit from a proper integration test against an actual grpc-web server...
If you wouldn't mind reverting the whitespace changes then I'll be happy to merge
Thanks again
86d4fe1
to
5187c4e
Compare
5187c4e
to
93c4444
Compare
@jonnyreeves should be good now (should I @jonnyreeves or @jonny-improbable?). |
Both work 😁 |
Published as (0.7.8-pre.217169b) |
…e-eng#131)" This reverts commit 0a44695.
It appears we were using the responseType instead of the requestType when creating the stream.
Fixes #132, #133