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

Return promises from service client methods #59

Closed
bhollis opened this issue May 1, 2018 · 14 comments
Closed

Return promises from service client methods #59

bhollis opened this issue May 1, 2018 · 14 comments

Comments

@bhollis
Copy link

bhollis commented May 1, 2018

How would folks feel about making the callback parameter on the generated service clients optional, and returning a native Promise when it's not provided? I'm happy to send a PR if there's interest.

@jonnyreeves
Copy link
Contributor

jonnyreeves commented May 2, 2018

I'm on the fence.

Supporting this change is the evidence that Promises replacing callbacks appears to be the future, see this relevant discussion on StackOverflow: https://stackoverflow.com/questions/30299613/why-does-nodejs-not-use-promise-for-the-readfile-api

My concern with this approach is that we make a breaking change from the official nodejs gRPC API which returns a ClientUnaryCall instance which can be used to abort requests and access grpc Metadata on successful calls (note this API has yet to be implemented in ts-protoc-gen)

If we return a Promise from unary invocations then we will also need to provide an API for these use cases. We will also need to be comfortable with the fact that users who adopt grpc-web as their transport layer may face a difficult migration to the official nodejs gRPC API when it also supports Browser based environments (see grpc/grpc-web#91) and/if the grpc-web project is deprecated.

Thoughts and suggestions welcome 😀

@easyCZ
Copy link
Contributor

easyCZ commented May 2, 2018

@bhollis How would you handle the case where a request returns a stream of messages but you want to react to them immediately when they arrive rather than when the whole stream finishes? This is one of the reasons promises are not particularly suitable for this job as we need to be able to return a value multiple times which Promises do not allow.

@bhollis
Copy link
Author

bhollis commented May 2, 2018

That's fair - Observables would be better, but they're only a proposal right now. I was mostly thinking it'd be nice for unary calls, but I can always build my own Promise/Observable wrappers around the callback-driven code.

@jonnyreeves
Copy link
Contributor

Just bumping this as there has been no further response from the community.

My current thinking is that we should mirror the node-grpc API for now to make migration pain free.

If you would like to see a Promise/Observable API then I would suggest we either:

  1. Put it behind a feature flag so users of ts-protoc-gen have to opt into it.
  2. Create a new package/repo which wraps the current API (this could then be used for both grpc-web and node-grpc) and we can mention it in the README

Let me know your thoughts

@manu-st
Copy link

manu-st commented May 18, 2018

We have been using https://github.com/bojand/grpc-caller to promisify the GRPC calls. Having ts-protoc-gen generates a typescript definition with Promises would help. At the moment, we have to hardcode it manually which requires more updates than we want to when modifying the proto files.

@jonnyreeves
Copy link
Contributor

@manu-st I'd be willing to consider a patch which generates promises, however it would need to be behind a protoc-gen feature flag

@stale
Copy link

stale bot commented Aug 22, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Aug 22, 2018
@stale stale bot closed this as completed Aug 29, 2018
@clanstyles
Copy link

Bump?

@jonnyreeves
Copy link
Contributor

jonnyreeves commented Jan 10, 2019

Thanks for bumping this issue @clanstyles, let me try and bring some closure.

As discussed in a handful of other issues, it's my current belief that ts-protoc-gen should be built around callbacks and event handlers. Other abstractions on top of these concepts (or: Promises and Observables) can be created by the community, presumably as a wrapper, ie:

promisify(myClient.doUnary, myMetadata)
  .then(response => /* … */)
  .catch(err => /* … */)

My rationale is that I would like to keep ts-protoc-gen's API surface area as small as possible to make it easier to maintain.

@chin2km
Copy link

chin2km commented Aug 19, 2020

can this be reopend?

@colemars
Copy link

Maybe we could re-open this discussion now that the official client supports promise clients?

Obviously feature parity isn't a req but would be a shame to see a departure of new users over lower level usability stuff like this.

@rhyslbw
Copy link

rhyslbw commented Apr 13, 2021

Maybe we could re-open this discussion now that the official client supports promise clients?

Agree

@cranesandcaff
Copy link

Big agree. Since the official client supports promises we should generate the types.

@god-of-javascript
Copy link

2023, still no promise client :D

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

No branches or pull requests

10 participants