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

close reason should be optional and options.shouldRetry -> bool as a replacement for isFatalConnectionProblem #418

Merged
merged 5 commits into from
Nov 11, 2023

Conversation

catapop84
Copy link
Contributor

This PR fixes 2 problems

  1. Server can close connection without providing any reason. With current implementation this will throw an error because reason: socket.closeReason! is expected to exists.
  2. Since the javascript library implementation is the main reference, they introduce shouldRetry: boolean option and deprecated isFatalConnectionProblem

Based on their documentation: https://github.com/enisdenjo/graphql-ws/blob/master/src/client.ts , shouldRetry can override fatal close code which is the default implementation.

   * The library classifies the following close events as fatal:
   * - _All internal WebSocket fatal close codes (check `isFatalInternalCloseCode` in `src/client.ts` for exact list)_
   * - `4500: Internal server error`
   * - `4005: Internal client error`
   * - `4400: Bad request`
   * - `4004: Bad response`
   * - `4401: Unauthorized` _tried subscribing before connect ack_
   * - `4406: Subprotocol not acceptable`
   * - `4409: Subscriber for <id> already exists` _distinction is very important_
   * - `4429: Too many initialisation requests`
   *
   * In addition to the aforementioned close events, any _non-CloseEvent_ connection problem
   * is considered fatal by default. However, this specific behaviour can be altered by using
   * the `shouldRetry` option.

…n with a code without providing any reason

- adding option shouldRetry -> bool that is meant to replace isFatalConnectionProblem(deprecated in the original javascript library) . this way we can configure to override and reconnect on certain fatal errors
@catapop84
Copy link
Contributor Author

Sometimes we make updates to our server and we need to reset the program. On closing server sends close code 1002 and no reason.
This way we can override if we choose to to reconnect once the server is rebooted. This will also make the library in line with the javascript equivalent .

PS. I know this might be confusing, but this PR is for the new websocket protocol: graphql-transport-ws from the library graphql-ws

@knaeckeKami
Copy link
Collaborator

@juancastillo0 what do you think?

@juancastillo0
Copy link
Contributor

Hi! Looks great! The implementation is a little different form the reference, but the differences make sense. There are no additional tests, however the changes are kind of incremental.

Not sure if we should treat it as a breaking change, since the close reason was non-nullable. We could also make it non-null and pass an empty String (or the close code as a String) when the server does not return a reason, but I personally don't like it. And I suppose it would still be a breaking change since the TransportWsClientOptions has a new field and it could be implemented, but I believe it would be less common.

If we make a breaking change maybe we could accommodate other changes like this one #411 (comment) and I am not sure what is the status of this PR #412.

In general I think it could be merge, not sure what you think about the test and the breaking change.

@knaeckeKami
Copy link
Collaborator

@catapop84 please pull the latest changes from master to CI succeeds

@catapop84
Copy link
Contributor Author

@knaeckeKami done

@catapop84
Copy link
Contributor Author

@knaeckeKami can you try one more time. Should be valid this time.

@knaeckeKami knaeckeKami merged commit 5a56f15 into gql-dart:master Nov 11, 2023
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.

3 participants