search

gql-dart / gql

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

[WebSocketLink] inactivityTimeout causes a reconnect #430

Open anlumo opened 9 months ago

anlumo commented 9 months ago

When a WebSocketLink is closed due to the inactivityTimeout running out, it's automatically reconnected due to autoReconnect = true. This defeats the purpose of an inactivityTimeout.

knaeckeKami commented 9 months ago

Can you use the TransportWebsocketLink instead?

The architecture of TransportWebsocketLink is generally better, and the protocol that transport WebSocketLink implements is deprecated.

I imagine there will probably not a lot of maintenance work flow into WebSocketLink (unless someone steps up ;) )

anlumo commented 9 months ago

Then please add the Deprecated annotation. I wasn't aware of that, and the documentation also doesn't say it.

knaeckeKami commented 9 months ago

Well, there are two different protocols for graphql over Websockets.

graphql-transport-ws ( https://github.com/enisdenjo/graphql-ws/blob/master/PROTOCOL.md ) and graphql-ws ( https://github.com/apollographql/subscriptions-transport-ws/blob/master/PROTOCOL.md ).

To make matters worse, the javascript library implementing the graphql-transport-ws protocol is named graphql-ws ( https://www.npmjs.com/package/graphql-ws ) so there is a lot of potential for confusion. The library that implemented the graphql-ws protocol was called subscriptions-transport-ws https://github.com/apollographql/subscriptions-transport-ws/tree/master

The graphql-ws protocol is deprecated. This is stated in the README.

But I would not completely deprecate the WebsocketLink class, there are still a lot of servers that only support this protocol.

However, if your server supports both, you should definitely use the graphql-transport-ws and TransportWebsocketLink class. Aside from the improved protocol, the dart implementation of the TransportWebsocketLink is better.

If you have suggestions on how the documentation around this can be improved, feel free to open issues or PRs.

anlumo commented 9 months ago

If it's broken and not going to be fixed, isn't that the definition of deprecation? If you don't want people to encounter warnings when using that class, maybe just include what you told me here in the documentation of that class.

TransportWebsocketLink works fine for me, btw. It also doesn't have the bug that the WebSocket protocol name isn't set automatically on connection (and so the server rejects the connection before it's even established).

knaeckeKami commented 9 months ago

Well, at least I'm not going to fix it in the near future. But I'm also not the original contributor and I'm not using the Websocketlinks myself. If someone from the community is willing to fix it or take over maintenance, they are welcome of course.