Skip to content

Unsubscribe fix #717

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

Closed
wants to merge 4 commits into from
Closed

Unsubscribe fix #717

wants to merge 4 commits into from

Conversation

jerel
Copy link
Contributor

@jerel jerel commented Jul 22, 2020

The current implementation of juniper_warp will kill the websocket at the first stop call or if a deserialization error is encountered. This means that any running operation (such as a subscription) that is stopped will require the client to reconnect and set up its subscriptions again.

This fix does two things:

  1. ACKs a connection_init command
  2. Only closes the websocket if the client sends a connection_terminate command

Both are modeled after the Apollo implementation https://github.com/apollographql/subscriptions-transport-ws/blob/master/PROTOCOL.md

Copy link
Member

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

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

@jerel Sorry for this taking so long! Would love a test to make sure we don't regress. Also, will this be obsoleted by #721?

@jerel
Copy link
Contributor Author

jerel commented Jul 31, 2020

@LegNeato yes, this PR is unnecessary due to #721. I'll close for now and if the new implementation from 721 still works this way when I get around to using it I'll re-open an issue or PR based on that.

@jerel jerel closed this Jul 31, 2020
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.

2 participants