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

PromiseClient is missing all the streaming methods #300

Closed
juanjoDiaz opened this issue Sep 27, 2018 · 6 comments
Closed

PromiseClient is missing all the streaming methods #300

juanjoDiaz opened this issue Sep 27, 2018 · 6 comments

Comments

@juanjoDiaz
Copy link
Contributor

It seems that only Unary calls are added to the PromiseClient.
That means that, if you combine streaming and unary calls you either stick to the callback-based client or you need to have both clients.

Any reason for that? Would a PR to add stream calls to the PromiseClient be welcomed?

@stanley-cheung
Copy link
Collaborator

PromiseClient is more of an internal experimental thing right now that's not supported here yet. (Hence the complete lack of documentation of its existence here).

So please don't use it yet :) We are planning to work on this but unlikely to be available in the next quarter or so.

@stanley-cheung
Copy link
Collaborator

Actually I'd like to use this thread as an API proposal discussion: What should a Promise-based API look like for streaming methods?

@juanjoDiaz
Copy link
Contributor Author

I think Callback vs Promise is a debate for async request-response operations.

Talking about streams of data, the Callback vs Promise doesn't make any sense.
So I think that streaming call should return exactly the same for both clients.
The current version seems to basically match Node.js' ReadableStream and that sounds like a great option to me. Once the bidirection stream is added it could match Node's Duplex Stream.

In fact, I think that there should be only promises and callbacks should die 🙂
I can't see any advantage of callbacks except the fact that people are used to them.

In that sense, I'd question whether there should be 2 separate clients. It would be trivial to modify the existing generator so req-res calls return a promise by the default or use a callback if a callback is passed.

@stanley-cheung
Copy link
Collaborator

Oh I think I misunderstood your original comment of "add stream calls to the PromiseClient" then. So you were saying that currently, in the undocumented PromiseClient, we just aren't outputing the streaming methods at all, and that we should. And there is no need to design a Promise-based API for streaming calls. Then yes, that was my thought as well. We would certainly like to keep the API as close to the Node-based ReadableStream as possible.

About Callback vs Promise, I will punt on this argument for now :)

@juanjoDiaz
Copy link
Contributor Author

Correct! My initial question was that the undocumented PromiseClient is missing those methods.
It should be trivial to add them there just as they are, that's why I asked if a PR would be welcome.

@stanley-cheung
Copy link
Collaborator

Yes a PR would be welcome :) Thanks! Sorry for the confusion.

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

2 participants