Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

useQueries hook causing infinite re-renders when using result as a dependency in useEffect #5137

Closed
DSK9012 opened this issue Mar 15, 2023 · 9 comments
Labels
enhancement New feature or request v5
Milestone

Comments

@DSK9012
Copy link

DSK9012 commented Mar 15, 2023

Other Similar Issues: #2991

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>;
}

Your minimal, reproducible example

Please look at code snippet provided

Steps to reproduce

Hope you got better Idea from above snippet.

Expected behavior

useQueries should memoize the response and should not force the consumed component to update same response when adding as dependency in useEffect.

How often does this bug happen?

Every time

Platform

OS: MacOS
Browser:Chrome

TanStack Query version

v3.39.3

@TkDodo
Copy link
Collaborator

TkDodo commented Mar 17, 2023

same as useQuery, where the top level object is also a new object in every render cycle, the useQueries array is "new", but the data property in each one is not.

The code snippets don't make much sense to me, as they don't show any real life use-case. What would you do in a useEffect that runs whenever any data changes? Writing to local state is a bit of an anti-pattern in itself. So can you please show an example, preferably a codesandbox reproduction, where this feature would be useful?

@gabrieldutra
Copy link

@TkDodo for useQuery this is fine since you'll very likely be destructuring the object and depend on the specifics you need, but for useQueries this is a bit different because you need to access each value to be able to do so. One example I can think of, and that I'm facing right now is if you need to do memoization depending on the queries:

function Component() {
  const queries = useQueries();
  const someExpensiveCalculation = useMemo(() => {
    queries.map(/* ... */);
  }, [queries, /* [...] */); // memoization is broken because of `queries`
}

The only way I could fix it for that case for now was to pick the attributes I needed (data, isError, isLoading, etc) and to force a shallow comparison to make the reference stable.

@TkDodo
Copy link
Collaborator

TkDodo commented Mar 25, 2023

I'll think about this problem. I guess what would be good is to have a combine of some sort that could iterate over all queries, something like:

const result = useQueries({
  queries: [query1, query2, ..., queryN],
  combine: (queries) => ...
})

whatever you return from combine would be the result, so per default, it would just be: combine: (queries) => queries to be backwards compatible.

I'm not sure if we could do this and have it work well with TypeScript, so I'm looping in @artysidorenko

@gabrieldutra
Copy link

gabrieldutra commented Mar 25, 2023

Thanks!

The way I solved it for now was by doing this:

function shallowCompareObjects<T extends {}>(a: T, b: T) {
  return Object.keys(a).every((key) => a[key] === b[key]);
}

function shallowCompareCollections<T extends {}>(a: T[], b: T[]) {
  return a.length === b.length && a.every((item, index) => shallowCompareObjects(item, b[index]));
}

function useMemoizedCollecionPick<T extends {}, P extends keyof T>(array: T[], pickValue: readonly P[]): Pick<T, P>[] {
  // eslint-disable-next-line react-hooks/exhaustive-deps
  const stablePickValue = useMemo(() => pickValue, [JSON.stringify(pickValue)]);
  const value = useMemo(() => array.map((item) => pick(item, stablePickValue)), [array, stablePickValue]);
  const prevValue = useRef(value);
  return useMemo(() => {
    if (shallowCompareCollections(value, prevValue.current)) {
      return prevValue.current;
    }
    prevValue.current = value;
    return value;
  }, [value]);
}

const result = useQueries();
const pickedResult = useMemoizedCollecionPick(result, ['data', 'error', 'isLoading']);

which gets me a little bit closer to useQuery destructuring inside whatever hook I need to use.

I'm guessing combine could work too assuming you update it only when the values change. TS for it should be fine with something like this

@artysidorenko
Copy link
Contributor

Heya, yeah I just had a play around locally as well, I think something like the below will work well in getting the types for this*.

export function useQueries<T extends any[], CombinedResult = QueriesResults<T>>({
  queries,
  context,
}: {
  queries: readonly [...QueriesOptions<T>]
  context?: UseQueryOptions['context']
  combine?: (result: QueriesResults<T>) => CombinedResult
}): CombinedResult {

*the result parameter doesn't suffer from those other callback param-typing issues; all the inference happens inside QueriesOptions so you get the correct type in the combine function

@TkDodo TkDodo added enhancement New feature or request and removed needs-info labels Apr 1, 2023
@TkDodo
Copy link
Collaborator

TkDodo commented Apr 1, 2023

I started working on this, but it's not trivial. I wanted to add it on observer level to have this feature available in core, but we determine suspense and error boundary features on framework level. If combine takes away the information we need, we can't do that anymore.

So I think it needs to happen on framework level, after suspense / errorBoundary handling. But there, we'd have a harder time memoizing the previous value to also guarantee referential stability. It might be that I'll add combine first, but it won't have any referential stability improvements, and it would also run on every render.

Oh also, if it comes, it will come in v5 because we have so many changes already that I don't want to deal with the conflicts 😅 .

@TkDodo TkDodo mentioned this issue Apr 2, 2023
3 tasks
@JobVil
Copy link

JobVil commented May 12, 2023

Just commenting here because I am also running into this issue.

@TkDodo TkDodo added the v5 label May 15, 2023
@TkDodo TkDodo added this to the v5 milestone May 15, 2023
@TkDodo
Copy link
Collaborator

TkDodo commented May 15, 2023

shipped in v5 alpha.34

@TkDodo TkDodo closed this as completed May 15, 2023
@tobysmith568
Copy link

For those who can't update yet, my current fix for memoizing from the queries is to increment a counter on success and then memoize from that counter:

// Doesn't work
const queries = useQueries(/* */);
const expensiveValue = useMemo(() => {/* */}, [...queries]);

// Does work
const successfulQueriesCounter = useRef(0);
const queries = useQueries([
  { onSuccess: () => successfulQueriesCounter.current = successfulQueriesCounter.current + 1 }
]);
const expensiveValue = useMemo(() => {/* */}, [successfulQueriesCounter.current]);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request v5
Projects
None yet
Development

No branches or pull requests

6 participants