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

chore(deps): bump graphql-ws to 5.1.2 #1497

Merged
merged 4 commits into from
Jun 11, 2021
Merged

chore(deps): bump graphql-ws to 5.1.2 #1497

merged 4 commits into from
Jun 11, 2021

Conversation

enisdenjo
Copy link
Contributor

Description

graphql-ws@v5.0.0 introduces a few, minimal, breaking changes. One of the quite important one is the addition of the subprotocol level ping and pong messages (enisdenjo/graphql-ws#201 (comment)).

The implementation in postgraphile is untouched.

Please note that the client will NOT ping the server by default. Meaning, this upgrade is not critical.

Performance impact

None.

Security impact

None.

Checklist

  • My code matches the project's code style and yarn lint:fix passes.
  • I've added tests for the new feature, and yarn test passes.
  • I have detailed the new feature in the relevant documentation.
  • I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
  • If this is a breaking change I've explained why.

@enisdenjo enisdenjo changed the title chore(deps): bump graphql-ws to 5.0.0 chore(deps): bump graphql-ws to 5.1.0 Jun 9, 2021
@enisdenjo enisdenjo changed the title chore(deps): bump graphql-ws to 5.1.0 chore(deps): bump graphql-ws to 5.1.1 Jun 9, 2021
@enisdenjo enisdenjo changed the title chore(deps): bump graphql-ws to 5.1.1 chore(deps): bump graphql-ws to 5.1.2 Jun 9, 2021
@benjie
Copy link
Member

benjie commented Jun 11, 2021

Should we leave this for the next PostGraphile major bump, or can this be merged in such a way that it's backwards compatible with existing clients? e.g. if we default to not issuing ping/pongs then will that mean the existing protocol is not broken, and users can opt-in to this behaviour?

I'm glad you're adding ping/pong to the protocol 👍

@enisdenjo
Copy link
Contributor Author

enisdenjo commented Jun 11, 2021

The server is 100% backwards compatible with all clients. Server-side still relies on the WebSocket transport level ping/pongs. Quite literally, nothing changed there (except for the fact that it responds to subprotocol client ping messages).

The client options, by default, are also backwards compatible with all graphql-ws versions. However, if you change the keepAlive client option, it will issue subprotocol level pings and expect pong messages from the server which will, due to validation, terminate the connection instead.

In essence, this update may as well be released in the next minor/patch bump. Furthermore, I'd also recommend not waiting for the major simply because nothing breaks and PostGraphile would just start accepting the new ping/pong message types.

@benjie benjie merged commit 965bc24 into graphile:main Jun 11, 2021
@benjie
Copy link
Member

benjie commented Jun 11, 2021

Thanks @enisdenjo; that's what I suspected but wanted to confirm!

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