Accepting teardown as subscriptionOperation prevents cleanups and dedupes

[mvarendorff2]

Describe the bug

Let me prefix this by saying: I am not sure if this is actionable because fairly edge case.

We used a subscriptionExchange with

  isSubscriptionOperation: (operation) => {
    return operation.kind !== 'mutation';
  },

(queries are automatically converted to live queries on the server).

Today we noticed a lack of deduplication across our queries causing a single hook to create three subscribed queries. After a lot of digging and trying around, I found the solution to be

  isSubscriptionOperation: (operation) => {
-    return operation.kind !== 'mutation';
+    return operation.kind !== 'mutation' && operation.kind !== 'teardown';
  },

I dug around a bit and while I am not sure, I assume the cause is that the teardown operations are not forwarded in the subscription client here (since isSubscriptionOperationFn returned true for them):

https://github.com/urql-graphql/urql/blob/67c991fff71bb0f09a6bc6c5c126c59863c679db/packages/core/src/exchanges/subscription.ts#L217-L223

Again: This is a very specific usecase and I understand if this would not be actionable but I figured I'd open this up still.

Quick note to the Reproduction: The websocket attempts to connect to ws://localhost:5000/graphql; you do not need a running server to see the problem in devtools!

1. Open the link
2. Open browser devtools
3. Filter for websocket (reload the code sandbox if the graphql WS doesn't show up)
4. Notice a large amount of errors and that the messageSub subscription is attempted multiple times
5. Add && operation.kind !== "teardown" to isSubscriptionOperation and save to reload the sandbox
6. Select the new connection in devtools
7. Notice a significantly small amount of requests made in the first place because urql bailed properly

Reproduction

https://codesandbox.io/s/wizardly-kowalevski-jwx03e?file=/src/index.js

Urql version

urql@4.0.2 + urql/core@4.0.7

Validations

• [X] I can confirm that this is a bug report, and not a feature request, RFC, question, or discussion, for which GitHub Discussions should be used
• [X] Read the docs.
• [X] Follow our Code of Conduct

[kitten]

I dug around a bit and while I am not sure, I assume the cause is that the teardown operations are not forwarded in the subscription client here

I don't quite understand what you're getting at here, sorry 🤔

Just to be sure, I'll include a bit more information than necessary.

We have four different operations, as of this moment: query, mutation, subscription, teardown. The first three are, rather self-evidently, events that tell the exchange pipeline to start a GraphQL operation corresponding to these three kinds, i.e. queries, mutations, subscriptions. The teardown is issued either to cancel an ongoing operation with the same operation.key or to tell the exchanges that this operation has ended.

In other words, any exchange can always receive any of the three.

So, you're right in excluding it from the subscriptionExchange, in that, that exchange will never handle teardown as "starts" of an operation. Instead, teardown is a signal to end an operation.

That's already handled for you though: https://github.com/urql-graphql/urql/blob/67c991fff71bb0f09a6bc6c5c126c59863c679db/packages/core/src/exchanges/subscription.ts#L205-L208 So, you'll always want to exclude it in isSubscriptionOperation

The reason why we've just punched in a function like isSubscriptionOperation is that it gives the highest amount of control. Arguably, maybe it shouldn't allow teardown from ever being passed to the exchange, but it's still handled because of the functions name.

I think I'll issue a small fix to always exclude it, but just to be clear, your modification to isSubscriptionOperation is correct to also let subscriptionExchange handle queries but operation.kind === 'query' || operation.kind === 'subscription is what I think we assumed people would write if enableAllOperations is too generic.

[mvarendorff2]

I don't quite understand what you're getting at here, sorry 🤔

I am not familiar with the internals of urql (evidently 😄); I was assuming that by preventing teardown operations to pass through because they were excluded from the forward observable / stream (not too familiar with the terminology of wonka either), it might prevent some clean-up or deduplication that could have occured deeper in the client.

Either way, I really appreciate the additional insight and fix! We also eventually settled on the version you mentioned explicitly whitelisting query and subscription operations; I believe the way in the issue was chosen for brevity under the false assumption that only the three graphql kinds existed.