search

apollographql / apollo-client

:rocket:  A fully-featured, production ready caching GraphQL client for every UI framework and GraphQL server.
https://apollographql.com/client
MIT License
19.38k stars 2.66k forks source link

Background query still active after component unmounted #11649

Closed PiR1 closed 8 months ago

PiR1 commented 8 months ago

Issue Description

When you have a component with a background query that have been unmounted, the background query is still active. So if you modify the cache corresponding to the queried objects, the background query is triggered and a request is sent to the server.

Link to Reproduction

https://codesandbox.io/p/github/PiR1/react-apollo-error-template/main?import=true

Reproduction Steps

Step to reproduce the issue (on the above codesandbox)

  1. go to background query page
  2. go to home page
  3. click on delete person
  4. see that the background query has been triggered (you can see it in the console)

General step to reproduce the issue

  1. render a component with a background query
  2. unmount it (eg: got to another page if you use react-router)
  3. modify cache related to the background query (ex: delete the object queried)
  4. see that the background query is triggered and a request is sent

@apollo/client version

3.9.6

phryneas commented 8 months ago

That is kinda expected/intended behaviour.

useBackgroundQuery only creates a queryRef that a useReadQuery can attach to. Due to how suspense works, we have to keep around that reference, even if the component with useReadQuery would never be mounting.

If you don't attach to it in a useReadQuery within 30 seconds (see autoDisposeTimeoutMs), that subscription will be unsubscribed.

You'll see in your demo that once you add a useReadQuery to the whole thing, it behaves the way you'd expect.

We have to solve it this way (with the autoDisposeTimeoutMs) because we have to keep the subscription alive long enough for the useReadQuery component not only to suspend/try render, but until it is actually rendered and committed to the component tree. That's unfortunately a drawback of how Suspense works in React.

PiR1 commented 8 months ago

Thank you for your answer!

I hadn't seen this option (autoDisposeTimeoutMs) at all.

That's the problem I'm facing, my child component that reads the reference doesn't have time to mount.

But wouldn't it be a good idea, for performance reasons, to unsubscribe the subscription if the component that called the background query is unmounted and the reference hasn't been read?

jerelmiller commented 8 months ago

@PiR1 I'm fairly confident this is related to https://github.com/apollographql/apollo-client/pull/11438. An optimization we made in 3.9 with regards to background queries/query refs is what @phryneas described above, which is that if a queryRef is not read by useReadQuery in a given amount of time (autoDisposeTimeoutMs), the query ref is put in sort of an idle state.

The work in https://github.com/apollographql/apollo-client/pull/11438 however did remove the detection of the background query unmounting. If I try this with 3.8.10 for example, I see the problem you're describing disappear.

Let me think through this a bit more. I stand by that change as I believe it is generally a positive one. But you have a good point about not keeping that subscription around when useBackgroundQuery unmounts before the autoDisposeTimeoutMs has kicked in. Let me play around with a couple things and see if we can get the best of both worlds.

jerelmiller commented 8 months ago

I've got a failing test that demonstrates this issue: https://github.com/apollographql/apollo-client/pull/11651. This also occurs when changing variables on useBackgroundQuery without consuming the queryRef before the autoDisposeTimeoutMs kicks in.

I tried a couple quick ideas without luck. I think this is going to require a bit more thought to try and nail all the cases down here and work well with strict mode. If you've got ideas and would like to contribute, feel free to use the tests in my PR as a base 🙂

PiR1 commented 8 months ago

Thank you for the update! I'll try to have a look on my side too, thanks to the tests you've done.

github-actions[bot] commented 8 months ago

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo Client usage and allow us to serve you better.

github-actions[bot] commented 7 months ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. For general questions, we recommend using StackOverflow or our discord server.