useSubscription: fix rules of React violations

phryneas commented 6 months ago

Another one for https://github.com/apollographql/apollo-client/pull/11511 - this one is essentially a rewrite.

changeset-bot[bot] commented 6 months ago

🦋 Changeset detected

Latest commit: 33829ecc5946fef5c8cff06880aa61b2221d7f57

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[bot] commented 6 months ago

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[bot] commented 6 months ago

Deploy Preview for apollo-client-docs ready!

Name	Link
Latest commit	33829ecc5946fef5c8cff06880aa61b2221d7f57
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.

jerelmiller commented 6 months ago

Out of curiosity, does this fix https://github.com/apollographql/apollo-client/issues/11165?

kjhuang-db commented 5 months ago

when will this be merged? can we integrate this https://github.com/apollographql/apollo-client/issues/10216 to it as it is critical to decouple streaming data update vs react-render in a lot of use case

jerelmiller commented 5 months ago

@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 commented 5 months ago

@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 https://github.com/apollographql/react-apollo/issues/3549
- TLDR, there is a need to decouple react rendering from streaming data update
the solution is either
1. follow useMutation to accept ignoreResults https://github.com/apollographql/apollo-client/issues/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?

jerelmiller commented 5 months ago

@kjhuang-db answered in discord :)

phryneas commented 4 months ago

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

apollographql / apollo-client