-
Notifications
You must be signed in to change notification settings - Fork 167
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
Add support for brokered subscriptions #217
Conversation
When configuration `:websocket_subscriptions` is set to false, don't respond to subscriptions with a chunked event stream and enter a loop sending subscription updates; instead, send the subscription ID to the client and end the request.
|> put_resp_header("content-type", "text/event-stream") | ||
|> send_chunked(200) | ||
|> subscribe_loop(topic, config) | ||
if config[:websocket_subscriptions] do |
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.
This is the wrong option name I think. What Absinthe.Plug is doing here is NOT websockets, it's long polling. Websockets is handled by absinthe phoenix, and happens when a subscription is sent over a pre-established websocket channel. Everything in Absinthe.Plug is HTTP.
However I get what you're going for. You want to make it possible to simply return a subscription handle, and indicate that the subscription was registered elsewhere. This is a need we'll honestly have for both Absinthe.Plug and Absinthe.Phoenix, cause in theory nothing should stop you from submitting a subscription document over websocket, but having that still set something up in MQTT.
However, I think this needs to be possible on a request by request basis, instead of as a flag that categorically sets a given plug one way or another. Not entirely sure how that option would be specified however. It seems like it could be something that Absinthe itself indicates in the return value from a subscribe call.
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.
Ok, sure, that would work too. How should we do this -- do you want to think more about it and let me know how you want to structure it? Or were you planning to build something different yourself?
Closing this since it's grown stale and there isn't a clear path forward. Please update us if anything changes! |
Previously subscriptions over HTTP were handled through SSE. This commits adds a callback so the user can choose to handle it differently. It's modelled after how [graphl-ruby](https://graphql-ruby.org/subscriptions/pusher_implementation.html) does it. I think this gives the flexibility to the user to implement their own subscription broker implementation. It gives access to the http request to be changed in a way the user sees fit. There's still some work to be done on the Absinthe side for a user, though this can be done in userland by replacing some phases. `absinthe_phoenix` could use a similar callback. See absinthe-graphql#217 for previous work.
This is work in progress, for now I'm putting it up here only to start the conversation. This change goes along with absinthe-graphql/absinthe#776
Like the changes in the other PR, this one is necessary to support message brokers for delivering subscription updates because obviously, updates aren't being sent through the WebSocket in that case (they are sent out-of-band through the broker instead.)
One question is how to encode the subscription ID in the HTTP response. GraphQL Ruby delivers the string
null
in the body with Content-Typeapplication/json
and sets the subscription ID in theX-Subscription-ID
header. At first I wanted to do it the same way for consistency, but doing so doesn't work well with one of my clients, Apollo iOS. It chokes on thenull
(it wants a JSON body that is an object or an array), and more importantly it doesn't provide a good way to access the GraphQL response headers.As far as I know, there is no standard for this and the GraphQL Ruby guys just made something up (?). So unless there are any objections I'd prefer to just put the ID in the body instead.
(I've also come to think that it's slightly more consistent because the body is where any errors would come in, so by putting the ID in the body you don't have to look in two different places.)
Thoughts?