-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
wip: RPC server, client, bidirectional streaming #1115
base: master
Are you sure you want to change the base?
Conversation
Looking good to me from a first glimpse, I like how straight forward this is and that it doesn't add too much code to the library :). Still, would love to have some opinions by people who'd like to use streaming. |
If somebody is interested I created working example with React Native + SwiftGrpc streaming https://github.com/trackforce/react-native-grpc-starter using my fork Protobuf.js version. |
Adding my 2 cents here after bumping into the same questions reported in #784 @anjmao's suggested API is definitely an improvement. To be familiar with existing interfaces and to match a probable underlying wire API, I'd rather see an API that is closer to nodejs's Stream — or even use directly Stream's interfaces (in object mode). // server stream
serverStream = () => stream.Readable<ServerStreamMessage>
const stream = service.serverStream();
stream.on('data', (serverStreamMessage) => {..})
stream.on('end', () => {})
// bi-directional stream
bidiStream = () => stream.Duplex<ServerStreamMessage, ClientStreamMessage>
const stream = service.bidiStream();
stream.on('data', (serverStreamMessage) => {..})
stream.on('end', () => {})
stream.write({ message: 'foo'})
stream.end() Alternatively, we could use APIs that are closer to web's streams but i know less these APIs. |
export interface RPCServerStream<T> { | ||
on(evt: 'recv', fn: (data: T) => any, ctx?: any): util.EventEmitter; | ||
on(evt: 'error', fn: (data: any) => any, ctx?: any): util.EventEmitter; | ||
on(evt: 'end', fn: (data: any) => any, ctx?: any): util.EventEmitter; |
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.
Can a client end a server stream before the stream is actually ended by the server itself?
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.
Yes, client which sends end event is indicating to server that it don't want to continue.
@alexstrat Ideally it would be great to have Duplex, Readable, Writable interfaces from node.js, but the problem I see is that it would be hard to use it for browsers and mobile (react native, native script) since stream module works only on node.js (correct me if I'm wrong). |
I discovered this PR when looking for a soloution to implement a The protobuf.js generated api is really nice, and this PR adds stream support - which is fantastic. I've built a prototype that uses your fork https://github.com/trackforce/protobuf.js#rpc-streaming and implements a $protobuf.RPCHandler using a grpc.Client directly. I've only implemented unaryCall and serverStreamCall but the other two should be trivial. Here's how it looks :
Its pretty straight forward since its based on https://github.com/trackforce/react-native-grpc-starter/blob/master/mobile/src/services/grpc/grpcCore.ts To use you can do the following :
Is there any chance of this PR and an addition like the above being accepted into protobuf.js ? |
One issue I found with the PR is the typescript signiture of rpc.Service.create, and constructor needs to be changed to accept |
Or another option could be to change RPCImpl as per : |
@cameronbraid Are you using nodejs grpc client? In such case I would rather stick to it since it already generates streaming method stubs for you :) |
Is there any traction on this? |
We've been using this PR to add types to For reference, here's some links to our code: |
It would be great if this could get merged. I've had to roll my own version of this for https://github.com/ipfs/js-ipfs but I'd much rather support was in protobuf.js for this. |
This PR will add possibility to implement server, client, bidirectional streams.
Server stream:
Client stream:
Bidirectional stream:
It is possible to pass new
RPCHandler
implementation.Some examples from my actual implementation (React Native + SwiftGrpc) using underlying EvenEmitter.