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

New module to support Django Channels v2 #18

Merged
merged 85 commits into from
Mar 29, 2021

Conversation

SmileyChris
Copy link
Contributor

Rather than modify the existing channels v1 implementation, this PR adds in a separate package (graphql.django) to handle channels v2.

It leverages some of the implementation of other modules (there is a bunch of similar looking code that probably needs to be refactored into a common base class really, but that's outside of scope).

The example should work out of the box from within the examples/django_channels2/ path with a pipenv install, ./manage.py runserver

@colanconnon
Copy link
Contributor

@SmileyChris this looks good to me

@arif-hanif
Copy link

arif-hanif commented Oct 22, 2018

@SmileyChris do you have any recommendation for running behind nginx in production?

I tested this PR out and it worked out well for me. I believe i have i have figured out a configuration to get on to production with gunicorn/uvicorn/nginx

@SmileyChris
Copy link
Contributor Author

@syrusakbary Is there anything which I can do to make it easier for you to feel confident in including this code?

@xeor
Copy link

xeor commented Dec 28, 2018

Any news on when we can get this merged? The check fails because of missing pathlib in 27, but it doesn't look like that is this commits fault..?

@Manviel
Copy link

Manviel commented Feb 14, 2019

I fork this https://pypi.org/project/graphql-ws-django/ and does not work with docker

@gotexis
Copy link

gotexis commented Mar 24, 2019

@SmileyChris

I checked your fork which seems promising.

Since this is not yet merged, I tried to install your branch by pipenv / pip, yet result in WinErrors(directories not found). Is it installable?

I hate to use the code by copying the code into my own project :)

@Manviel Is your pypi release based on this fork? I can just install from there?

Update: I installed from @Manviel pypi release however, testing via /graphiql/, the websocket keep getting bad handshake (500) and disconnect repeatedly. Not sure how to debug since there isn't any info related outputted to my PyCharm console.

INFO WebSocket HANDSHAKING /subscriptions [127.0.0.1:63069]
INFO WebSocket DISCONNECT /subscriptions [127.0.0.1:63069]
INFO WebSocket HANDSHAKING /subscriptions [127.0.0.1:63078]
INFO WebSocket DISCONNECT /subscriptions [127.0.0.1:63078]

Also,

from channels.generic.websockets import JsonWebsocketConsumer

should be

from channels.generic.websocket import JsonWebsocketConsumer

@SmileyChris
Copy link
Contributor Author

@gotexis Well I'm using my branch in a pre-production project just fine (granted, not on Windows).

@gotexis
Copy link

gotexis commented Mar 24, 2019

@gotexis Well I'm using my branch in a pre-production project just fine (granted, not on Windows).

My development is on Windows T T

@slorg1
Copy link

slorg1 commented Dec 14, 2020

@Cito Thank you for your comment. I just spend ~4h getting this working (PyPI is out of date, the doc is all over the place) and @SmileyChris 's code is the next best thing for anyone using graphene Django today.

But the other problem is that obviously nobody has the time to really maintain and manage this project. Maybe bring this up in the Graphene slack, and discuss who would be willing to become a maintainer and move this project forward. Also, note that graphql-ws is currently still based on GraphQL-core v2 which is deprecated. The best way to move forward would be to update to GraphQL-core v3 before adding new features.

I understand your concern and from my point of view, that is what Chris did. He moved the bucket forward for us towards having a more complete coverage and, so far, his code is "up-to-date" and working (if you do not use the outdated PyPI).

To you point: who knows what the future is made of?
In the meantime, it would make a lot of sense to merge his PR in as in the meantime as there is a lot of confusion around and no better solution. Getting his PR in gets us (the community) a solid base to keep pushing with where not folding it into this repo is blocking progress, IMHO.

Final thought, I do not know if you realize it but this repro is listed on the graphene python website as the gold standard: https://docs.graphene-python.org/projects/django/en/latest/subscriptions/
(that is how I found it)

Please help us by approving this PR: "A journey of a thousand miles begins with a single step" :-)

Thank you!!

Copy link

@slorg1 slorg1 left a comment

Choose a reason for hiding this comment

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

From testing it, this works. I gave you a couple of pointers for the doc of issues I ran into.

Thank you for doing this!! Very helpful!

Gevent
~~~~~~
GRAPHENE = {
'SCHEMA': 'yourproject.schema.schema'
Copy link

Choose a reason for hiding this comment

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

this tripped me up: you should mention in the doc that it is required for the channel work to function where it is technically not needed for graphene to work.

I had to add to my existing project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is under the django channels configuration section, so it would imply that it's the setup required for channels, wouldn't it? Maybe I'm not understanding - please elaborate

Copy link

@slorg1 slorg1 Dec 15, 2020

Choose a reason for hiding this comment

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

This is a setup I found on the Django-graphene website: https://docs.graphene-python.org/projects/django/en/latest/tutorial-plain/#update-settings

It is "new"ish and, so far, works without it unless you activate the subscriptions (not sure why).

I point it out for anyone like me who has a working project setup before the django-graphene update that works as-is: subscriptions will not work and the error is not that clear. To be clear, I am using the latest stable version of the django-graphene though. mystery mystery

It is a suggestion because I chased my tail for ~20mins looking at an opaque error ;-)

"default": {
"BACKEND": "asgiref.inmemory.ChannelLayer",
"ROUTING": "django_subscriptions.urls.channel_routing",
},
Copy link

Choose a reason for hiding this comment

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

the example is out-dated. They retired this. I used:

    "default": {"BACKEND": "channels.layers.InMemoryChannelLayer",},
}

instead, and that works now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is in the outdated "old django" section anyway, is that backend configuration really retired in channels v1?

Copy link

Choose a reason for hiding this comment

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

From what I found asgiref retired their in-memory channel layer. So, if you get asgiref, it no longer has the class listed in the doc :-/

about the ROUTING part, idk, I only tested channels V2 and having that key with V2 was crashing (actively rejected, not ignored).

Copy link
Member

@Cito Cito left a comment

Choose a reason for hiding this comment

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

Sorry, I don't have the time to review this. Looks you're making good progress, so approving this to no block you. But as I mentioned, please discuss on the Graphene Slack how this project can be managed, who can become responsible for this project and do the reviews and approvals in the future. As much as I would like to help, I really don't have the time and would like to pass this off to somebody else.

Copy link
Contributor Author

@SmileyChris SmileyChris left a comment

Choose a reason for hiding this comment

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

Thanks for the review, @slorg1!

Gevent
~~~~~~
GRAPHENE = {
'SCHEMA': 'yourproject.schema.schema'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is under the django channels configuration section, so it would imply that it's the setup required for channels, wouldn't it? Maybe I'm not understanding - please elaborate

@SmileyChris SmileyChris merged commit 366d1aa into graphql-python:master Mar 29, 2021
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.

None yet

8 participants