Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Background query still active after component unmounted #11649

Closed
PiR1 opened this issue Mar 8, 2024 · 7 comments · Fixed by #11696
Closed

Background query still active after component unmounted #11649

PiR1 opened this issue Mar 8, 2024 · 7 comments · Fixed by #11696

Comments

@PiR1
Copy link
Contributor

PiR1 commented Mar 8, 2024

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
Copy link
Member

phryneas commented Mar 8, 2024

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
Copy link
Contributor Author

PiR1 commented Mar 8, 2024

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
Copy link
Member

@PiR1 I'm fairly confident this is related to #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 #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
Copy link
Member

I've got a failing test that demonstrates this issue: #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
Copy link
Contributor Author

PiR1 commented Mar 11, 2024

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

Copy link
Contributor

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.

Copy link
Contributor

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants