useSubscription: fix rules of React violations #11863

phryneas · 2024-05-21T09:37:32Z

Another one for #11511 - this one is essentially a rewrite.

…strict_mode" This reverts commit 4571e1a, reversing changes made to 5be85a0.

changeset-bot · 2024-05-21T09:37:36Z

🦋 Changeset detected

Latest commit: 33829ec

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	Minor

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

github-actions · 2024-05-21T09:38:52Z

size-limit report 📦

Path	Size
dist/apollo-client.min.cjs	38.69 KB (+0.04% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs"	47.48 KB (+0.01% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production)	45.08 KB (+0.07% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js"	34.22 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production)	32.08 KB (0%)
import { ApolloProvider } from "dist/react/index.js"	1.26 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production)	1.24 KB (0%)
import { useQuery } from "dist/react/index.js"	5.32 KB (0%)
import { useQuery } from "dist/react/index.js" (production)	4.39 KB (0%)
import { useLazyQuery } from "dist/react/index.js"	5.6 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production)	4.67 KB (0%)
import { useMutation } from "dist/react/index.js"	3.59 KB (0%)
import { useMutation } from "dist/react/index.js" (production)	2.81 KB (0%)
import { useSubscription } from "dist/react/index.js"	3.63 KB (+12.24% 🔺)
import { useSubscription } from "dist/react/index.js" (production)	2.78 KB (+14.49% 🔺)
import { useSuspenseQuery } from "dist/react/index.js"	5.47 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" (production)	4.12 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js"	4.98 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" (production)	3.63 KB (0%)
import { useLoadableQuery } from "dist/react/index.js"	5.04 KB (0%)
import { useLoadableQuery } from "dist/react/index.js" (production)	3.69 KB (0%)
import { useReadQuery } from "dist/react/index.js"	3.35 KB (0%)
import { useReadQuery } from "dist/react/index.js" (production)	3.3 KB (0%)
import { useFragment } from "dist/react/index.js"	2.32 KB (0%)
import { useFragment } from "dist/react/index.js" (production)	2.27 KB (0%)

netlify · 2024-05-21T09:43:08Z

✅ Deploy Preview for apollo-client-docs ready!

Name	Link
🔨 Latest commit	`33829ec`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-client-docs/deploys/668534061444cb00082c8652
😎 Deploy Preview	https://deploy-preview-11863--apollo-client-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

phryneas · 2024-05-21T13:10:39Z

src/testing/react/__tests__/mockSubscriptionLink.test.tsx

@@ -64,7 +62,7 @@ describe("mockSubscriptionLink", () => {
      </ApolloProvider>
    );

-    const numRenders = IS_REACT_18 ? 2 : results.length + 1;


We are now using useSyncExternalStore, so nothing will autobatch here in React 18. Seeing how that confused people with subscriptions, that's probably a good thing.

Good call out. We have a section on the hooks page about subscription autobatching: https://www.apollographql.com/docs/react/api/react/hooks/#usesubscription we should create a ticket to update that with a version range once we know which version this change will ship in (and honestly that disclaimer should be on the subscriptions page too)

That's a very good point - what do you think about these changes? e249bad (#11863)

Also, see this idea: #11863 (comment)

phryneas · 2024-05-21T14:58:23Z

src/react/hooks/useSubscription.ts

+      fetchPolicy !== observable.__.fetchPolicy ||
+      !equal(variables, observable.__.variables)) &&
+      (typeof shouldResubscribe === "function" ?
+        !!shouldResubscribe(options!)


shouldResubscribe should now only execute if something is actually different - but since we're doing this in render, it will be executed every render if they are different, not only if these values changed.

phryneas · 2024-05-21T16:24:24Z

src/react/hooks/useSubscription.ts

+    setObservable(
+      (observable = createSubscription(
+        client,
+        subscription,
+        variables,
+        fetchPolicy,
+        context
+      ))
+    );


Calling setObservable during render will prevent an awkward in-between render where you already passed in new variables/query/client and it still shows you results for the old one.

phryneas · 2024-05-22T10:02:00Z

src/react/hooks/useSubscription.ts

+              variables,
+            };
+            observable.__.setResult(result);
+            update();


Idea: Generally we could think about a omitData option that would remove data from the result and skip calling this update (=component rerendering) if only the data changes. That way, people could subscribe and trigger updates from their own onData handlers as they please.

Uh yeah, that's #10216 ^^

phryneas · 2024-05-22T11:12:58Z

src/react/hooks/useSubscription.ts

-          context: options?.context,
-        })
-      );
-      canResetObservableRef.current = false;


The ref here was a workaround around strictMode, but didn't account for any kind of concurrency and broke rules of React.
https://github.com/apollographql/apollo-client/pull/9707/files

jerelmiller · 2024-05-22T14:42:47Z

Out of curiosity, does this fix #11165?

jerelmiller

I didn't get through the whole review before I left, but figured I'd submit what I had for now. Feel free to take or leave it.

jerelmiller · 2024-05-22T03:15:53Z

src/react/hooks/useSubscription.ts

@@ -102,32 +112,24 @@ export function useSubscription<
  TVariables extends OperationVariables = OperationVariables,
 >(
  subscription: DocumentNode | TypedDocumentNode<TData, TVariables>,
-  options?: SubscriptionHookOptions<NoInfer<TData>, NoInfer<TVariables>>
+  options: SubscriptionHookOptions<NoInfer<TData>, NoInfer<TVariables>> = {}


Suggested change

options: SubscriptionHookOptions<NoInfer<TData>, NoInfer<TVariables>> = {}

options: SubscriptionHookOptions<NoInfer<TData>, NoInfer<TVariables>> = Object.create(null)

I believe we've been using this version of an empty object throughout the project.

Fair enough.

jerelmiller · 2024-05-22T03:16:17Z

src/react/hoc/__tests__/subscriptions/subscriptions.test.tsx

@@ -11,8 +11,6 @@ import { itAsync, MockSubscriptionLink } from "../../../../testing";
 import { graphql } from "../../graphql";
 import { ChildProps } from "../../types";

-const IS_REACT_18 = React.version.startsWith("18");


src/react/hooks/useSubscription.ts

jerelmiller · 2024-05-22T14:55:51Z

src/react/hooks/useSubscription.ts

+  let { skip, fetchPolicy, variables, shouldResubscribe, context } = options;
+  variables = useDeepMemo(() => variables, [variables]);


Suggested change

let { skip, fetchPolicy, variables, shouldResubscribe, context } = options;

variables = useDeepMemo(() => variables, [variables]);

const { skip, fetchPolicy, shouldResubscribe, context } = options;

const variables = useDeepMemo(() => options.variables, [options.variables]);

Perhaps a personal preference, but could we create the variables variable like this? I find this a bit easier to remember that variables references the memoized value easier than having to look at where its reassigned from. (and feel free to leave the first let if you prefer... I know 'let' vs 'const' yada yada).

This is technical more bytes, so feel free to ignore this suggestion if you prefer the way it is.

Perfectly fair :)

kjhuang-db · 2024-06-14T02:59:35Z

when will this be merged? can we integrate this #10216 to it as it is critical to decouple streaming data update vs react-render in a lot of use case

jerelmiller · 2024-06-14T03:03:40Z

@kjhuang-db we won't release this PR until 3.11 which is scheduled for early July. With a big refactor like this, we tend to want to put this in a minor release.

The work to integrate the new option should likely be done in a separate PR to ensure those changes don't get buried along with the rest of this code, but it should absolutely start with the changes from this branch to avoid conflicts.

kjhuang-db · 2024-06-14T13:06:45Z

@kjhuang-db we won't release this PR until 3.11 which is scheduled for early July. With a big refactor like this, we tend to want to put this in a minor release.

The work to integrate the new option should likely be done in a separate PR to ensure those changes don't get buried along with the rest of this code, but it should absolutely start with the changes from this branch to avoid conflicts.

@jerelmiller sounds good. curious since this change seems switching the state from local to externalStore, with such big refactor why it is not a major release?

will check out the PR locally, the goal is still to address the issue of re-rendering issue useSubscription triggers unnecessary rerenders react-apollo#3549
- TLDR, there is a need to decouple react rendering from streaming data update
the solution is either
1. follow useMutation to accept ignoreResults Add an ignoreResults option to the useSubscription API #10216 --- preferred
2. provide a customized hook useSubscriptionWithCallback.

will branch out from this PR and see if I can add on it: I would prefer this in OSS apollo to avoid a local "copy" in my org's codebase, thanks

also a qq for @phryneas: as discussed on Discord, one concern of current useSubscription declarative api (return state data directly) is batching behavior with react 18: the previous usage of useState within useSubscription might get batched (and break consumers' update based on useEffect(, [state]) usage)

do you foresee putting return in externalSyncStore will address this issue? from the change in doc, it seems still an issue in 18?

related: facebook/react#25191 (comment) (what a small world)

jerelmiller · 2024-06-14T19:52:09Z

@kjhuang-db answered in discord :)

phryneas · 2024-06-25T09:11:41Z

Let's address #10216 and #11165 in separate follow-up PRs. I've added both to the 3.11.0 milestone.

jerelmiller

I don't see anything super major. TBH this hook still seems a little complicated, but you're not doing anything that wasn't already there so this change makes sense to me.

Had a couple minor bits of feedback, but nothing blocking that I can see. Thanks for doing this!

.changeset/little-suits-return.md

src/react/components/__tests__/client/Subscription.test.tsx

src/react/hooks/useSubscription.ts

jerelmiller · 2024-07-02T22:03:29Z

src/react/hooks/useSubscription.ts

 *
- * If your subscription API sends multiple messages at the same time or in very fast succession (within fractions of a millisecond), it is likely that only the last message received in that narrow time frame will result in a re-render.
+ * If you want to react to incoming data, please use the `onData` option instead of `useEffect`. State updates you make inside a `useEffect` hook might cause additional rerenders, and `useEffect` is mostly meant for side effects of rendering, not as an event handler.


State updates you make inside a useEffect hook might cause additional rerenders

Wouldn't the same be true of state updates in onData?

That being said, I think the difference here is that you don't have to wait for React's render cycle and triggering the effect to make the state update. If a subscription fires in rapid succession, you give React a better chance to batch the state updates together which might result in fewer rerenders. Does that sound accurate? If so, perhaps we could tweak the description here to say something along those lines?

Wouldn't the same be true of state updates in onData?

Up to React. If they finally start batching uSES and useState updates together (which they committed to in a bug report issue), there will not be an additional render.
It's definitely cleaner to call a state update in onData than one render later.

I've added the sentence

State updates made in an event handler like onData might - depending on the React version - be batched and cause only a single rerender.

src/react/hooks/useSubscription.ts

jerelmiller · 2024-07-02T22:26:44Z

src/react/hooks/useSubscription.ts

+              error: void 0,
+              variables,
+            };
+            observable.__.setResult(result);


Out of curiosity, why the setResult function instead of just using an assignment?

observable.__.result = result;

Is it a type safety thing?

It's hiding a mutable change from the compiler 😓

It cannot reliably detect that this is a deliberate non-reactive write (because we're not using a ref, but instead store values as "internal state" of the observable), so it would complain.

jerelmiller · 2024-07-02T22:39:07Z

src/react/hooks/useSubscription.ts

+            if (!subscriptionStopped) {
+              observable.__.setResult({
+                loading: false,
+                data: void 0,


We should possibly look into this when we address #11165. We persist data in useQuery when we've had a successful result but we get an error on a subsequent result.

That being said, I know this hook is meant to capture the latest value returned from the subscription, so maybe this makes the most sense? Thoughts? (definitely not something to address in this PR, but wanted to add this note as a discussion topic).

jerelmiller · 2024-07-02T22:41:01Z

src/react/hooks/useSubscription.ts

+              // TODO: fetchResult.data can be null but SubscriptionResult.data
+              // expects TData | undefined only
+              data: fetchResult.data!,
+              error: void 0,


We should look into this with #11165. I don't believe we're accurately capturing errorPolicy: 'all' in this hook since next won't populate errors and error won't populate data. I believe next would be called in the case of partial data with errorPolicy: 'all', but would be worth checking.

Not something to do in this PR since this has likely been a bug/oversight for some time, but wanted to note this so we don't miss it.

Co-authored-by: Jerel Miller <[email protected]>

…ooks/useSubscription

phryneas added 2 commits May 17, 2024 16:21

Revert "Merge pull request #9707 from kazekyo/fix_usesubscription_in_…

987ff6d

…strict_mode" This reverts commit 4571e1a, reversing changes made to 5be85a0.

essentially rewrite useSubscription

03d89ea

phryneas added 2 commits May 21, 2024 13:32

use setResult update method

06db150

adjust tests

71dc7aa

phryneas commented May 21, 2024

View reviewed changes

phryneas added 3 commits May 21, 2024 16:26

change observable during render; lazy initialization of subscription

ac0d804

no more need for stable options, performance optimization

4aa14cf

changeset

af260cf

phryneas marked this pull request as ready for review May 21, 2024 14:55

phryneas requested review from jerelmiller and alessbell May 21, 2024 14:56

phryneas commented May 21, 2024

View reviewed changes

adjust documentation

e249bad

phryneas commented May 22, 2024

View reviewed changes

jerelmiller reviewed May 22, 2024

View reviewed changes

phryneas modified the milestones: React Hooks refresh, 3.11.0 Jun 25, 2024

review feedback

297919f

phryneas mentioned this pull request Jul 1, 2024

Inside StrictMode useSubscription subscribes twice #6037

Closed

jerelmiller changed the base branch from main to release-3.11 July 2, 2024 17:29

jerelmiller approved these changes Jul 2, 2024

View reviewed changes

phryneas and others added 3 commits July 3, 2024 12:17

Apply suggestions from code review

9f64d3f

Co-authored-by: Jerel Miller <[email protected]>

clarify comment

654d949

Merge remote-tracking branch 'origin/release-3.11' into pr/rules-of-h…

74fe836

…ooks/useSubscription

phryneas added the auto-cleanup 🤖 label Jul 3, 2024

phryneas added 2 commits July 3, 2024 13:16

update size-limits

1af05a6

fix test

33829ec

phryneas merged commit 98e44f7 into release-3.11 Jul 3, 2024
42 checks passed

phryneas deleted the pr/rules-of-hooks/useSubscription branch July 3, 2024 11:27

github-actions bot mentioned this pull request Jul 9, 2024

Version Packages (rc) #11940

Merged

github-actions bot mentioned this pull request Jul 22, 2024

Version Packages #11967

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

useSubscription: fix rules of React violations #11863

useSubscription: fix rules of React violations #11863

phryneas commented May 21, 2024

changeset-bot bot commented May 21, 2024 •

edited

Loading

github-actions bot commented May 21, 2024 •

edited

Loading

netlify bot commented May 21, 2024 •

edited

Loading

phryneas May 21, 2024

alessbell May 21, 2024

phryneas May 22, 2024

phryneas May 22, 2024

phryneas May 21, 2024

phryneas May 21, 2024

phryneas May 22, 2024 •

edited

Loading

phryneas Jun 25, 2024

phryneas May 22, 2024

jerelmiller commented May 22, 2024

jerelmiller left a comment

jerelmiller May 22, 2024

phryneas Jun 25, 2024

jerelmiller May 22, 2024

jerelmiller May 22, 2024

phryneas Jun 25, 2024

kjhuang-db commented Jun 14, 2024 •

edited

Loading

jerelmiller commented Jun 14, 2024

kjhuang-db commented Jun 14, 2024 •

edited

Loading

jerelmiller commented Jun 14, 2024

phryneas commented Jun 25, 2024

jerelmiller left a comment

jerelmiller Jul 2, 2024

phryneas Jul 3, 2024 •

edited

Loading

phryneas Jul 3, 2024

jerelmiller Jul 2, 2024

phryneas Jul 3, 2024 •

edited

Loading

jerelmiller Jul 2, 2024

jerelmiller Jul 2, 2024

	options: SubscriptionHookOptions<NoInfer<TData>, NoInfer<TVariables>> = {}
	options: SubscriptionHookOptions<NoInfer<TData>, NoInfer<TVariables>> = Object.create(null)

		let { skip, fetchPolicy, variables, shouldResubscribe, context } = options;
		variables = useDeepMemo(() => variables, [variables]);

useSubscription: fix rules of React violations #11863

useSubscription: fix rules of React violations #11863

Conversation

phryneas commented May 21, 2024

changeset-bot bot commented May 21, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented May 21, 2024 • edited Loading

size-limit report 📦

netlify bot commented May 21, 2024 • edited Loading

✅ Deploy Preview for apollo-client-docs ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phryneas May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerelmiller commented May 22, 2024

jerelmiller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjhuang-db commented Jun 14, 2024 • edited Loading

jerelmiller commented Jun 14, 2024

kjhuang-db commented Jun 14, 2024 • edited Loading

jerelmiller commented Jun 14, 2024

phryneas commented Jun 25, 2024

jerelmiller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phryneas Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phryneas Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changeset-bot bot commented May 21, 2024 •

edited

Loading

github-actions bot commented May 21, 2024 •

edited

Loading

netlify bot commented May 21, 2024 •

edited

Loading

phryneas May 22, 2024 •

edited

Loading

kjhuang-db commented Jun 14, 2024 •

edited

Loading

kjhuang-db commented Jun 14, 2024 •

edited

Loading

phryneas Jul 3, 2024 •

edited

Loading

phryneas Jul 3, 2024 •

edited

Loading