-
Notifications
You must be signed in to change notification settings - Fork 178
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
Subscriptions: SSE distinct connection support #1195
base: main
Are you sure you want to change the base?
Conversation
Hey, I wanted to see if someone has any feedback for me on this. Thanks! |
Apologies @danplischke but I am no longer with Mirumee. Poking @DamianCzajkowski |
Hi @danplischke, thank you for the contribution. We are currently paced with commercial projects, but I am searching for a solution for you and the rest of the new PRs. Are you on the time pressure? |
Hi @mociepka Let me know if I can somehow make this easier on you. Happy to help. |
If this is to be merged into Ariadne, it needs to be done as a separate handler. It will also need to have a documentation explaining what it is and how to use it (linking to graphql-sse repo with comment "go and read it" won't cut it as a doc). However, a question needs to be answered first (by @mociepka) if there will be a commitment to maintaining this after it gets merged. Because when something gets into Ariadne it means we are taking 100% responsibility for it and we will fix it when bugs happen or spec changes and client libs are suddenly broke. Is the GraphQL SSE even versioned or is it a living standard where whatever is there in latest release is the spec? If answer to those two questions is Also, making this part of Ariadne will mean eventual fixes or updates in SSE support will be tied to Ariadne releases. I'll drop suggestions to the actual code in this PR later, but at quick glance I wonder how does the protocol that uses line terminator as separator handle multiline characters in result payloads? |
Thank you, @rafalp, for outlining the risks and opportunities. SSE as a transport layer looks like a valuable addition, and I am happy to welcome it. I understand that SSE is not yet standardized, but it originates from @enisdenjo, and (correct me if I am wrong, @rafalp) we use his spec for graphql-ws. Addressing @danplischke's questions:
Looking forward to your thoughts! |
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.
Please cleanup imports in modules, make magic methods of your classes on top of those classes bodies (currently they are mixed with rest of logic) and then group both class definitions and their methods in order of their usage in business logic.
Also move your changes to a dedicated handler importable from ariadne.contrib.sse
ariadne/asgi/handlers/http.py
Outdated
List, | ||
Union, | ||
) | ||
from typing import Type |
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.
Imports will need cleaning up
ariadne/asgi/handlers/http.py
Outdated
create_task_group, | ||
) | ||
from graphql import DocumentNode | ||
from graphql import MiddlewareManager |
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.
Ditto
This work needs to be split into a separate handler, then it should accept configuration options as
I am afraid I can't. This is also finnicky in standard impl we are using for regular subscriptions. |
…sts to fix Starlette TestClient error, manually set anyio dependency lower for python 3.8 integration tests as current version is not available for 3.8
Thanks @rafalp and @mociepka for your feedback! I have
Just fyi: I had to update the versions in the requirements.txt in tests_integrations/fastapi (using pip compile) due to an error in the TestClient in the old starlette version which was used in the workflows. Afterwards, I had to manually set the anyio version lower for the Python 3.8 workflow to run through. Feel free to change this. @rafalp concerning your question about the line terminator as separator: import json
v = json.dumps("valu\n\n\r\ne")
print([ord(b) for b in v])
print([ord(b) for b in "valu\n\n\r\ne"]) |
This PR introduces a first implementation of subscriptions over Server-Sent Events (SSE) based on the definition in graphql-sse. This provides an alternative to the existing WebSocket support, which is particularly useful in scenarios where WebSocket connections may be restricted (e.g., corporate proxies).
Key Features / Considerations
It would be great to get some feedback from you on:
Looking forward to your feedback!
Thanks in advance!