#1775 caused useQueries to return unstable values between render cycles

[szszoke]

Describe the bug
The array returned by useQueries is not stable between render cycles which makes it unusable for anything down the line that uses shallow equality to determine if something had changed.

Small snippet to illustrate the bug

const responses = useQueries(preparedQueries);

console.log("changed?");
useEffect(() => console.log("changed!"), [responses]);

Console output

changed?
changed!
changed?
changed!
...

Expected behavior
The return value of useQueries is stable between render cycles as long as the queries did not change. The returned value can be used as a dependency for hooks like useMemo/useEffect which use shallow equality checks to determine if a dependency had changed since the last render

Desktop (please complete the following information):

• OS: Linux
• Browser: Firefox
• Version: 86

Additional context
The new behavior was introduced via #1775.

Relevant code lines:

https://github.com/tannerlinsley/react-query/blob/f2137dc4e4553256c4ebc1891b548fe35efe9231/src/react/useQueries.ts#L29

https://github.com/tannerlinsley/react-query/blob/f2137dc4e4553256c4ebc1891b548fe35efe9231/src/core/queriesObserver.ts#L67

[szszoke]

Versions after 3.8.3 are affected.

[TkDodo]

it was mentioned here, in 3.9.0 for concurrent mode safety:

Unfortunately I was not able to keep the referential integrity of the result object between renders because we do not know which optimistic result will eventually get committed.

is .data in each item referentially stable? I get that this might not be of much help if you are having a variable amount of queries

[szszoke]

it was mentioned here, in 3.9.0 for concurrent mode safety

Then I didn't do my homework. I apologize for that.

is .data in each item referentially stable?

I don't actually know that because my code was relying on the returned array being referentially stable.

I think I will be able to work around this issue for now by using Promise.all and queryClient.fetchQuery inside an effect.

The documentation for useQueries should probably mention that the returned array is not stable between render cycles.

[TkDodo]

The documentation for useQueries should probably mention that the returned array is not stable between render cycles.

yes, would you like to do that? It's also the case for useQuery. If we mention that, we should also mention that data is stable, very much so actually, even if background refetches occur, and even for selecting a slice of the data, you will get the same reference unless something changed.

[szszoke]

It would likely be useful to have a page where the referential stability of the hooks is discussed. With useQuery it is sort of implied that unless your query parameters changed or the data becomes stale in your cache, you get the same cached data between render cycles.

I can try writing a new page, or just extend the existing pages.

Maybe it is better to extend the existing pages.

What do you think?

[TkDodo]

sure, please open a PR that addresses it in the docs, extending the existing pages is fine. I'll close this issue.

[jcready]

That is quite a huge (see: disastrous) change that was introduced. It's unclear to me what the use for useQueries is if it doesn't return a stable result. And it's not mentioned in the docs at all. And this ticket was closed.

Edit: also a breaking change in a minor version is not a great look

[TkDodo]

It's unclear to me what the use for useQueries is if it doesn't return a stable result.

The use case is to run a variable amount of queries. I don't see what referentially stable return values have to do with this?

also a breaking change in a minor version is not a great look

I don't see the library guaranteeing to return the same array in every render, so it's technically not a breaking change ;)

---

On a more serious note, I see that this change is more severe for useQueries then it is for useQuery. With useQuery, you can just destruct data and get a stable reference:

const { data } = useQuery()

while with useQueries, you would get a new array every time, and even if you .map over it to get just the data, it would still be a new array every time:

const allData = useQueries(...).map(it => it.data)

---

@jcready maybe you can outline your use-case a bit, what you are doing with the result of useQueries that requires referential stability, and maybe we can find a solution to the problem?

[jcready]

The use case is to run a variable amount of queries. I don't see what referentially stable return values have to do with this?

You don't see what referentially stable return values for useQueries has to do with a ticket titled "#1775 caused useQueries to return unstable values between render cycles"?

I don't see the library guaranteeing to return the same array in every render, so it's technically not a breaking change ;)

That's not what defines a breaking change. In fact, the maintainers of this library were well aware of the breaking change and introduced it in a minor version bump anyways:

Unfortunately I was not able to keep the referential integrity of the result object between renders because we do not know which optimistic result will eventually get committed.

🎉 This PR is included in version 3.9.0 🎉

Just look at all the tickets that point back to #1775

maybe you can outline your use-case a bit, what you are doing with the result of useQueries that requires referential stability, and maybe we can find a solution to the problem?

The use-case is pretty straight forward. Use the result of useQueries in pretty much any other react hook that accepts a dependencies array: useEffect, useLayoutEffect, useCallback, useMemo, and all other react hooks that use those hooks as building blocks.

For anyone looking for a way to work around this breaking change this is what I've had to setup:

1. First grab a copy of the useMemoCompare hook
2. Create some helper functions:

   export function compareQuery<D>(a: UseQueryResult<D>, b: UseQueryResult<D>): boolean {
       return (
           a.data === b.data &&
           a.status === b.status &&
           a.dataUpdatedAt === b.dataUpdatedAt &&
           a.errorUpdatedAt === b.errorUpdatedAt
       );
   }
   
   export function compareQueries<D>(a: UseQueryResult<D>[], b: UseQueryResult<D>[]): boolean {
       return a === b || (a.length === b.length && a.every((q, i) => compareQuery(q, b[i])));
   }

3. Then you can use it with a useQueries result like this:

   const stableQueryResults = useMemoCompare(
       useQueries(/*...*/),
       compareQueries
   );
   
   const allData = useMemo(() => stableQueryResults.map(it => it.data), [stableQueryResults]);

This solution is definitely sub-optimal and may not handle every case, but it's better than the alternative: running even more expensive operations on the unstable useQueries result.

[TkDodo]

yes, I think the tradeoff of making the library adhere to the "rules of react" (no side effects during render), also dubbed as "works in concurrent mode" trumped the referential stability. I'm sorry if this has caused a headache for you. Its debatable if it is considered a breaking change imo, but that ship has sailed. Maybe we can revisit the implementation once React 18 comes out and we have access to things like useSyncExternalStore

the largest use case for useQueries is still rendering the result of multiple queries:

const queries = useQueries(...)

queries.map({ data } => <Component data={data} />)

since data on each element is still stable, these cases are not affected. Also, performing expensive aggregations via the select option on each query should still be optimized / only executed when something changed:

const queries = useQueries(
    something.map(x => ({ queryKey: x, queryFn: fetchX, select: expensiveTransformation })
)

maybe this can help you move the expensive part of your calculation into select which then potentially leaves you with a non-expensive aggregation (again, depends).

finally, still not ideal, but if the number of queries is known, you can destruct data and work with multiple of those.

[ivanjeremic]

Maybe we can revisit the implementation once React 18 comes out and we have access to things like useSyncExternalStore

Is latest react-query now making use of useSyncExternalStore internally?

[TkDodo]

Is latest react-query now making use of useSyncExternalStore internally?

v4 does, yes.

[itayganor]

v4 does, yes.

It still seems like useQuery not useQueries return stable objects. Example.

For context (pun unintended), I'm willing to save a query with user login data in the application's context. According to the eslint rule react/jsx-no-constructed-context-values, you have to memoize context values. So it's all a bit confusing.

[TkDodo]

the top level object that's returned from useQuery is not referentially stable - the .data property is.

[itayganor]

the top level object that's returned from useQuery is not referentially stable - the .data property is.

Is it something that is likely to change sometime?

As I described, this behavior makes keeping queries in context inefficient. It is usually desired to keep them in contexts so you could render its different states (loading and error), rather than just displaying the data.

[TkDodo]

I don't think there are good reasons to put the whole query result into context. Are you aware that you can just call useQuery wherever you want to get the data? Maybe state what problem you're trying to solve with putting it in context...

[itayganor]

Are you aware that you can just call useQuery wherever you want to get the data?

I'm aware, and it does solve the problem I'm describing. But just for convenience:

Consider a userContext, where you have information and actions for the logged-in user.

function UserContextProvider({children}: {children: ReactNode}) {
    const userQuery = useQuery(['user'], () => getUser());
    const userPreferences = useUserPreferences();
    // ... other user-related things

    const value = useMemo(() => ({
        userQuery,
        userPreferences,
    }), [userQuery, userPreferences]);

    return (
        <userContext.Provider value={value}>
            {children}
        </userContext.Provider>
    );
}

Let's say you have two places where you render a user's name - in the header and the comments section.
In both places, you want to display a skeleton loader until data arrives - only then you'll be able to display the name.
To achieve this, you have to have the query object in both places.

I find it right to store user-related data in such userContext, including the query object. In such context there's a single query, with a single source-of-truth of query settings and dependencies.

The only other way to access the same query object (with the same dependencies, logic and query options) in multiple parts of the app, is to wrap your usage of useQuery with a custom hook:

function useUser() {
    return useQuery(['user'], () => getUser(), {
        // ...arbitrary options
    });
}

Due to which. other user data (such as preferences) is separated from other user data sources.

I find it simpler to access "everything user" from a single place: the user context.

[TkDodo]

const useUserContext = () => {
  const userQuery = useQuery(['user'], () => getUser());
  const userPreferences = useUserPreferences();
  // ... other user-related things

  return {
    userQuery,
    userPreferences,
  }
}

this is pretty much the same, just without actual "context" ?

[itayganor]

this is pretty much the same, just without actual "context" ?

I agree it will work in this game, but once you add a state (what if useUserPreferences is a wrapper of useState)? you'll have multiple sources of truth :\

[TkDodo]

what if useUserPreferences is a wrapper of useState

then you would probably put that state into context and then make a custom hook that reads from this context + from useQuery to combine the two. But there is no reason to put the query itself into context.

Basically, you have two things (A + B) and you want to derive something from that (C) and put that into context, whereas I would put just A in context and derive C on the fly (with a hook) from Context + B (B being the query here).

For the consumers, nothing changes because it doesn't matter if a hook only reads from context or does more things.

[joshribakoff-sm]

The use case is to run a variable amount of queries. I don't see what referentially stable return values have to do with this?

Because it's a React library, and React works based upon referentially stable values. This also caused 100% CPU usage infinite loop on my end, which was very hard to debug.

The use case is exactly as stated. Variable number of queries. Even if I useMemo() on the data itself, it's not referentially stable. In my case it was being passed to a useEffect as part of "debouncing" and it ended up causing infinite loops that lock up the tab and require killing the tab (making it hard to debug).

Perhaps you could elaborate on the problem with having useQueries return a stable array? Seems like you believe making the array stable causes bugs (and want to save your users from these bugs). Users are going to make the array stable anyways via workarounds because they don't understand the bugs. If there are bugs, it would be helpful to explain that better for us (and the alternative approach so we can implement our use case?). If there are not bugs, it would be helpful for the lib to make the array stable for us so we don't have to employ workarounds on our own.

[TkDodo]

in v5, you can use combine:

• https://tanstack.com/query/latest/docs/framework/react/reference/useQueries#combine

the result is structurally shared, so it will be stable across re-renders.

[joshribakoff-sm]

Still seems like it would be nice for the original array to just be stable in the first place (or understand why it causes an issue for it to not be stable) as that feels like a "workaround" is still required, the default behavior of adopting the useQueries hook for some users will continue to unfortunately be their app crashes with 100%, or acts erratic, and is difficult to debug (until they hopefully eventually discover the issue and that "combine" is adopted).

[TkDodo]

their app crashes with 100%

those examples usually involve useEffect + setState and for those cases, combine is really what you want. It avoids additional render cycles, too. You almost never want to call setState inside useEffect.