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

add support for graphql-core 3 to graphql_ws.aiohttp #43

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

hoefling
Copy link

@hoefling hoefling commented Apr 2, 2020

Signed-off-by: oleg.hoefling [email protected]

This PR adds support for graphql-core>=3 to graphql.aiohttp.AiohttpSubscriptionServer. Note that this will break the rest of the BaseSubscriptionServer implementations! Unfortunately, I am not familiar with Sanic or gevent to adapt those too.

This PR violates against the PR submission rule

3 The pull request should work for Python 2.6, 2.7, 3.3, 3.4 and 3.5, and for PyPy. Check https://travis-ci.org/graphql-python/graphql_ws/pull_requests and make sure that the tests pass for all supported Python versions.

as graphql-core>=3 is compatible with Python 3.6 onwards. Also, all of the Python versions listed have reached EOL anyway (except Python 3.5 which will reach EOL in September).

…uring on travis, add test dependencies to requirements_dev.txt, add status badges to readme

Signed-off-by: oleg.hoefling <[email protected]>
Signed-off-by: oleg.hoefling <[email protected]>
@codecov-io
Copy link

codecov-io commented Apr 2, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@8f36f53). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #43   +/-   ##
=========================================
  Coverage          ?   35.58%           
=========================================
  Files             ?        8           
  Lines             ?      489           
  Branches          ?       54           
=========================================
  Hits              ?      174           
  Misses            ?      303           
  Partials          ?       12           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f36f53...0b2efdc. Read the comment docs.

Signed-off-by: oleg.hoefling <[email protected]>
@bueltan
Copy link

bueltan commented Jun 12, 2020

Thank for sharing , it's really appreciated.

error_type,
error_payload
)
return self.send_message(connection_context, op_id, error_type, error_payload)

def unsubscribe(self, connection_context, op_id):
if connection_context.has_operation(op_id):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the connection_context.get_operation(op_id).dispose() seems to depend on the old rxpy-dependency that has been removed from graphql-core and is effectively removed in this branch by removing setup_observable_extension.

this causes an unhandled exception on unsubscribe

@dopry
Copy link

dopry commented Jan 6, 2023

@hoefling do you have any plans to update this PR?

The wider ecosystem seems to be adopting graphql-core 3 and the version constraint here is bound to start generating issues. I'm experiencing issues in the examples for https://github.com/torchbox/wagtail-grapple since we've updated to django-graphene >= 3.

@hoefling
Copy link
Author

@dopry let me take a look at it, it's been a while now :-)

@erikwrede
Copy link
Member

erikwrede commented Jan 11, 2023

Any objections against just integrating this into GQL-Server? Websocket-Based Subscriptions have become a common pattern in GQL and an integration would reduce the amount of duplicate code and management effort.

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

Successfully merging this pull request may close these issues.

6 participants