cylc / cylc-ui

Web app for monitoring and controlling Cylc workflows
https://cylc.github.io
GNU General Public License v3.0
37 stars 27 forks source link

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

Open MetRonnie opened 2 years ago

MetRonnie commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

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

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 - https://github.com/cylc/cylc-uiserver/issues/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 :angry: so this bug will need to be squashed at the same time.

So in conclusion this is essentially blocked by both:

oliver-sanders commented 1 year ago

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).