-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
fix(scheduleGarbageCollection): Do not garbage collect newly cached queries of the same hash #332
Conversation
I think I found the issue: The following conditions have to be true it seems:
Here is the sequence of events after a test is done:
So the reason that the RTL patch fixes things is because it ensures the scheduled query garbage collections occurs before the next test is run. There could be several ways to fix this but essentially |
@@ -13,7 +12,6 @@ import { sleep } from './utils' | |||
describe('useQuery', () => { | |||
afterEach(() => { | |||
queryCache.clear() | |||
cleanup() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is called automatically by RTL?
Ohhhhh yeah. This is good stuff! |
tl;dr
When a query is unmounted, it schedules a GC. Normally this is fine but during a test run, we clear the cache before components get unmounted automatically (at least in
@testing-library/react
) so the GC still runs and wipes out the next test's cached queries.Background
Tracked down a very tough-to-isolate case where when using
manual: true
around queries will cause test inconsistencies when trying to use prime the cache usingsetQueryData
orprefetchQuery
.To reproduce this, you have to import the components from another module, you can't declare them inline in the suite. I tried that initially and things were passing.
It appears that not everything is cleaned up on
unmount
like you'd expect in this case, since running each test independently (using.only
) will pass, but when run sequentially, the second fails.If I use this patch of RTL I made at testing-library/react-testing-library#632 then the tests will pass, which leads me to think this an underlying issue with react-query.
If I prefetch both queries each time, the tests will pass but the point of this is to showcase that if I don't do that, I get test inconsistency. In the real world, we ran into this because we have 6+ queries that run in a large composite component and we aren't mocking each one for every test, just the queries that are being tested.
Changes
queryCache
specstate.markedForGarbageCollection
flag to check before removing queriesProposed Solution
I think this is a fairly streamlined solution now. I have introduced a new action
actionMarkGC
that sets thestate.markedForGarbageCollection
flag totrue
.When the
query.scheduleGarbageCollection()
timeout runs, it makes sure it only removes the queries where the flag istrue
and the query hash matches.This should avoid accidentally removing a newly cached entry.