useSubscription: keep subscription Concast alive if new in-flight subscription subscribes at the same time last in-flight subscription unsubscribes

[Hsifnus]

Checklist:

• If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
• If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
• Make sure all of the significant new logic is covered by tests

This PR attempts to address the useSubscription React hook issue outlined in #10695. It is also related to #10693 but does not directly address it outside of the case in which unsubscription and resubscription to the same subscription observable happens in the same frame.

[apollo-cla]

@Hsifnus: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

[netlify]

‼️ Deploy request for apollo-client-docs rejected.

Name	Link
🔨 Latest commit	6068147

[changeset-bot]

🦋 Changeset detected

Latest commit: 10b51c3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@apollo/client	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[phryneas]

Could you please try if an approach like in #10629 would solve the problem as well? That would make it a much smaller change and only affect people that use the useSubscription hook regarding bundle size.

[Hsifnus]

Could you please try if an approach like in #10629 would solve the problem as well? That would make it a much smaller change and only affect people that use the useSubscription hook regarding bundle size.

Figured out a way to immediately stop receiving subscription values in useSubscription while effectively deferring unsubscription from the Concast via setTimeout in 8f7156b

As such, I have shifted the PR to impact useSubscription instead of Concast.

[RustamKarimov3]

/release:pr

[alessbell]

/release:pr

[github-actions]

A new release has been made for this PR. You can install it with npm i @apollo/[email protected].

[phryneas]

@Hsifnus I believe you could circumvent the additional subscription doing something like this - what do you think?

@@ -117,8 +117,10 @@ export function useSubscription<TData = any, TVariables extends OperationVariabl
       return;
     }
 
+    let subscriptionStopped = false;
     const subscription = observable.subscribe({
       next(fetchResult) {
+        if (subscriptionStopped) return;
         const result = {
           loading: false,
           // TODO: fetchResult.data can be null but SubscriptionResult.data
@@ -142,6 +144,7 @@ export function useSubscription<TData = any, TVariables extends OperationVariabl
         }
       },
       error(error) {
+        if (subscriptionStopped) return;
         setResult({
           loading: false,
           data: void 0,
@@ -151,6 +154,7 @@ export function useSubscription<TData = any, TVariables extends OperationVariabl
         ref.current.options?.onError?.(error);
       },
       complete() {
+        if (subscriptionStopped) return;
         if (ref.current.options?.onComplete) {
           ref.current.options.onComplete();
         } else if (ref.current.options?.onSubscriptionComplete) {
@@ -163,14 +167,9 @@ export function useSubscription<TData = any, TVariables extends OperationVariabl
       // immediately stop receiving subscription values, but do not unsubscribe
       // until after a short delay in case another useSubscription hook is
       // reusing the same underlying observable and is about to subscribe
-      const ignoredSubscription = observable.subscribe({
-        next() {},
-        error() {},
-        complete() {},
-      });
-      subscription.unsubscribe();
+      subscriptionStopped = true;
       setTimeout(() => {
-        ignoredSubscription.unsubscribe();
+        subscription.unsubscribe();
       });
     };
   }, [observable]);

[Hsifnus]

@Hsifnus I believe you could circumvent the additional subscription doing something like this - what do you think?

@@ -117,8 +117,10 @@ export function useSubscription<TData = any, TVariables extends OperationVariabl
       return;
     }
 
+    let subscriptionStopped = false;
     const subscription = observable.subscribe({
       next(fetchResult) {
+        if (subscriptionStopped) return;
         const result = {
           loading: false,
           // TODO: fetchResult.data can be null but SubscriptionResult.data
@@ -142,6 +144,7 @@ export function useSubscription<TData = any, TVariables extends OperationVariabl
         }
       },
       error(error) {
+        if (subscriptionStopped) return;
         setResult({
           loading: false,
           data: void 0,
@@ -151,6 +154,7 @@ export function useSubscription<TData = any, TVariables extends OperationVariabl
         ref.current.options?.onError?.(error);
       },
       complete() {
+        if (subscriptionStopped) return;
         if (ref.current.options?.onComplete) {
           ref.current.options.onComplete();
         } else if (ref.current.options?.onSubscriptionComplete) {
@@ -163,14 +167,9 @@ export function useSubscription<TData = any, TVariables extends OperationVariabl
       // immediately stop receiving subscription values, but do not unsubscribe
       // until after a short delay in case another useSubscription hook is
       // reusing the same underlying observable and is about to subscribe
-      const ignoredSubscription = observable.subscribe({
-        next() {},
-        error() {},
-        complete() {},
-      });
-      subscription.unsubscribe();
+      subscriptionStopped = true;
       setTimeout(() => {
-        ignoredSubscription.unsubscribe();
+        subscription.unsubscribe();
       });
     };
   }, [observable]);

Looks like that would be the most straightforward way of circumventing the second subscription. Implemented something similar to that in c890e45

[phryneas]

Looks good to me - let's get this in!