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

Replace subscriptions-transport-ws with graphql-ws #1028

Open
MetRonnie opened this issue May 17, 2022 · 3 comments · May be fixed by #1429
Open

Replace subscriptions-transport-ws with graphql-ws #1028

MetRonnie opened this issue May 17, 2022 · 3 comments · May be fixed by #1429
Assignees
Labels
dependencies Pull requests that update a dependency file investigation javascript Pull requests that update Javascript code
Milestone

Comments

@MetRonnie
Copy link
Member

subscriptions-transport-ws@npm:0.11.0 is deprecated: The subscriptions-transport-ws package is no longer maintained. We recommend you use graphql-ws instead. For help migrating Apollo software to graphql-ws, see https://www.apollographql.com/docs/apollo-server/data/subscriptions/#switching-from-subscriptions-transport-ws For general help using graphql-ws, see https://github.com/enisdenjo/graphql-ws/blob/master/README.md

Pull requests welcome!

@MetRonnie MetRonnie added the dependencies Pull requests that update a dependency file label May 17, 2022
@MetRonnie MetRonnie added this to the Pending milestone May 17, 2022
@MetRonnie MetRonnie added the javascript Pull requests that update Javascript code label May 17, 2022
@MetRonnie
Copy link
Member Author

From #1199

subscriptions-transport-ws is no longer maintained

Users should migrate to graphql-ws, a newer actively-maintained implementation of a similar protocol.

-- https://github.com/apollographql/subscriptions-transport-ws#subscriptions-transport-ws-is-no-longer-maintained

The two libraries do not use the same WebSocket subprotocol, so you need to make sure that your server and clients all use the same library.

-- https://www.apollographql.com/docs/react/data/subscriptions#choosing-a-subscription-library

@oliver-sanders
Copy link
Member

Assigning this as I've made a start, progress so far:

  • We are currently using subscriptions-transport-ws which has been deprecated since 2018.
  • We are now recommended to use graphql-ws, however, this is not a like for like replacement because the subprotocol is different (as commented above).
  • The subprotocol is essentially the pattern of messages used to communicate subscription requests/data over websockets. There are multiple standards.

For some utterly perverse reason the graphql-transport-ws uses the graphql-ws subprotocol, whereas graphql-ws uses graphql-transport-ws!!! I eventually found this digging around the Apollo docs after a prolonged period of intense confusion:

Note: Confusingly, the subscriptions-transport-ws library calls its WebSocket subprotocol graphql-ws, and the graphql-ws library calls its subprotocol graphql-transport-ws! In this article, we refer to the two libraries (subscriptions-transport-ws and graphql-ws), not the two subprotocols.

-- https://www.apollographql.com/docs/react/data/subscriptions/#websocket-subprotocols

This matters to us because our server uses the Python graphql-ws library which uses the graphql-ws protocol.

So in JavaScript the libraries and protocols are the wrong way around but in Python they are the right way around!!!

So to migrate to this new library, which seems to be unavoidable since the old one is archived, we would need to add graphql-transport-wssubprotocol support to the Python graphql-ws library. The good news is that there's already a PR to do this, the bad news is that it's two years old with no maintainers to merge it which brings us to this bundle of joy - cylc/cylc-uiserver#333.

Using the open PR with a small modification to graphql-ws:

diff --git a/graphql_ws/base.py b/graphql_ws/base.py
index e69f12e..2b3cbf3 100644
--- a/graphql_ws/base.py
+++ b/graphql_ws/base.py
@@ -87,6 +87,8 @@ class BaseSubscriptionServer(object):
         op_type = parsed_message.get("type")
         payload = parsed_message.get("payload")
 
+        connection_context.transport_ws_protocol = True
+
         if op_type == GQL_CONNECTION_INIT:
             return self.on_connection_init(connection_context, op_id, payload)

And one small modification to cylc-uiserver:

diff --git a/cylc/uiserver/handlers.py b/cylc/uiserver/handlers.py
index aecef40..fe5ec6b 100644
--- a/cylc/uiserver/handlers.py
+++ b/cylc/uiserver/handlers.py
@@ -372,7 +372,7 @@ class SubscriptionHandler(CylcAppHandler, websocket.WebSocketHandler):
         self.sub_statuses: Dict = sub_statuses
 
     def select_subprotocol(self, subprotocols):
-        return GRAPHQL_WS
+        return 'graphql-transport-ws'
 
     @websockets_authenticated
     def get(self, *args, **kwargs):

I was able to get this working with the following diff to cylc-ui:

diff --git a/package.json b/package.json
index 75d2e870..f4148a35 100644
--- a/package.json
+++ b/package.json
@@ -41,7 +41,6 @@
     "nprogress": "1.0.0-1",
     "preact": "10.16.0",
     "preact-compat": "3.19.0",
-    "subscriptions-transport-ws": "0.11.0",
     "svg-pan-zoom": "3.6.1",
     "vue": "3.3.4",
     "vue-i18n": "9.2.2",
diff --git a/src/graphql/index.js b/src/graphql/index.js
index cc252c96..7295bfba 100644
--- a/src/graphql/index.js
+++ b/src/graphql/index.js
@@ -22,10 +22,11 @@ import {
   InMemoryCache,
   split
 } from '@apollo/client/core'
+import { GraphQLWsLink } from '@apollo/client/link/subscriptions'
 import { getMainDefinition } from '@apollo/client/utilities'
-import { WebSocketLink } from '@apollo/client/link/ws'
+// import { WebSocketLink } from '@apollo/client/link/ws'
 import { setContext } from '@apollo/client/link/context'
-import { SubscriptionClient } from 'subscriptions-transport-ws'
+import { createClient } from 'graphql-ws'
 import { store } from '@/store/index'
 import { createUrl } from '@/utils/urls'
 
@@ -72,27 +73,34 @@ export function getCylcHeaders () {
  * @return {SubscriptionClient} a subscription client
  */
 export function createSubscriptionClient (wsUrl, options = {}, wsImpl = null) {
-  const opts = Object.assign({
-    reconnect: true,
-    lazy: false
-  }, options)
-  const subscriptionClient = new SubscriptionClient(wsUrl, opts, wsImpl)
-  // these are the available hooks in the subscription client lifecycle
-  subscriptionClient.onConnecting(() => {
-    store.commit('SET_OFFLINE', true)
-  })
-  subscriptionClient.onConnected(() => {
-    store.commit('SET_OFFLINE', false)
-  })
-  subscriptionClient.onReconnecting(() => {
-    store.commit('SET_OFFLINE', true)
-  })
-  subscriptionClient.onReconnected(() => {
-    store.commit('SET_OFFLINE', false)
-  })
-  subscriptionClient.onDisconnected(() => {
-    store.commit('SET_OFFLINE', true)
+  const subscriptionClient = createClient({
+    url: wsUrl,
+    shouldRetry: () => true,
+    on: {
+      connecting: () => {
+        console.log('# on.connecting')
+        store.commit('SET_OFFLINE', true)
+      },
+      opened: (socket) => {
+        console.log('# on.opened')
+        // store.commit('SET_OFFLINE', true)
+      },
+      connected: (socket, payload) => {
+        console.log('# on.connected')
+        store.commit('SET_OFFLINE', false)
+      },
+      closed: (event) => {
+        console.log('# on.closed')
+        store.commit('SET_OFFLINE', true)
+      },
+      error: (error) => {
+        console.log('# on.error')
+        console.error(error)
+        store.commit('SET_OFFLINE', true)
+      },
+    }
   })
+
   // TODO: at the moment the error displays an Event object, but the browser also displays the problem, as well as the offline indicator
   //       would be nice to find a better error message using the error object
   // subscriptionClient.onError((error) => {
@@ -127,7 +135,7 @@ export function createApolloClient (httpUrl, subscriptionClient) {
   })
 
   const wsLink = subscriptionClient !== null
-    ? new WebSocketLink(subscriptionClient)
+    ? new GraphQLWsLink(subscriptionClient)
     : new ApolloLink() // return an empty link, useful for testing, offline mode, etc
 
   const link = split(

However, in doing so #1200 appeared to become more a regular occurrence 😠 so this bug will need to be squashed at the same time.

So in conclusion this is essentially blocked by both:

oliver-sanders added a commit to oliver-sanders/cylc-ui that referenced this issue Aug 9, 2023
* The graphql-transport-ws library is no longer maintained.
* Move to grahpql-ws.
* Closes cylc#1028
@oliver-sanders
Copy link
Member

After I got retries set up properly the slow-initial-connection bug surfaces about as often with graphql-ws as with graphql-transport-ws (the new library will raise the error socket closed after retry attempts have been exhausted, default 5).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file investigation javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants