TanStack / query

🤖 Powerful asynchronous state management, server-state utilities and data fetching for the web. TS/JS, React Query, Solid Query, Svelte Query and Vue Query.
https://tanstack.com/query
MIT License
42.45k stars 2.9k forks source link

useQueries causes constant rerenders #2991

Closed alecf closed 2 years ago

alecf commented 2 years ago

Describe the bug When using useQueries, the owning component rerenders repeatedly.

To Reproduce Steps to reproduce the behavior:

  1. call useQueries()
  2. put a console log in the render method, or add a useeffect:
    const result = useQueries(...);
    useEffect(() => {
    console.log('result changed!');
    }, [result]);

Expected behavior The component should not be rerendered constantly

Desktop (please complete the following information):

Additional context

This is due to a missing useMemo(): https://github.com/tannerlinsley/react-query/blob/master/src/react/useQueries.ts#L121

The problem is that later in that function, a useEffect fires, which updates state, which causes everything to rerender, which causes this array to be generated again.

A simple useMemo() fixes the problem. Working on a PR now

TkDodo commented 2 years ago

While a useMemo in this spot might make some situations better, it won't do much for many cases, because it depends on the queries that is passed into useQueries, which is rarely stable. So if you do:

useQueries([{ queryKey1, queryFn1 }, { queryKey2, queryFn2 }])

the memoization won't work because we pass a new array every time. Also, useQueries is often used with dynamic amounts of queries where a .map is involved, where it also wouldn't work, like:

useQueries(something.map(...))

Maybe you can tell me more about the effect that you want to run ?

alecf commented 2 years ago

@TkDodo yes, that is correct - but if you do use useMemo() for the input then it works beautifully.

Without this, I find that useQueries is a useless API because it just makes my tab peg the CPU. There is actually no way to use useQueries in a practical way because of this...

So to answer your question: I want to use useQueries as it is documented, to make N simultaneous queries based on a previous list of items.

TkDodo commented 2 years ago

Without this, I find that useQueries is a useless API because it just makes my tab peg the CPU. There is actually no way to use useQueries in a practical way because of this...

ookay. At least you'll save on heating then I guess 🤷

I want to use useQueries as it is documented, to make N simultaneous queries based on a previous list of items.

I don't how see how this correlates with useMemo. I'm not against merging the PR, I just want to understand what you are trying to achieve because I don't think that useMemo will do a lot in many situations. For example:

const queries = useQueries([query1, query2])

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

will work pretty well even without useMemo. If Component is wrapped in React.memo, it will also not re-render needlessly, because each query.data is properly memoized and structurally shared.

alecf commented 2 years ago
const queries = useQueries([query1, query2])

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

Right, it definitely doesn't help in this case. But at least that responsibility is on the consumer of the API to deal with. without this patch, there is no way for the consumer to actually work around this issue.

But even in the above case, I guess I'm surprised that this isn't doing constant rerenders for you then? If anything, I think the docs also need to say "the queries passed in should be memoized to avoid constant rerenders"

My use case is something like this:


    const rootObj = useQuery(...get one object);
    const objQueries = useMemo(() => rootObj.subObjs.map(subObj => makeQuery(subObj.id) ?? []), [rootObj]);
    const subObjs = useQueries(objQueries);

And that is how I benefit from this patch.

will work pretty well even without useMemo. If Component is wrapped in React.memo, it will also not re-render needlessly, because each query.data is properly memoized and structurally shared.

But it will rerender needlessly, that's the point! Look in react-devtools, do a profile on ANY consumer of useQueries There is a useEffect in the hook that runs and triggers a rerender, that dirties the tree because the result of the queries:

This is the relevant code in from the hook:

function useQueries(queries) {

  // (alecf): guaranteed new version of defaultedQueries on any render
  const defaultedQueries = queries.map(options => {
     ..
  });

 // (other code here)

  // (alecf): This useEffect is always fired because defaultedQueries changes each time.
  // It dirties the observer, which in turn causes a rerender, which triggers the map() above, 
  // which then triggers this useEffect, etc.
  React.useEffect(() => {
    // Do not notify on updates because of changes in the options because
    // these changes should already be reflected in the optimistic result.
    observer.setQueries(defaultedQueries, { listeners: false })
  }, [defaultedQueries, observer])
}
TkDodo commented 2 years ago

But even in the above case, I guess I'm surprised that this isn't doing constant rerenders for you then? If anything, I think the docs also need to say "the queries passed in should be memoized to avoid constant rerenders"

not sure what you mean with "constant re-renders". It's not like an infinite loop of re-renders?

The fact that you get a new array from useQueries is only relevant if:

Again, I think that we should do it, and I will as soon as the PR doesn't have syntax or prettier errors (which it now seems to not have, so we can go ahead). But stating that the hook is useless because it returns a new Array on every render is plain false, sorry.

This is the relevant code in from the hook:

I'm aware, I have seen the PR ;)

alecf commented 2 years ago

sorry for calling it "useless" :)

But yes, I'm seeing "constant re-renders" - not an infinite loop, but yes a re-render about every 200ms when the rest of my app is fully idle

TkDodo commented 2 years ago

But yes, I'm seeing "constant re-renders" - not an infinite loop, but yes a re-render about every 200ms when the rest of my app is fully idle

That sounds weird, like a different kind of bug weird. I mean if you have a big array and the promises resolve over time, there will be one re-render for each resolved promise. Also, unless you use further optimizations like tracked queries, there will be re-renders when you focus the window etc.

I'm going to merge the useMemo fix, because as you said, at least it gives the control back to the users to memoize their input if they want, but I would also appreciate it if you could spawn up a codesandbox example that shows these constant re-renders please.

tannerlinsley commented 2 years ago

:tada: This issue has been resolved in version 3.33.5 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

berli888 commented 2 years ago

It happens when you use setState inside a useEffect that depends on the array arr1 returned by useQueries. Migrating to v3.39.0 doesn't seem to fix the problem.

On my case, I tried to use setState to massage the array arr1

A workaround is to directly massage the array arr1 inside the queryFn of useQueries

DSK9012 commented 1 year ago

In Addition to @berli888, Infinite re-renders happen when trying to update local state with other than reference type of queries response in useEffect where we added queries result in dependencies array.

And also when any query response is not consistent(Meaning for first call of "posts" search gives [posts] and when no posts, return response as {message: "No data found."}), You will infinite re-renders.

Here are some examples to get better idea:

function App() {
  const [fetchedReferenceTypeData, setFetchedReferenceTypeData] = useState([]);
  const [localReferenceTypeState, setLocalReferenceTypeState] = useState([]);
  const [primitiveState, setPrimitiveState] = useState(1);
  const queries=useQueries([
    {
      queryKey: 'posts',
      queryFn: ()=>fetchPosts()
    },
    {
      queryKey: 'bookmarks',
      queryFn: ()=>fetchBookmarks()
    },
  ])

  // ✅ - No Issues
  useEffect(() => {
    setPrimitiveState(2);
  }, []);

  // ✅
  useEffect(() => {
    setPrimitiveState(3);
  }, [queries]);

  // ✅
  useEffect(() => {
    setLocalReferenceTypeState(["1"]);
  }, [primitiveState]);

  // 🔴 - Cause Infinite re-renders since we get new array from useQueries everytime in this case.
  useEffect(()=>{
    // Updating the local reference type with data other than the response from queries
    setLocalReferenceTypeState(["1"]);
  }, [queries]);

  // 🔴 
  useEffect(()=>{
    // Updating the local reference type with data other than the response from queries
    setFetchedReferenceTypeData(["1"]);
  }, [queries]);

  // ✅
  useEffect(()=>{
    // Assume here posts response type id [] of posts
    setFetchedReferenceTypeData(queries.data.data);
  }, [queries]);

  // 🔴
  useEffect(()=>{
    // Can still cause infinite re-renders when there is in-consistent response from any of the queries
    // Assume here posts response type is {} when there are no posts, In this we can directly add some cond in fetchPosts func itself to return consistent data type.
    setFetchedReferenceTypeData(queries.data.data);
  }, [queries]);

  return <h1>This is the weird hook from react-query.</h1>;
}

So, I don't this issue is fixed @TkDodo and @alecf. Please correct me if I'm wrong. Let me open a new issue.

version: v3