Update query config when new config is provided

[kentcdodds]

Consider:

queryCache.setQueryData("posts", [
  // this could be set to something useful,
  // but it's irrelevant for this example
]);

function usePosts() {
  return useQuery("posts", async () => {
    // 🚨 This function will *never* be called 🚨
    const { data } = await axios.get(
      "https://jsonplaceholder.typicode.com/posts"
    );
    return data;
  });
}

Here's the broken app: https://4cq8j.csb.app/

Here's the code: https://codesandbox.io/s/cranky-goodall-4cq8j?file=/src/index.js (I noticed that Codesandbox's fast-refresh "fixes" the issue though, so don't trust that, open the app by itself).

The problem is simple: The first thing to create a query object will keep that config forever. So because setQueryData is called first and does not provide a queryFn, there's no query function associated with that query and any useQuery('posts', ...) call thereafter will not call the query function to retrieve the data. Resulting in forever stale data.

I think the solution is to merge the query config that's in the query cache for a given query. So at some point when useQuery or setQueryData is called, we'd merge the existing query configuration with the incoming query configuration and update the query object within the queryCache.

[kentcdodds]

What's interesting is that I observe that sometimes it appears that the queryFn will get called. Still investigating the situation. But from what I'm noticing is that the queryCache does not update the queryFn for the query within the cache. So the code that actually does call the queryFn is not getting the function from the cache, but from the hook call. This may explain why codesandbox's fast-refresh makes it work eventually.

[kentcdodds]

If it helps, here's the PR where I update react-query: https://github.com/kentcdodds/bookshelf/pull/66/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2L30-R31

One test fails due to this bug (I guess I should've made an issue instead of a discussion).

[kentcdodds]

If it helps, this test starts breaking on [email protected] (it works with [email protected]). Here's the diff: v2.13.0...v2.13.1

[boschni]

This is caused by the recent change on how queries are configured. Let me try to explain:

The query is a singleton, but there can be multiple hooks attached to the query, each with different configurations and query functions. Previously the query configuration was set during render, which means the query configuration would change multiple times during a render and the eventual configuration was dependent on the rendering order. This could cause inconsistent behaviour.

To resolve this, the query configuration is now set, only when a hook or prefetch executes the query. In this case the hook mounts, but because there is fresh data in the cache, it will not execute the query. When the window is re-focussed, it will try to refetch the last executed configuration, which in this case, does not have a query function defined.

I'm not 100% sure on what the right approach is here, but I also wouldn't expect a setQueryData to cause this behaviour. Let me think a bit about what would be the right thing to do here. Any suggestions are welcome!

[tannerlinsley]

Sounds like we may need to have some parts of the query always updating on render, eg. certain configuration options that only apply to the query instance and not the individual observable/hook instance. One of those is the queryFn which (at least as it is defined today) shouldn't be changing underlying behavior across your application (Note that it's still okay for its identity to change from render to render eg. queryFn: () => goGetStuff())

[tannerlinsley]

Thinking about this more, I'm not sure what's wrong with just updating the core query config with the latest rendered observer config. Anywhere we are relying on the actual config for the query, we'll be pulling it from the query instance itself and it shouldn't be changing across your app, and likewise anywhere we want observer-specific config, we should be pulling it from the observer. Thoughts?

[kentcdodds]

FWIW, I thought that's how it worked already, so seems reasonable to me

[boschni]

Let's try to break down in which situations a query is executed and which configuration is used:

1. queryCache.prefetch -> use given config
2. hook (mount, interval, fetchMore, refetch) -> use hook config
3. window focus / reconnect / invalidate -> use config from last fetch

Currently on window focus, the query will check if there are any observers which indicate they are enabled, stale and have refetchOnWindowFocus set. If so, it will execute the query with the configuration from the last fetch.

Because of the issue mentioned here, and also because the hook which triggered the last fetch might not be mounted anymore, maybe it would be better if it would use the configuration from the first observer which is enabled, stale and has refetchOnWindowFocus set? An MR to enable this behaviour can be found here: #964 .

When updating on render it could happen that the query would use a configuration from a hook which is disabled, not stale, or has refetchOnWindowFocus set to false.

[alfrescian]

I wasn't aware of this discussion and created an issue regarding this behavior: #963
The scenario there is a bit more complicated as it's using SSR prefetchQuery, cache hydration and SSR but to me it looks to be caused by the same problem.

Note: even when providing a query-config in prefetch-query then only cacheTime seems to be passed to the client-side., further options like staleTime, refetchOn... are always using the default. Here's the sandbox https://codesandbox.io/s/distracted-pond-h45px

[Ephem]

I understand why it seemed that way, but the reason that sandbox didn't work wasn't that the other config values didn't get passed to the client side (I commented in the issue). Since it isn't obvious why we only pass some values, I wanted to take the time to explain that quirk:

Right now the idea for hydration is that we only include the values we need right away when we dehydrate a query, which is cacheTime, data and updatedAt. The rest of the values can be recreated the first time the query is used on the client instead, that is, we use the config from useQuery etc. We do this to keep the payload minimal, since if we passed along the entire config, that would exist both in the html and the JS-payload that gets sent to the client.

I think we need to do a better job documenting this quirk. 😄

[boschni]

@kentcdodds the issue should be fixed now in 2.15.1

[kentcdodds]

It looks like it does update the data when I click around the app, but on the initial load and mount of the useQuery hook completely ignores the queryFn. I would expect that if the staleTime was set when I set the data, but it's not.

[boschni]

Ah yes I see what you mean. The issue with the window refocussing has been fixed, but the hook should also fetch on mount. Initially I thought the hook did not fetch because setQueryData had just set fresh data, but it indeed should. We need more tests to cover these behaviours. Created a fix for it in #966

[kentcdodds]

Super, I'm 99% certain that fixed it. Thanks!

[kentcdodds]

Confirmed. This fixed the bookshelf repo. Thanks a bunch!

[tannerlinsley]

Nice! Thanks Kent!

[squarethecircle]

@tannerlinsley @boschni I'm having this same issue, but coming through refetchQueries instead. Basically, the sequence looks like this:

1. I call setQueryData to add some preloaded data to my cache, with positive staleTime so it's not immediately refetched on query mount.
2. I mount my query, which doesn't run because of the positive stale time.
3. I trigger a mutation, and invalidate my query cache on success -- but because the queryFn in the cache config is () => Promise.reject(), as initialized by setQueryData, I get an error here instead of refetching data as I would expect -- refetchQueries just reads from the core cache, without looking at observers.

I noticed that @tannerlinsley suggested above

Thinking about this more, I'm not sure what's wrong with just updating the core query config with the latest rendered observer config. Anywhere we are relying on the actual config for the query, we'll be pulling it from the query instance itself and it shouldn't be changing across your app, and likewise anywhere we want observer-specific config, we should be pulling it from the observer. Thoughts?

It seems like this would solve this class of problems once and for all -- is this a possibility?

Update:
This one-line change does indeed solve my issue, although I'm not sure if it breaks anything else: squarethecircle@8c8eb0a
Are there any issues with this approach?

[boschni]

Does v3 fix the problem for you?