setQueryData can create "dead" queries

[phatmann]

I have two different queries in my app. One query gets messages and one gets conversations.

useQuery('messages', fetchMessages)
useQuery('conversations', fetchConversations)

Each conversation object needs to get populated with the last received message, if any. After the messages are fetched, I look through the conversations cache and then update it appropriately.

useQuery('messages', fetchMessages, {
  onSuccess: (messages) => setQueryData('conversations', existingConversations => {
    if (!existingConversations) {
      return
    }
    
    return updateWithRecentMessages(existingConversations, messages)
})

When the app started up, the messages query would fire before the first conversations query, so existingConversations would be undefined. This undefined value then gets written to the conversations cache. For some reason, this then causes the query function for 'conversations' to never fire. That is, React Query never marks the undefined conversation cache as stale.

Once I realized this, the workaround was easy: don't use an updater function. So the correct code is:

useQuery('messages', fetchMessages, {
  onSuccess: (messages) => {
    const existingConversations = client.getQueryData('conversations')

    if (!existingConversations) {
      return
    }

    setQueryData('conversations', updateWithRecentMessages(existingConversations, messages))
  }
}

I would like other users to avoid this issue, since it caused me a lot of pain and head-scratching. One idea: if the updater function returns undefined, do not set the cache data. If this seems like the right approach, I am happy to make a PR for it.

The combination of the updater function and undefined caches has come up many times. Here are the related discussions and issues:

• TypeScript type for `setQueryData` updater function #925
• setQueryData Function updater call even if there is no cache #834
• there should be a method like queryCache.hasQueryData #486
• typeDefinition of updater function in setQueryData #506
• setQueryData doesn't create a new query #453
• Support setQueryData for new queries #109

I am starting to wonder if having an updater function syntax is a good idea or not. It was created to be "convenient", but the issues with Typescript and undefined caches make it seem less than ideal.

[TkDodo]

One idea: if the updater function returns undefined, do not set the cache data.

As you said, we've been there before :) One of the issues you've linked was also opened by me a long time ago :)
One problem is that undefined could be a legit value for a cached entry 🤷‍♂️. We try not to take any assumptions on that (I think).

The current implementation is made that if you call setQueryData, the cache entry will be created. There is no "bailing out" of it.

Once I realized this, the workaround was easy: don't use an updater function. So the correct code is:

That is also my approach, because with TypeScript, it's very hard to work with the undefined stuff. Maybe if we want bailing out, what we could do is something that react useState is also doing: If you provide the referentially same value, we don't update. This would work for undefined, but also, if the current value is 5, and you set it to 5 as well, we would bail.

The problem could be in the details: looking at the code, we build a cache entry first, and then call setData on it:

https://github.com/tannerlinsley/react-query/blob/255f7c66a8d4a5c33a3b167304f787c7c4acf6f5/src/core/queryClient.ts#L136-L139

since setData needs access to the previous value. So I think it's possible, but we would have to shuffle the code around a bit :)

[TkDodo]

thank you. I just thought about disabled queries: If you have a disabled query, it will create a cache entry in idle state with undefined data. But I don't think it's a problem, right? Currently, if you return undefined from setQueryData, the query will go into success state, and with the new behaviour, it would bail out and stay in idle state, which I think is more correct ... right?

[phatmann]

I cannot see any inherent problem with an idle cache that has undefined data. A user calling getQueryData might think cache does not exist, but if they really care, they can use QueryCache.find.

[phatmann]

When writing unit tests, I noticed that calling setQueryData does not call the onSuccess callback for the cache. (As discussed, the onSuccess callback is called for each query observer). The documentation says that the cache's onSuccess function "will be called if some query is successful". So I am not sure if this is a bug or not. I think for consistency the cache's onSuccess callback should be called for every setQueryData call that triggers the observers' onSuccess. This is what I will do in the PR. And I will update the docs as well to match observer onSuccess criteria.

[phatmann]

The PR is ready to review! Check it out: #2905 @tannerlinsley @TkDodo

[TkDodo]

@phatmann yes, that sounds like a bug. I think I just forgot about that when I introduced that callback, because there is no such thing for the onError callback 😅