feat: query cache provider #476

jackmellis · 2020-05-10T07:24:13Z

the query cache now comes from a context, meaning users can easily override and control the cache being used inside their application

see #460

this is a basic use case of creating an isolated cache for testing

in component:

const MyComponent = () => {
  const queryCache = useQueryCache();

  const [ mutate ] = useMutation(someFn, {
    onSettled() {
      queryCache.refetchQueries('basket');
    }
  });
  return ( ... );
};

in unit test:

import { makeQueryCache, QueryCacheProvider } from 'react-query';

const cache = makeQueryCache();
sinon.spy(cache, 'refetchQueries');


mount(
  <QueryCacheProvider queryCache={cache}>
    <MyComponent/>
  </QueryCacheProvider>
);

// ...

await invokeSomeMutation(); // after mutating, we should refetch the basket

expect(cache.refetchQueries).to.be.calledWith('basket');

tannerlinsley · 2020-05-10T16:01:32Z

Nice! I would love to see the tests converted to use this. Thoughts?

jackmellis · 2020-05-10T21:29:07Z

Sure I can update them if you like

tannerlinsley · 2020-05-10T21:35:45Z

If it’s not too much trouble! 👍

jackmellis · 2020-05-11T06:11:37Z

I've updated the tests. I sometimes get a warning about overlapping act() calls. I can't figure out why and I'm not sure if it was happening before or if I've uncovered something...

tannerlinsley · 2020-05-11T13:37:02Z

src/setFocusHandler.js

@@ -9,7 +9,7 @@ const onWindowFocus = () => {
  const { refetchAllOnWindowFocus } = defaultConfigRef.current

  if (isDocumentVisible() && isOnline()) {
-    queryCache
+    defaultQueryCacheRef.current


Hmmm. Does this feel funny to you? What if a user had multiple query caches in their app? How would they hook up to this? Should multiple caches be allowed?

Yes it definitely felt funny! But as you can set up the focus handler outside of any sort of react context, it's very hard to determine which cache to use.

I used the same pattern as the config where there is a defaultConfigRef - that the provider writes over. The focus handler actually uses the config ref as well so it's already a bit of a grey area...

for reference https://github.com/tannerlinsley/react-query/blob/master/src/setFocusHandler.js#L9

Definitely open to cleaner solutions!

Since this specific config value is "global", it kind of do make sense to use the latest one. For caches, maybe we should keep track of all active caches and update them all? I think that should make it possible to get rid of defaultQueryCacheRef?

I've removed the defaultQueryCacheRef in favour of a list of active caches

Ephem · 2020-05-11T16:52:06Z

Nice work! This also relates to server rendering as per #461, if you want I can take a closer look and test it from that perspective tomorrow. 😄

tannerlinsley · 2020-05-11T17:03:50Z

@Ephem Awesome, since this is clearly cross-cutting, I'll let you test before proceeding.

Also, just to confirm, this will all still be backwards compatible right? With the main goals being:

Truly isolated tests
SSR isolation

Confirm?

Ephem

First of all, very nice work on this, I like the approach you are taking!

Just so my view on this PR is clear I think the goal should be to support explicit caches for testing, using different caches for different parts of the application/separate React roots and laying the foundation to build improved SSR support on top, but not change SSR behaviour in this PR.

Tests

All internal tests are now updated to the new approach, which is great, but this also means the old approach is currently untested. Maybe add a few key tests targeting using the global cache, without a Provider?
Maybe add a few tests using multiple caches/providers?

Query cleanup

I think an existing major issue right now is cache cleanup. I think the issues #432 and #460 are both related to this and also the act warnings in this PR. I think this could and should be fixed in this PR since the problem might actually become worse otherwise.

EDIT: This could just as well be done in a separate PR before this is merged.

Let me expand:

I'm pretty sure these problems stem from query timeouts not being cleared.

cache.clear = () => {
  cache.queries = {}
  notifyGlobalListeners()
}

The old cache clear function does not in any way remove the pending query timeouts for staleness etc. To get rid of these problems, I think calling cache.clear() should also synchronously loop through all queries and remove all timeouts related to that query.

If a user creates a cache manually with makeQueryCache, they own the cache and are responsible for calling cache.clear().
If react-query creates a cache, the library owns that cache and needs to clean it up if it is no longer used (for example, in ReactQueryCacheProvider).

Serverside rendering

This approach looks great from a SSR-perspective and this PR implements a key part of what is needed to improve that story. There is more work that is needed on top, but that should be done in a follow up and I see nothing that wouldn't be compatible!

Ephem · 2020-05-12T07:12:44Z

src/queryCache.js

+}
+
+export function ReactQueryCacheProvider({ queryCache, children }) {
+  defaultQueryCacheRef.current = queryCache || makeQueryCache()


If queryCache is not provided, this will create a new queryCache on every render. Maybe this is also something to add a test for to avoid regression later?

It should only create it once. That looks like it will be created on every render (if the queryCache isn't passed)

I've cleaned this logic up now so it tracks queryCache instances between renders

src/useBaseQuery.js

Ephem · 2020-05-12T09:02:21Z

src/setFocusHandler.js

@@ -9,7 +9,7 @@ const onWindowFocus = () => {
  const { refetchAllOnWindowFocus } = defaultConfigRef.current

  if (isDocumentVisible() && isOnline()) {
-    queryCache
+    defaultQueryCacheRef.current


Since this specific config value is "global", it kind of do make sense to use the latest one. For caches, maybe we should keep track of all active caches and update them all? I think that should make it possible to get rid of defaultQueryCacheRef?

src/queryCache.js

the query cache now comes from a context, meaning users can easily override and control the cache being used inside their application

this commit does the follwing: track all active query caches added via the provider, handles mounting/unmounting if creating a cache, ensure we only do it once when unmounting an internally-created cache, make sure to clear it first setFocusHandler now loops through all of the currently-active caches

jackmellis · 2020-05-14T20:30:34Z

First of all, very nice work on this, I like the approach you are taking!

Just so my view on this PR is clear I think the goal should be to support explicit caches for testing, using different caches for different parts of the application/separate React roots and laying the foundation to build improved SSR support on top, but not change SSR behaviour in this PR.

Tests

All internal tests are now updated to the new approach, which is great, but this also means the old approach is currently untested. Maybe add a few key tests targeting using the global cache, without a Provider?

Maybe add a few tests using multiple caches/providers?

Query cleanup

I think an existing major issue right now is cache cleanup. I think the issues #432 and #460 are both related to this and also the act warnings in this PR. I think this could and should be fixed in this PR since the problem might actually become worse otherwise.

EDIT: This could just as well be done in a separate PR before this is merged.

Let me expand:

I'm pretty sure these problems stem from query timeouts not being cleared.
cache.clear = () => {
  cache.queries = {}
  notifyGlobalListeners()
}
The old cache clear function does not in any way remove the pending query timeouts for staleness etc. To get rid of these problems, I think calling cache.clear() should also synchronously loop through all queries and remove all timeouts related to that query.

If a user creates a cache manually with makeQueryCache, they own the cache and are responsible for calling cache.clear().

If react-query creates a cache, the library owns that cache and needs to clean it up if it is no longer used (for example, in ReactQueryCacheProvider).

Serverside rendering

This approach looks great from a SSR-perspective and this PR implements a key part of what is needed to improve that story. There is more work that is needed on top, but that should be done in a follow up and I see nothing that wouldn't be compatible!

I've added tests to show the global/default cache still works and a few tests for different caching scenarios

The provider now tracks its queryCache value and if it's a self-created one, it cleans it up and removes it on unmount. If the cache is manually created, it's up to the creator to manage cleanup.

I haven't touched on the in-depth cache cleanup (cancelling timeouts etc.) as I don't want to lose the focus of this PR. Happy to take this on separately though if nobody is working on it...

Ephem · 2020-05-15T06:26:53Z

Changes and tests looks great to me, really great work!

I totally agree with addressing cache cleanup in a separate PR. Since it's possible that using a lot of Providers in tests could worsen the existing problems (more timeouts) I think it should probably be addressed before a release.

README.md

Co-authored-by: Tanner Linsley <[email protected]>

tannerlinsley · 2020-05-15T21:09:58Z

🎉 This PR is included in version 1.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

jackmellis force-pushed the feat/use-query-cache branch from fde53ef to 66777c6 Compare May 11, 2020 06:10

tannerlinsley reviewed May 11, 2020

View reviewed changes

Ephem requested changes May 12, 2020

View reviewed changes

Jack Ellis added 3 commits May 14, 2020 21:07

feat: query cache provider

44dc8ef

the query cache now comes from a context, meaning users can easily override and control the cache being used inside their application

test: add in-depth tests around query cache providers

7bf45e1

jackmellis force-pushed the feat/use-query-cache branch from 66777c6 to 84eb13c Compare May 14, 2020 20:09

style: format touched files

3c8329d

jackmellis force-pushed the feat/use-query-cache branch from 84eb13c to 3c8329d Compare May 14, 2020 20:22

jackmellis requested a review from tannerlinsley May 14, 2020 20:33

fixup! style: format touched files

87bf958

jackmellis mentioned this pull request May 15, 2020

fix: clear cache timeouts #495

Merged

tannerlinsley requested changes May 15, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

docs: correct readme description

7371990

Co-authored-by: Tanner Linsley <[email protected]>

tannerlinsley approved these changes May 15, 2020

View reviewed changes

tannerlinsley merged commit 51375fd into TanStack:master May 15, 2020

tannerlinsley added the released label May 15, 2020

Ephem mentioned this pull request Jun 14, 2020

Support per-request cache with SSR #461

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: query cache provider #476

feat: query cache provider #476

jackmellis commented May 10, 2020 •

edited

Loading

tannerlinsley commented May 10, 2020

jackmellis commented May 10, 2020

tannerlinsley commented May 10, 2020

jackmellis commented May 11, 2020

tannerlinsley May 11, 2020

jackmellis May 11, 2020 •

edited

Loading

jackmellis May 11, 2020

Ephem May 12, 2020

jackmellis May 14, 2020

Ephem commented May 11, 2020

tannerlinsley commented May 11, 2020

Ephem left a comment •

edited

Loading

Ephem May 12, 2020

tannerlinsley May 12, 2020

jackmellis May 14, 2020

Ephem May 12, 2020

jackmellis commented May 14, 2020

Ephem commented May 15, 2020

tannerlinsley commented May 15, 2020

feat: query cache provider #476

feat: query cache provider #476

Conversation

jackmellis commented May 10, 2020 • edited Loading

tannerlinsley commented May 10, 2020

jackmellis commented May 10, 2020

tannerlinsley commented May 10, 2020

jackmellis commented May 11, 2020

Choose a reason for hiding this comment

jackmellis May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ephem commented May 11, 2020

tannerlinsley commented May 11, 2020

Ephem left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackmellis commented May 14, 2020

Ephem commented May 15, 2020

tannerlinsley commented May 15, 2020

jackmellis commented May 10, 2020 •

edited

Loading

jackmellis May 11, 2020 •

edited

Loading

Ephem left a comment •

edited

Loading