Fix issue where queryRef is recreated unnecessarily in strict mode

[jerelmiller]

Fixes #11815

Fixes a regression in 3.9.10 caused by #11738 that recreated the queryRef when useBackgroundQuery would rerender. This could potentially cause many fetches when paired with a fetchPolicy that ignored the cache.

NOTE: This change reverts most of the work done by #11738 which allowed queryRefs to be disposed of synchronously. This changed has caused too many headaches in strict mode and the amount of code added to avoid a setTimeout isn't worth it.

[changeset-bot]

🦋 Changeset detected

Latest commit: 7d5ff9f

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

[jerelmiller]

I'm open to other names, but figured something like this makes it easier to understand the point of it.

[github-actions]

size-limit report 📦

Path	Size
dist/apollo-client.min.cjs	38.61 KB (-0.03% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs"	46.49 KB (-0.02% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production)	44.05 KB (-0.02% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js"	34.17 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production)	32.06 KB (0%)
import { ApolloProvider } from "dist/react/index.js"	1.23 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production)	1.21 KB (0%)
import { useQuery } from "dist/react/index.js"	5.28 KB (0%)
import { useQuery } from "dist/react/index.js" (production)	4.37 KB (0%)
import { useLazyQuery } from "dist/react/index.js"	5.52 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production)	4.59 KB (0%)
import { useMutation } from "dist/react/index.js"	3.52 KB (0%)
import { useMutation } from "dist/react/index.js" (production)	2.74 KB (0%)
import { useSubscription } from "dist/react/index.js"	3.21 KB (0%)
import { useSubscription } from "dist/react/index.js" (production)	2.4 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js"	5.44 KB (-0.38% 🔽)
import { useSuspenseQuery } from "dist/react/index.js" (production)	4.1 KB (-0.55% 🔽)
import { useBackgroundQuery } from "dist/react/index.js"	4.96 KB (+0.36% 🔺)
import { useBackgroundQuery } from "dist/react/index.js" (production)	3.61 KB (+0.41% 🔺)
import { useLoadableQuery } from "dist/react/index.js"	5.05 KB (0%)
import { useLoadableQuery } from "dist/react/index.js" (production)	3.71 KB (0%)
import { useReadQuery } from "dist/react/index.js"	3.31 KB (-0.18% 🔽)
import { useReadQuery } from "dist/react/index.js" (production)	3.25 KB (-0.16% 🔽)
import { useFragment } from "dist/react/index.js"	2.29 KB (0%)
import { useFragment } from "dist/react/index.js" (production)	2.23 KB (0%)

[netlify]

✅ Deploy Preview for apollo-client-docs ready!

Name	Link
🔨 Latest commit	5b4e5c0
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-client-docs/deploys/6631848c0e79a90008f43ae4
😎 Deploy Preview	https://deploy-preview-11821--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]

/release:pr

[github-actions]

A new release has been made for this PR. You can install it with:

npm i @apollo/[email protected]

[netlify]

✅ Deploy Preview for apollo-client-docs ready!

Name	Link
🔨 Latest commit	7915884
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-client-docs/deploys/66340772512d1d0008ee4425
😎 Deploy Preview	https://deploy-preview-11821--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]

After discussing this with @phryneas, I think the synchronous disposal might be more trouble than its worth. We've seen a couple issues pop up because of that change, and we're adding a TON more code just to get around that single setTimeout. Note, this is purely a strict mode issue.

There were two reasons that I switched from an async dispose to a sync dispose:

1. If a client happens to be shared between tests, it means one test's data can leak into another because the query ref may not have been disposed between tests.

We discussed this a bit more and because we do not recommend that you share a client between tests, we'd be ok with the data leaking between tests. We always recommend that each test run its own instance of ApolloClient to ensure proper test isolation.

2. If a component happens to unmount at the same time another mounts with the same query/variables, it allows those components to have their own query ref instance.

This is a bit of an edge case, but the sync disposal allows for this more naturally. I don't expect this to happen often however and if it does, we have the queryKey option in user-space to allow someone to make them distinct references.

Other than what's listed above, I don't have any strong reasons to keep the synchronous disposal. I believe its more trouble than its worth at this point, so I will update this PR to rollback to use the async disposal with the setTimeout. This should allow us to remove quite a bit of added code as well as a bonus.

[alessbell]

@jerelmiller thanks for that recap! Going back to the asynchronous disposal sounds like the right tradeoff here.

We already note that client instances should not be shared between tests in our own docs, and I've opened a PR for this page https://mswjs.io/docs/faq/#apollo-client in the MSW docs to adjust the recommendation there: mswjs/mswjs.io#393.

[alessbell]

🚀