useQueries with keepPreviousData:true can get to infinite fetch loop when toggling a query off and on

[jvuoti]

When using useQueries to issue optional queries, you can seemingly get to an infinite fetch loop if you toggle the same query off and on again. This seems to only happen when having the keepExistingData: true set, as well as only when you toggle some other query than the last one.

We noticed this in one of our views which uses useQueries to fetch one or two data series, depending on what the user chooses. If the user toggles one data series off and then on again, the view will start to refetch all the data over and over again.

To Reproduce
Steps to reproduce the behavior:

1. Go to https://codesandbox.io/s/nice-sid-9tkgi?file=/src/index.js -> the view should load two queries
2. Click on 'Fetch with ID 1' to toggle the query off -> only query with ID 2 is fetched and result is shown
3. Click on 'Fetch with ID 1' again to toggle the query -> both queries are fetched and result shown
4. However, checking the dev tools you can see both the queries stay in "fetching" state. Also the console shows the queries are being issued over and over again:

Expected behavior
If you toggle the first query back on, the data should be fetched again only once.

Additional info

react-query version 3.23.2

It seems that if you toggle only the last query off and on, this does not happen. e.g. in the codesandbox, toggle "Fetch with ID 2" off and on again, and no additional fetches are issued.

Also, this only seems to happen if you have the keepExistingData:true set for the queries. We have now turned that off for our queries, and the view no longer issues extra queries.

Desktop:

• OS: MacOS Big Sur 11.6
• Browser: Chrome 93.0.4577.63, Firefox 91.0.2

[TkDodo]

Thanks for reporting. I contributed that keepPreviousData works for useQueries not too long ago (it wasn't doing anything at all before that). The PR is here if you're curious: #2340

It would be great if you could take a look into this, I think we need to respect the enabled flag somehow - I clearly wasn't anticipating one query getting disabled, my main concern was getting it to work when the length of the query array changes between renders :/

[jvuoti]

Thanks for the quick response! Looking at that PR, using the index to get the observer there might have an effect why this happens only for the first query and not the last one...

Haven't worked on this library myself yet, but I could try to take a look on this next week. Thanks for pointing to the correct point in code :)

[TkDodo]

In the meantime, not sure if it's feasible for your use-case, but if you keep the length of useQueries stable and work with the enabled property instead to toggle queries on / off, the previous data will be kept correctly. Here is your sandbox refactored to that approach: https://codesandbox.io/s/heuristic-chebyshev-fdzdx?file=/src/index.js

[TkDodo]

I have also transformed your example into a minimal test-case (assertions still missing because I don't know yet what the exact result should be):

  it('should keep previous data when toggling queries', async () => {
    const key = queryKey()
    const states: UseQueryResult[][] = []

    function Page() {
      const [enableId1, setEnableId1] = React.useState(true)
      const ids = enableId1 ? [1, 2] : [2]

      const result = useQueries(
        ids.map(id => {
          return {
            queryKey: [key, id],
            queryFn: async () => {
              await sleep(5)
              return id * 5
            },
            keepPreviousData: true,
          }
        })
      )

      states.push(result)

      React.useEffect(() => {
        setActTimeout(() => {
          setEnableId1(false)
        }, 20)

        setActTimeout(() => {
          setEnableId1(true)
        }, 20)
      }, [])

      return null
    }

    renderWithClient(queryClient, <Page />)
  })

[TkDodo]

out of curiosity @jvuoti : why do you have keepPreviousData turned on in your example? It doesn't really do anything, because there is no key change that would make keepPreviousData do anything 🤔 . The codesandbox runs fine without that flag on ...

[jvuoti]

Oh yeah, I tried to make the codesandbox as minimal a reproduction as possible - but it is not that realistic now 😅

In our actual case we have a view where the user can view a data series or compare it to another series. The data is basically financial statement data, so the user can e.g. compare realized income to the budget, or two different budget versions etc. The user may then go back and forth when comparing data series, e.g. compare the current realized income to budget version A, then B - and then again to version A. When the user selects a series they had previously viewed, the issue with the infinite requests gets triggered.

Generating the queries for all of the series and just toggling the enabled property could be a good idea, but we can have so many different series (tens or hundreds with all the possible query filter combinations), unfortunately it would not work for us.

We wanted to use keepPreviousData for the queries to reduce the page "jumping". Now that we turned it off, when the user selects a new series, the table we have for showing the data will momentarily clear out before showing the new series. This then reduces the height of the table, moving the rest of the elements on the page etc. So not having keepPreviousData on is more of an annoyance, which I think we can work around by tweaking our layout. But it would be nice to fix this issue, as suddenly getting lots of repeat queries hitting the backend is quite surprising 😬 I'll see if I can look into this more tonight.

[TkDodo]

One problem is which previous data to keep. If we have the following keys:

["key", "something", 1] --> returns an array of strings
["key", "somethingElse", 2] --> returns an array of numbers

and we then remove the first entry and, at the same time, change the query key of the second query, we will wind up with:

["key", "somethingElse", 22] --> returns an array of numbers

it seems quite impossible to know that keepPreviousData should keep the data of the previous ["key", "somethingElse", 2] key, because it has no correlation to it (different query key and different position). If we keep the array of strings for the 0th element of queries from the previous array, we will likely keep wrong data.

the only solution that comes to my mind now is to, again, disable keepPreviousData if the query arrays have different lengths altogether

[jvuoti]

I now debugged the issue a bit, and I think the problem is somehow with the observers list and getting existing data by index in queriesObserver.getObserver(): Now it tries to find the query by hash, or by index if not found and keepExistingData === true. However, as the order of the queries is not necessarily always the same, it seems that it can now keep on returning the wrong observer.

For instance, in the codesandbox example, originally the query ids are [1,2] and the this.observers list will have query hashes ["[1]","[2]"]. If you toggle query 1 off, the ids list will be [2], and the this.observers query hashes correspondingly just ["[2]"].

Now, if you then toggle query 1 back on again, the ids will again be [1,2], but the previous this.observers list will still be ["[2]"] at first. So when the observer with hash "[1]" is not found, this.observers[0] is returned - and that is now the observer with query hash "[2]". In this case (adding a new query) it probably should rather return a new observer?

Anyways, in the end new observers list will actually be for ["[2]","[2]"]. And then when the next render update comes, the observer for hash "[2]" will again be found, and "[1]" not - and then it will again default to this.observers[0], and that is again the one with hash "[2]".

The actual results for query 1 still come through and they are shown. However, this all seems to cause the queries to fetch again, perhaps because the observers list gets recreated? Didn't yet have time to dig deeper into that, will try to do that later.

Also, as the code in queriesObserver.updateObservers() uses the query hash from the query, not observer, so the newObserversMap may also have the wrong observer for the queryHash. Testing it out, I noticed that if you reduce the cache time for the query, e.g. to 100 ms in the current sandbox, you may initially get the results for query 2 for query 1. The data will be correct after the first fetch for query 1 returns, but still you can get shown a flash of wrong data 😬

So I'm not yet sure how to fix this. Limiting the keepPreviousData settings to cases where the amount of queries is the same might help, but I'm not quite sure. E.g. in our use case, the user may just change the a query to another, so the amount of queries stays the same, but the order of the queries may change, i.e. you could change from ids [1,2] to [2,3]. With the current logic, this could also cause the observers list to again only be ["[2]","[2]"], I think.

Perhaps we could try to get the existing observers by query hash first, so the queries that stay the same would not be affected in any case. Then we could gather the queries with no matches as well as observers no longer used to separate, sorted lists. From there we could match the values by index. Or perhaps by more complicated logic, e.g. by matching as much of the key as possible.

E.g. if the user has changed queries from [1,2] -> [2,3], we would first notice that query 2 is has an observer, so use that. The only unmatched new observer would be for 3, and the only old existing one is for 1. Then it probably would be safe to assume we can just reuse the observer for id 1 for the new query, getting the existing data from there.

However, looking at the code, this might complicate the logic quite a bit. I can try it out a bit, most likely tomorrow night.