graphql-python
Add tornado
Thanks for all the work!
This adds a tornado handler and context, which is a very minimal change on top of websockets_lib.py. Tornado is a tad more verbose, I guess, but pretty useful and important in its own right. While theoretically possible, I haven't bothered with python2 support.
Because tornado's websocket on_message implementation is callback-based, I've used an asyncio.Queue to make it act more like what other things would expect, but it seems like handle might need some more information. I've left it looking for the magic recv, just to keep the line changes down.
I couldn't immediately identify any places to plug in new tests... guess I'll see what CI does!
Travis build on Python 2 is failing due to pathlib, which is part of standard lib in Python 3. Should we add pathlib as requirement, or maybe go with os, path, etc to keep backward compatibility?
Sorry to have put this down for so long!
I can certainly rewrite without pathlib... But tornado py2 support is on the wane, with 6.0 being py3.5+ I believe.
On Tue, Jul 16, 2019, 18:57 Bruno P. Kinoshita [email protected] wrote:
Travis build on Python 2 is failing due to pathlib, which is part of standard lib in Python 3. Should we add pathlib as requirement, or maybe go with os, path, etc to keep backward compatibility?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/graphql-python/graphql-ws/pull/25?email_source=notifications&email_token=AAALCRBITNPQ3TW4ORCM6WLP7ZG6BA5CNFSM4GOI45N2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2CQFVA#issuecomment-512033492, or mute the thread https://github.com/notifications/unsubscribe-auth/AAALCRGYFUEOIWQNP5CHFTTP7ZG6BANCNFSM4GOI45NQ .
Yeah, don't know how much I want to wade back into @gen.coroutines over async...
Yeah, don't know how much I want to wade back into @gen.coroutines over async...
Yeah.. it would be way easier to update/maintain this if Py3+ only was possible.
Yeah.. it would be way easier to update/maintain this if Py3+ only was possible.
Is there stated official cut-off release for Py2? I could certainly hold my breath until one was announced, as I have already vendored it into https://github.com/deathbeds/jupyter-graphql/pull/2, which hasn't been moving much.
Had another look at the changes today, and was thinking in sending a PR to @bollwyvl 's branch to support Py2 as we also want this feature.
However, graphql_ws.aiohttp is Py3 only. So it looks to me like a few files like base and constants are Py2 compatible. And gevent I think.
But if the user wants to use websockets or asyncio, then that part of the library is Py3 only? The tests pass on Py2, and I can also install the library with Py2. Running the examples with websockets_lib or asyncio/aiohttp result in error in Py2.
So if that's correct, I think we can ignore the Py2 requirement, and use @bollwyvl 's code as-is? The master branch is not passing in Travis as well - https://travis-ci.org/graphql-python/graphql-ws/builds/432902410
to support Py2 as we also want this feature.
That's great: no doubt tornado can support py2, and it would not be a big deal to support it, and could likely be written and tested in such a way as to support both tornado 5 and 6, but I am disinclined to advertise support for py2 at this time.
The master branch is not passing in Travis as well
Yeah, that makes it hard to get started as a contributor.
part of the library is Py3 only
Yeah, I think a pinned issue/README statement of graphql-ws Python 2 support would be reasonable to add, including how long python 2 support would continue.
@jkimbo - I know you haven't been involved with this one.. But no maintainers appear active here...
Do you know what's happening with this project? it seems to be stagnant..
Is it anything to do with this graphql-core-next?
https://github.com/graphql-python/graphql-ws/tree/next/master
It would be good to have this PR in ...
Tested the pull request with a small Vue.js application and it worked (thanks @dwsutherland for the help troubleshooting a few minor issues).
https://github.com/kinow/tornado-sandbox/tree/master/web5 (copied the tornado.py from this project as tornado_ws_wip.py there)
Ping. We've been happily using the code in this pull request in https://github.com/cylc/cylc-uiserver. Any plans on review/merging it? Thanks!
Hi @kinow -- I'm new to the team and trying to actively get through the PRs and assess them. I'm currently doing a reshuffle of the core code to split out the common sync and async code across the current servers. Hopefully it should make a smaller effort to integrate, but it may require some rewriting.
Hi @SmileyChris !
Thanks for the quick reply!
Hi @kinow -- I'm new to the team and trying to actively get through the PRs and assess them. I'm currently doing a reshuffle of the core code to split out the common sync and async code across the current servers.
Sounds great! graphql-ws has been really useful already, so I think it will be even better after these changes.
Hopefully it should make a smaller effort to integrate, but it may require some rewriting.
@bollwyvl 's code is simple well structured, and we have become somewhat familiar with it. So we'd be happy to help testing it with our current system or even trying to suggest how to fix some conflicts if necessary.
Thanks!
#46 is the PR tracking the base class rewrites. You may want to check out that branch to get an idea about building on top of that
We have been using code derived from this for a while, it works well, thanks all.
Happy to review differences and contribute any positive changes once this is in.
