Skip to content

Adding a new transport class to handle Phoenix channels #100

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 25 commits into from
Sep 7, 2020
Merged

Adding a new transport class to handle Phoenix channels #100

merged 25 commits into from
Sep 7, 2020

Conversation

leruaa
Copy link
Contributor

@leruaa leruaa commented Jun 6, 2020

First of all, thank you for this great library !

I'm building a tool that connect to a server built on Absinthe. This toolkit relies on the Phoenix framework channels feature to handle GraphQL subscriptions.

So I created a PhoenixChannelWebsocketsTransport class that implement all the logic to subscribe and receive data from a Phoenix channel.

Of course I'm open to any suggestions to improve this PR

@coveralls
Copy link

coveralls commented Jun 6, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-0.5%) to 99.483% when pulling 28ad41d on acatinon:feature-phoenix-channel into faadac4 on graphql-python:master.

@KingDarBoja
Copy link
Contributor

Not an expert on Elixir related framework/libraries but this transport class could be added as extra optional feature so any Elixir (Phoenix in this case) user can install it by running the following command:

pip install gql[phoenix-transport]

@leszekhanusz what do you think?

Also, I will take a look at it whenever I can, currently busy with some graphql-server-core stuff.

leszekhanusz
leszekhanusz requested changes Jun 7, 2020
View reviewed changes
Copy link
Collaborator

@leszekhanusz leszekhanusz left a comment

Choose a reason for hiding this comment

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

Very interesting.
I like how this transport inherits the websockets transport.
I tried to find a spec which describe the protocol implemented here but I have not found it. Could you please add a link to it ?

@leruaa
Copy link
Contributor Author

leruaa commented Jun 7, 2020

I tried to find a spec which describe the protocol implemented here but I have not found it. Could you please add a link to it ?

@leszekhanusz Unfortunately I didn't find any specs for Phoenix channels either. There are pointers to some docs in this thread, but i've mainly done reverse engineering on the C# Phoenix client project.

Btw I will take your feedbacks into account.

@leruaa
Copy link
Contributor Author

leruaa commented Jun 9, 2020

@leszekhanusz I finished taking your comments into account.

@KingDarBoja KingDarBoja added type: feature A new feature type: tests Adding missing or correcting existing tests labels Jun 14, 2020
@KingDarBoja KingDarBoja added the status: waiting for reviewer Waiting for a maintainer to review label Jun 29, 2020
@leruaa
Copy link
Contributor Author

leruaa commented Jun 29, 2020

Do you need me to further improve the test coverage on this PR ?

@KingDarBoja
Copy link
Contributor

Do you need me to further improve the test coverage on this PR ?

The /gql/transport/phoenix_channel_websockets.py has 96.26% of coverage as seen at coveralls report, but I think it is pretty high already.

So don't worry about it at the moment.

@KingDarBoja
Copy link
Contributor

KingDarBoja commented Jun 29, 2020 •
edited
Loading

@acatinon updating with the latest changes from master should bring coverage to 100%, also there is few mypy errors on the phoenix transport.

@leruaa
Copy link
Contributor Author

leruaa commented Jun 29, 2020

@KingDarBoja Done.

KingDarBoja
KingDarBoja reviewed Jul 5, 2020
View reviewed changes
KingDarBoja
KingDarBoja reviewed Jul 5, 2020
View reviewed changes
@KingDarBoja KingDarBoja added status: waiting for author The PR author should address requested changes and removed status: waiting for reviewer Waiting for a maintainer to review labels Jul 5, 2020
@KingDarBoja
Copy link
Contributor

This one is almost ready to merge, only thing left is increasing coverage to 100%.

cc @leszekhanusz

@KingDarBoja KingDarBoja added the lib: phoenix Phoenix Channel Extra Package label Jul 19, 2020
KingDarBoja
KingDarBoja approved these changes Sep 7, 2020
View reviewed changes
Copy link
Contributor

@KingDarBoja KingDarBoja left a comment

Choose a reason for hiding this comment

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

Coverage reached 100% 🚀

@leszekhanusz
Copy link
Collaborator

Coverage reached 100% rocket

I cheated a little and just added pragmas for the few remaining lines...

@leszekhanusz leszekhanusz merged commit 706f789 into graphql-python:master Sep 7, 2020
@leszekhanusz
Copy link
Collaborator

@acatinon Thanks again for this PR and sorry for the delay

@leruaa leruaa deleted the feature-phoenix-channel branch September 7, 2020 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leszekhanusz leszekhanusz leszekhanusz approved these changes

+1 more reviewer

@KingDarBoja KingDarBoja KingDarBoja approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@leruaa leruaa

Labels
lib: phoenix Phoenix Channel Extra Package status: waiting for author The PR author should address requested changes type: feature A new feature type: tests Adding missing or correcting existing tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@leruaa @coveralls @KingDarBoja @leszekhanusz @JBrVJxsc