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

Allow subscribe function to return a Promise #895

Closed
NeoPhi opened this issue Jun 4, 2017 · 7 comments
Closed

Allow subscribe function to return a Promise #895

NeoPhi opened this issue Jun 4, 2017 · 7 comments
Assignees

Comments

@NeoPhi
Copy link

NeoPhi commented Jun 4, 2017

The current subscribe resolve logic does not permit returning a Promise that would resolve to an AsyncIterable. The use case is running asynchronous business logic to determine if the subscription should be allowed or additional information needed to return a valid AsyncIterable. While it would be possible to merge that logic into a high order iterable, I feel the ability to return a Promise would better match the existing GraphQL semantics.

@Urigo
Copy link
Contributor

Urigo commented Jun 12, 2017

So if I understand correctly, the use case for adding additional API is in order to reject the subscription at the first place.
So there will never be a use case for the Promise to be resolved..
To achieve that today with a single API, you would just need to throw the AsyncIterator.
In this comment there are a few links for implementations of AsyncIterator, one of them around Promises.

Personally I feel that adding another API would just create more mess but I see that there are a lot of likes to your suggestion so I would love to be persuaded differently.

@NeoPhi
Copy link
Author

NeoPhi commented Jun 12, 2017

We have 2 use cases. The first is the error use case. However, our more common use case is having to lookup information based on the subscription parameters to determine what channels to subscribe to.

I don't see this as introducing a new API. This is just allowing the AsyncIterable to be returned at a later time via a Promise instead of immediately.

@robzhu robzhu self-assigned this Jun 12, 2017
@robzhu
Copy link
Contributor

robzhu commented Jun 12, 2017

@NeoPhi thanks for filing this issue. Can you help me understand your use-case in more depth? What's wrong with handling an AsyncIterator that throws on the first invocation of next()? We've been talking through a few options, and I think we have at least one proposal on the table that addresses your point:

subscribe() will return a Promise<AsyncIterable | GraphQLResponse | Exception>. GraphQLResponse will capture 400-class errors that contain messages that might be useful to the client. Exception will capture 500-class errors that the client cannot fix (for example, underlying event-bus errors). AsyncIterable would only be returned if the source stream was successfully constructed and the server has a confidence to expect future publishes on the source stream.

It's certainly more verbose of an API, but it explicitly captures all the success/error cases. Thoughts?

@NeoPhi
Copy link
Author

NeoPhi commented Jun 12, 2017

The primary use case is that we have a subscription in which the client passes in a couple of arguments. We have asynchronous business logic that runs which translates those arguments into a set of channel names that an AsyncIterable then listens on, hence our desire to be able to have the subscribe() function return a Promise. Your proposal of Promise<AsyncIterable | GraphQLResponse | Exception> would address this.

@jperl
Copy link

jperl commented Jun 19, 2017

I just ran into this as well. The use case is to validate the user has permission to subscribe based on the arguments provided -- before starting the subscription.

@robzhu
Copy link
Contributor

robzhu commented Jun 19, 2017

@jperl, @NeoPhi, I submitted a PR to address this issue, please take a look and let me know if this it fixes the problem for you: #918

@jperl
Copy link

jperl commented Jun 19, 2017

It does. Thank you!

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

4 participants