gql-dart / gql

Libraries supporting GraphQL in Dart
MIT License
267 stars 121 forks source link

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

Closed catapop84 closed 10 months ago

catapop84 commented 11 months ago

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.
catapop84 commented 11 months ago

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 commented 11 months ago

@juancastillo0 what do you think?

juancastillo0 commented 11 months ago

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 https://github.com/gql-dart/gql/issues/411#issuecomment-1666551422 and I am not sure what is the status of this PR https://github.com/gql-dart/gql/pull/412.

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

knaeckeKami commented 11 months ago

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

catapop84 commented 11 months ago

@knaeckeKami done

catapop84 commented 11 months ago

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