-
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
Change SSE response structure so that it's compatible with GraphQL SSE standard #292
base: main
Are you sure you want to change the base?
Change SSE response structure so that it's compatible with GraphQL SSE standard #292
Conversation
…E standard Change SSE response structure so that it's compatible with GraphQL SSE standard https://github.com/enisdenjo/graphql-sse/blob/master/PROTOCOL.md#next-event
@benwilson512 😉☝️ |
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.
Hey @perzanko thanks for the PR!
As you note, this is a backwards incompatible change. I think we should support some sort of flag on probably the Absinthe.Plug
plug that we can use to switch between the types of return shapes.
@perzanko are you planning on fixing this still, or should I take over to add this flag? Here for anyone who comes across this and needs a bit ugly but working workaround. plug Absinthe.Plug,
schema: ExampleApiWeb.Graphql.AdminSchema,
pubsub: ExampleApiWeb.Graphql.AbsinthePubsub,
json_codec: __MODULE__
@doc false
def encode!(%{sub: data}, config) do
"event: next\ndata: #{Jason.encode!(data, config)}"
end
@doc false
def encode!(value, config) do
Jason.encode!(value, config)
end and def publish_subscription(topic, result) do
broadcast = %{
topic: topic,
event: "subscription:data",
payload: %{
result: %{sub: result},
subscriptionId: topic
}
}
Phoenix.PubSub.local_broadcast(@pubsub, topic, broadcast)
end |
This seems to be a regression or a typo, since the code expects an `atom` but the runtime actually returns a string. ``` ** (CaseClauseError) no case clause matching: {:error, "closed"} (absinthe_plug 1.5.8) lib/absinthe/plug.ex:389: Absinthe.Plug.subscribe_loop/3 (plug 1.16.1) lib/plug.ex:173: Plug.forward/4 ``` To avoid these kind of errors, I decided to be more permissive on the errors, since any subscription should disconnect when an error occurs.
This function looks like a private one that was published by mistake
8a0af58
to
0de0846
Compare
@benwilson512 (cc @perzanko ) I updated the PR with a few fixes that we need in production (fixing also #294) 1st commit -> Fixes the match with 🙏 Sorry we didn't update the PR before, I just discovered this fork on Monday! |
@@ -369,14 +374,14 @@ defmodule Absinthe.Plug do | |||
|> subscribe_loop(topic, config) | |||
end | |||
|
|||
def subscribe_loop(conn, topic, config) do | |||
defp subscribe_loop(conn, topic, config) 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.
{:ok, conn} -> | ||
subscribe_loop(conn, topic, config) | ||
|
||
{:error, :closed} -> | ||
{:error, _} -> |
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.
ℹ️ Source code chunk/2
Description
The current payload of a subscription response in the SSE protocol looks like this.
It is simply encoded JSON with a new line.
The proposed response format is as follows:
The encoded response additionally contains an event segment
event: next
and the data is prefixed with adata:
field, indicating data segment. This structure is consistent with GraphQL over Server-Sent Events Protocol specification and official SSE standard.Reason
Many graphql clients expect a response consistent with the above standard. An example of this is graphql-mesh, which expects responses from subgraph services sent in a structured manner. Or even the highly popular client graphql-sse used in browsers also requires this format.
This is a breaking change and should be implemented with backward compatibility. Perhaps introducing an additional configuration option or an option to override the response format would be appropriate in this case, perhaps even more appropriate.