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

feat(subscription): implement new rising protocol graphql-ws #958

Closed
jjangga0214 opened this issue Sep 5, 2021 · 10 comments · Fixed by #1060
Closed

feat(subscription): implement new rising protocol graphql-ws #958

jjangga0214 opened this issue Sep 5, 2021 · 10 comments · Fixed by #1060
Assignees
Labels
dart client Issue relates mainly to the standalone dart client enhancement New feature or request
Milestone

Comments

@jjangga0214
Copy link
Contributor

The official GraphQL specification deliberately does not specify the transport layer.

For subscription, historically, GraphQL over WebSocket Protocol by apollographql/subscriptions-transport-ws library is widely adapted as de-facto standard.

However, the library is not actively maintained, and it officially recommends migrating to the "new" GraphQL over WebSocket Protocol of enisdenjo/graphql-ws.

Those two protocols are incompatible with each other.

The ecosystem is incrementally adapting to the new de-facto standard.

Currently, graphql_flutter only implements subscriptions-transport-ws's protocol. So, if a server uses graphql-ws protocol, graphql_flutter cannot be used.

Thus, hereby I request implementing a new protocol.

Thanks.

@jjangga0214 jjangga0214 added the enhancement New feature or request label Sep 5, 2021
@jjangga0214 jjangga0214 changed the title feat(subscription): implement new raising protocol graphql-ws feat(subscription): implement new rising protocol graphql-ws Sep 5, 2021
@sbatezat
Copy link

sbatezat commented Dec 30, 2021

Thanks @jjangga0214 for these details. It should be mentionned on the readme that graphql-flutter is not supporting graphql-ws protocol (and thus, it's - for now - a useless library as it's the new standard...)

Do you know some other library - up to date with WebSocket standards ?

@vincenzopalazzo vincenzopalazzo added the dart client Issue relates mainly to the standalone dart client label Dec 30, 2021
@vincenzopalazzo vincenzopalazzo added this to Needs triage in v5.1.0 via automation Dec 31, 2021
@vincenzopalazzo vincenzopalazzo self-assigned this Dec 31, 2021
@vincenzopalazzo vincenzopalazzo moved this from Needs triage to High priority in v5.1.0 Feb 18, 2022
@sarp86
Copy link

sarp86 commented Mar 8, 2022

Would be nice to implemented as soon as possible. thx!

@maximilianmaihoefner
Copy link
Contributor

maximilianmaihoefner commented Mar 9, 2022

I also needed this, so i put together a quick working example: https://github.com/maximilianmaihoefner/graphql-flutter/tree/graphql-transport-ws

You can try it out by specifying the following in your pubspec.yaml:

dependencies:
  graphql:
    git:
      url: https://github.com/maximilianmaihoefner/graphql-flutter/tree/graphql-transport-ws
      path: packages/graphql
      ref: graphql-transport-ws
  graphql_flutter:
    git:
      url: https://github.com/maximilianmaihoefner/graphql-flutter
      path: packages/graphql_flutter
      ref: graphql-transport-ws

dependency_overrides:
  graphql:
    git:
      url: https://github.com/maximilianmaihoefner/graphql-flutter
      path: packages/graphql
      ref: graphql-transport-ws

And specifying the new Subprotocol when creating the WebsocketLink:

final WebSocketLink websocketLink = WebSocketLink(
  subscriptionUri,
  config: const SocketClientConfig(autoReconnect: false),
  subProtocol: SocketSubProtocol.graphqlTransportWs,
);

The main changes have been made in websocket_client.dart, it is however not a super clean implementation.

@vincenzopalazzo
Copy link
Collaborator

@maximilianmaihoefner Why not open a PR to contribute to your work?

@maximilianmaihoefner
Copy link
Contributor

@vincenzopalazzo I considered opening a PR, but I don't think my implementation is super clean or maintainable, it probably needs a better concept for supporting both subprotocols. Right now I'm just doing string comparisons to decide which Message to send, it is also missing tests. And I'm not sure I have the time right now to work on these issues. If you want I can submit a PR, but as mentioned, I don't think its in a mergeable state.

@vincenzopalazzo
Copy link
Collaborator

@maximilianmaihoefner I see the code, and yes I agree with your with the better implementation of the protocol healing.

However, we can integrate it and clean it after more testing.

Nothings born clean the first time :)

@maximilianmaihoefner
Copy link
Contributor

@vincenzopalazzo I opened a PR, another thing that should be looked at is the ping implementation, right now I'm highjacking the inactivityTimeout, which results in a ping every 30 Seconds, the original JS-Client, by default, never sends pings. Only if the keepAlive is specified.

@chrisdrobison
Copy link

I am confused by some of the comments. Based on the PR, graphql-ws (the newer protocol) was already implemented. It's the older version of the protocol that you're adding?

@chrisdrobison
Copy link

Nevermind, I see the difference now.

@maximilianmaihoefner
Copy link
Contributor

@chrisdrobison No worries, I was confused when I looked at the code for the first time as well. The new protocol is called graphql-transport-ws (which was the name of the old library) and the old protocol is called graphql-ws (which is the name of the new library). So I can definitely understand the confusion.

@vincenzopalazzo vincenzopalazzo added this to the v5.2.0 milestone Apr 9, 2022
@vincenzopalazzo vincenzopalazzo added this to To do in v5.2.0 via automation Apr 9, 2022
@vincenzopalazzo vincenzopalazzo moved this from To do to In progress in v5.2.0 Apr 9, 2022
v5.2.0 automation moved this from In progress to Done May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dart client Issue relates mainly to the standalone dart client enhancement New feature or request
Projects
v5.1.0
High priority
Development

Successfully merging a pull request may close this issue.

6 participants