Skip to content

fix(react-data-query): Retain queries in cache until GC#9502

Open
FrederikBolding wants to merge 4 commits into
mainfrom
fb/stop-removing-queries-from-ui-cache
Open

fix(react-data-query): Retain queries in cache until GC#9502
FrederikBolding wants to merge 4 commits into
mainfrom
fb/stop-removing-queries-from-ui-cache

Conversation

@FrederikBolding

@FrederikBolding FrederikBolding commented Jul 14, 2026

Copy link
Copy Markdown
Member

Explanation

Following this PR, instead of automatically removing queries from the UI query client when a BaseDataService instance evicts it from the cache, we retain the query in cache until the QueryClient itself cleans it up (via cacheTime).

This prevents issues with observers in the UI having their cache wiped just because a refetch hasn't happened in a while. Since the UI query client considers the data stale, it may also choose to refetch and repopulate the query in the BaseDataService instance before eviction anyways.

References

https://consensyssoftware.atlassian.net/browse/WPC-1168

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've communicated my changes to consumers by updating changelogs for packages I've changed
  • I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

Note

Medium Risk
Changes cache lifecycle for all data-service-backed queries; behavior shifts from immediate UI eviction on service removal to TanStack cacheTime, which could affect stale-data visibility but fixes unintended cache wipes for active observers.

Overview
UI query client no longer drops cached data when BaseDataService evicts a query. The granular cacheUpdated listener now ignores removed payloads instead of calling cache.remove, so active observers keep their data until TanStack Query’s own cacheTime garbage collection runs after subscriptions end.

Tests replace the old “sync removal on remove event” case with scenarios using fake timers: data stays in cache while observers remain subscribed even after long simulated time, and entries clear only after observers are destroyed and cacheTime elapses. Changelog documents the fix under Unreleased.

Reviewed by Cursor Bugbot for commit ada6f7a. Bugbot is set up for automated code reviews on this repo. Configure here.

@FrederikBolding FrederikBolding marked this pull request as ready for review July 14, 2026 13:33
@FrederikBolding FrederikBolding requested a review from a team as a code owner July 14, 2026 13:33
@FrederikBolding FrederikBolding force-pushed the fb/stop-removing-queries-from-ui-cache branch from dc46c9c to ada6f7a Compare July 14, 2026 13:34

it('proxies requests to the underlying service', async () => {
const { clientA: client } = createClients();
const { clientA: client, service } = createClients();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Maybe we should change createClients to withClients and have it automatically call service.destroy? Seems like this would make writing tests easier in the future (but of course also adds to the size of this PR).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that these tests will get hard to maintain long term, we can resolve this separately though

await Promise.all([promiseA, promiseB]);

const hash = hashQueryKey(getAssetsQueryKey);
jest.advanceTimersByTime(inMilliseconds(1, Duration.Day));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this duration come from? Does it make sense for this duration to be set explicitly in the test somehow? Otherwise it makes this test a bit magical.

@FrederikBolding FrederikBolding Jul 15, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a comment would help? This is cacheTime for ExampleDataService:

cacheTime: inMilliseconds(1, Duration.Day),

});
});

jest.advanceTimersByTime(0);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary? I've seen this in other tests but can't remember why. Is GC occurring at this step or no?

@FrederikBolding FrederikBolding Jul 15, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All observer notifications from TanStack look something like this: setTimeout(() => notify(), 0);. So it is required in order for the subscription promise to resolve when using fake timers.

@FrederikBolding FrederikBolding requested a review from mcmire July 15, 2026 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants