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 basic Channels integration #1407

Merged

Conversation

LucidDan
Copy link
Contributor

@LucidDan LucidDan commented Nov 5, 2021

Description

This is a draft PR implementing #1408 - so far it has pretty good coverage of the websocket side of things, but I need to finish:

  • the HTTP consumer
  • the high level API for the Channels Layers, to make it easy to work with, especially if you're in a sync Django view.
  • some more unit tests, especially in relation to the http consumer

I've just rewritten the PR to make the commits a little tidier, and also to clean up the implementation of the consumers which I wasn't happy with.

I'm currently treating it as separate from the Django integration, with its own package path in strawberry.channels - I can imagine before long all the integrations might get consolidated in a common namespace but for now I'm sticking with the same style as FastAPI, aiohttp, etc.

The most likely use case for this will be using it to provide subscriptions support on Django, with the channels layer offering a pretty nice way to integrate it with the rest of a Django project, even if it is a mostly synchronous one.

I've been building some tests, mostly re-using tests from the other GraphQL integrations (I have been wondering as well, do we really need separate test suites for each integration when it comes to websockets? Seems like something that could be done once for all of them, except perhaps for the issue that the context isn't implemented consitently?)

The Channels integration is pretty small and simple to be honest - the biggest part of this is likely to actually be a good example and some documentation (like a how-to) on how this can actually be used. I have something in progress for that, which I figure I should PR to the strawberry/examples repo...

The main thing I'm working on now that the websockets consumer implementation is fairly stable is to add the Layers high level API to make it easy to integrate with a Django app. See the issue ticket for thoughts on this.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@LucidDan LucidDan mentioned this pull request Nov 7, 2021
7 tasks
@LucidDan LucidDan force-pushed the channels-subscriptions-consumer branch from 860d7f2 to b496ded Compare November 7, 2021 11:39
@patrick91 patrick91 self-requested a review November 15, 2021 18:58
@patrick91
Copy link
Member

@LucidDan hey! thanks for this PR 😊 I'm going to take a look at it during the weekend :)

I've been building some tests, mostly re-using tests from the other GraphQL integrations (I have been wondering as well, do we really need separate test suites for each integration when it comes to websockets? Seems like something that could be done once for all of them, except perhaps for the issue that the context isn't implemented consitently?)

that's a good point, but I'd personally feel more confident in having one test for each integration 😊

Add Channels optional dependency and an Extra package for channels
Complete implementation for websockets, partial implementation for HTTP (multipart/form-data still pending)
These unit tests are mostly copied over from FastAPI, and certainly aren't comprehensive or covering all the Channels-specific cases. However, they provide a reasonably good level of coverage for actual protocol behaviour.

The HTTP consumer does not yet have any unit tests.
@patrick91 patrick91 force-pushed the channels-subscriptions-consumer branch from b496ded to 098778d Compare January 14, 2022 20:16
@codecov
Copy link

codecov bot commented Jan 14, 2022

Codecov Report

Merging #1407 (fdd6988) into main (091dd22) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1407      +/-   ##
==========================================
+ Coverage   98.09%   98.19%   +0.10%     
==========================================
  Files         152      160       +8     
  Lines        6095     6381     +286     
  Branches     1160     1200      +40     
==========================================
+ Hits         5979     6266     +287     
  Misses         58       58              
+ Partials       58       57       -1     

@patrick91
Copy link
Member

/pre-release

@botberry
Copy link
Member

botberry commented Jan 14, 2022

Pre-release

👋

Pre-release 0.125.0.dev.1660322955 [af4caec] has been released on PyPi! 🚀
You can try it by doing:

poetry add strawberry-graphql==0.125.0.dev.1660322955

@patrick91
Copy link
Member

I made some fixes for the HttpHandler, we might need some tests for it 😊

@bellini666 bellini666 force-pushed the channels-subscriptions-consumer branch from b5ee597 to b0bc38a Compare August 4, 2022 00:14
@bellini666
Copy link
Member

@patrick91 @DoctorJohn I just made some changes based on some suggestions you've made and replied to some others.

Also, I merged the main branch into this one and readded the channels dev dependency for testing (it seems that the nir's PR for docs removed it by mistake)

@nrbnlulu
Copy link
Member

nrbnlulu commented Aug 4, 2022

sorry 🐵

@bellini666
Copy link
Member

@patrick91 @DoctorJohn I just made some last adjustments based on feedback and also marked some resolved comments as resolved (they were bloating up =P).

That are still a couple of comments opened waiting for some feedback.

Please tell me if is there something missing here :)

@patrick91
Copy link
Member

/pre-release

Copy link
Member

@patrick91 patrick91 left a comment

Choose a reason for hiding this comment

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

I think we can finally merge this PR, thanks for all the work everyone! And sorry for taking so much time to review it 😊

I think next steps after this will be to provide our own tutorial, so people don't have to jump into the channels docs, I'll open an issue about that!
Then we should check that issues with tests, but we can do that in another PR too :)

@bellini666 feel free to merge this PR 😊

@bellini666
Copy link
Member

It is with great pleasure that I merge this PR. Thanks for everyone involved in reviews, testing, docs, and @LucidDan for the initial code :)

@bellini666 bellini666 merged commit d4501ae into strawberry-graphql:main Aug 12, 2022
@JamieOWilliams
Copy link

Thank you to everyone who worked on this PR. It is a great addition to an already awesome project. Thank you to @bellini666, @LucidDan, @nrbnlulu, and everyone else for getting this over the line!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

10 participants