Fixed useQuery under StrictMode creating 2 observable query

[Cellule]

trafficstars

Closes #11914

[netlify[bot]]

👷 Deploy request for apollo-client-docs pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	e568c78bb446e6843927510bc899ef8e0fbced8a

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: e568c78bb446e6843927510bc899ef8e0fbced8a

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

[svc-apollo-docs]

✅ Docs Preview Ready

No new or changed pages found.

[Cellule]

@jerelmiller Let me know how you feel about the direction I've taken in my PR to fix the StrictMode issue before I dig in more into the current test failures

[jerelmiller]

I asked the team about this and my colleague pointed out something that we do in useSubscription that we may or may not be able to do here. Check out this bit of code:

https://github.com/apollographql/apollo-client/blob/6496b25613a10c7d99633d1ceef8c895d224d527/src/react/hooks/useSubscription.ts#L354-L363

Here we wrap the subscription with its own Observable so that we only create the ObservableQuery when its subscribed. Not sure if you want to perhaps noodle on this? This might bring its own challenges with useQuery, but if you're able to get something working like this then we can isolate the changes to useQuery itself without having to touch the core API.

Thoughts?

[Cellule]

I asked the team about this and my colleague pointed out something that we do in useSubscription that we may or may not be able to do here. Check out this bit of code:

https://github.com/apollographql/apollo-client/blob/6496b25613a10c7d99633d1ceef8c895d224d527/src/react/hooks/useSubscription.ts#L354-L363

Here we wrap the subscription with its own Observable so that we only create the ObservableQuery when its subscribed. Not sure if you want to perhaps noodle on this? This might bring its own challenges with useQuery, but if you're able to get something working like this then we can isolate the changes to useQuery itself without having to touch the core API.

Thoughts?

So I was digging into this, I haven't able to easily replicate that kind of behavior for useQuery. My biggest issue was that too much code relies on the ObservableQuery to be readily available and returned to the user through obsQueryFields It would likely be possible to make it work, but as I started hacking at it, it got messy fast.

So instead I considered another angle, since we want to register the query in the QueryManager when the ObservableQuery subscribes, why not let it take care of subscribing. The ObservableQuery is also in charge of removing itself through tearDownQuery so I thought it made sense. I removed this.queries.set(observable.queryId, queryInfo); from QueryManager.watchQuery and added this.queryManager["queries"].set(this.queryId, this.queryInfo); in the subscribe function of ObservableQuery. It worked in my scenario, but I noticed that a few other tests failed, namely in createQueryPreloader and some others. The tests looked fine, so I was suspicious. I'm not sure why you would want to register a query that you won't subscribe to, but anyway.

So now I'm looking at a solution to have this behavior only in useQuery. I passed a hidden option through WatchQueryOption that would only set the query in QueryManager.queries in the ObservableQuery subscribe function. It seems to work, but look a bit hackish (maybe less than my current solution?)

Boy oh boy, I thought the solution worked because I only tested on React19, but it seems on React18 and 17 the behavior of useSyncExternalStore causes to call the getSnapshot function breaking React rules which registers the query in the ApolloCache When another query gets result, it would broadcast to all cache watches causing the query to reobserve. I initially hid/mitigated this part with the shouldNotify "fix"

Now I really feel like we might need to go down the path of only creating the ObservableQuery when it actually subscribes 😮‍💨

[jerelmiller]

Gah. I had another conference to attend then was on vacation. Apologies for my slow time to respond!

I will take a look at your changes and respond to your latest comment next week. Thanks so much for your patience!

[Cellule]

Gah. I had another conference to attend then was on vacation. Apologies for my slow time to respond!

I will take a look at your changes and respond to your latest comment next week. Thanks so much for your patience!

Haha no worries, glad it's still on your radar. The current state of my PR is not really great hahaha. I feel I might have to pull my sleeve and do the dirty work. Let me know what you think from my other comments then I can try to take another go at it 😁

[jerelmiller]

Hey @Cellule 👋

Thanks for your patience! We've done a pretty aggressive update to our core code for the upcoming 4.0 release which should solve this problem once and for all. 4.0 has updated the definition of "active" vs "inactive" queries where an active query is now considered a query with at least one subscriber that isn't skipped/in standby. An inactive query is the inverse; a query without subscribers or is in standby.

With this change, we don't register the query with QueryManager until the query has been kicked off, either by subscribing, or by calling refetch/fetchMore etc (after which it will be removed from QueryManager if there are no subscribers once the request finishes). This should help avoid the issue in React where strict mode might create two ObservableQuery instances where the first is considered "active" under 3.x. In 4.0, that first ObservableQuery should now just get garbage collected.

If you're curious on specifics, see https://github.com/apollographql/apollo-client/pull/12647 for more details on the change. This was released in 4.0.0-alpha.20 if you'd like to try this out and provide feedback! Thanks again for the contribution!

[Cellule]

Awesome! I'll definitely give it a try in the near future :D