Skip to content

Infinite renders when using output of useQueries in useEffect dependency array #3049

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

Closed
connorch opened this issue Dec 4, 2021 · 16 comments
Closed

Comments

@connorch
Copy link

connorch commented Dec 4, 2021

Describe the bug
When using the output of useQueries in the dependency array of a useEffect, the containing component will rerender infinitely. This bug seems to have been introduced in v3.9.0 and exists in all later versions.
Please see the Codesandbox I created below and change the version of react-query from 3.8.3 to any later version.

To Reproduce
Steps to reproduce the behavior:

  1. Go to https://codesandbox.io/s/wild-dust-p7swc?file=/src/QueryExample.js
  2. Change the react-query version from v3.8.3 to v3.9.0 or greater to see the issue. (Warning, might freeze your browser due to infinite rerenders)

Expected behavior
Expect the results of v3.8.3 and lower, where the useEffect is only called once when needed.

Desktop (please complete the following information):

  • OS: MacOS
  • Browser: Chrome - Version 96.0.4664.55 (Official Build) (arm64)
  • Version: >v3.9.0
@TkDodo
Copy link
Collaborator

TkDodo commented Dec 4, 2021

It was a tradeoff we had to do in 3.9.0. from #1775

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.

What’s your use-case, because what you are in the example can be done without useEffect…

I think you can also memoize the input as per #2991, but results will still come in gradually from your api so the effect will get called multiple times..

@connorch
Copy link
Author

connorch commented Dec 4, 2021

Oh I see, thanks for the clarification.
This example is closer to my real use-case. The sandbox's react-query version is set to v3.8.3 to prevent nuking your browser window haha

Basically I would like to create a separate hook that takes a dynamic array as an argument, which is used to generate multiple urls and fetch data from those urls. Then the hook performs an expensive operation to reformat the data for consumption in a component.

I can't really put the expensive logic into the hook body because then it will get called every time the component rerenders. Being able to put that expensive logic into a useEffect would prevent this issue.

Do you have any ideas for how to get around this?

@TkDodo
Copy link
Collaborator

TkDodo commented Dec 4, 2021

I think what we would need to do is wrap the result creation in useMemo as well, because right now, you will get

I think it would just mean changing this line:

https://github.com/tannerlinsley/react-query/blob/693ae251104dd18cf2bb983760ecc5a27a8f8616/src/reactjs/useQueries.ts#L136

to:

  const result = React.useMemo(
    () => observer.getOptimisticResult(defaultedQueries),
    [observer, defaultedQueries]
  )

as observer is stable, and defaultedQueries depend on the query input, which you could also then memoize yourself with useMemo, like:

  const responses = useQueries(
    useMemo(
      () =>
        words.map((word) => ({
          queryKey: word,
          queryFn: () =>
            fetcher(`https://api.dictionaryapi.dev/api/v2/entries/en/${word}`)
        })),
      [words]
    )
  );

Do you want to PR that, ideally with a test that checks for referential stability when a re-render occurs that doesn't change any input?

until then, you might need to use-deep-compare-effect to get around the issue.

that being said, I still wouldn't do it in an effect + local state, but with a useMemo, which is made to memoize expensive computations:

  const definitions = useMemo(() => {
    const defs = [];
    responses.forEach((res) => {
      if (!res?.data) return;
      defs.push(res?.data[0]?.meanings[0]?.definitions[0].definition);
      // imagine other expensive data operations
    });
   return defs
  }, [responses]);

@connorch
Copy link
Author

connorch commented Dec 4, 2021

I tried what you said by memoizing result in useQueries and memoizing the argument queries passed into the useQueries() call, and unfortunately this fix doesn't fully work.

It actually gives responses too much referential stability... responses doesn't get updated at all after the first return and gets stuck in a response.isLoading = true and response.data = undefined state.

Are there any other variables in useQueries that we can use to tell the useMemo to update as the response data populates?

use-deep-compare-effect should help me get around this issue for now though. Thanks for suggesting that!

@TkDodo
Copy link
Collaborator

TkDodo commented Dec 4, 2021

yeah, sorry for the wrong direction. calculating the result depends on other factors as well, not just the options. If the fetch returns, we also need the new result. So this doesn't work.

use-deep-compare-effect should help me get around this issue for now though. Thanks for suggesting that!

👍

@connorch
Copy link
Author

connorch commented Dec 4, 2021

Thanks @TkDodo for working with me on this haha.

Is there really no way that we could add referential stability to the results, except when the response data is actually updated? It seems like my use-case would be a fairly common one.

@TkDodo
Copy link
Collaborator

TkDodo commented Dec 4, 2021

Is there really no way that we could add referential stability to the results, except when the response data is actually updated? It seems like my use-case would be a fairly common one.

the result is computed optimistically on every render so that we can give information ahead of time, so that we can e.g. go to loading state before the fetch has happened (in an effect). If we don't do that, it would lead to more re-renders with in-between states, so I don't think there is a way for now.

FYI, in v4, we will change the useQueries api to allow for top-level options:

useQueries({
  queries: [query1, query2, query3]
})

this will theoretically give us the option to allow for a top-level select or combine function that can run once all queries have fetched to do transformations. But its just an idea at the moment, nothing concrete yet.

@TkDodo
Copy link
Collaborator

TkDodo commented Jan 23, 2022

closing because I don't think we can improve this for now, and we have a workaround.

@TkDodo TkDodo closed this as completed Jan 23, 2022
@carstenblt
Copy link

What's the current status of this? I am running into the same issue with useEffect -> set state. Some times it works, some times it does not. I guess switching to useMemo will not make a difference? Actually I'd be fine with a few reruns, until all queries are fetched. Will there be a workaround in v4?

@TkDodo
Copy link
Collaborator

TkDodo commented Apr 27, 2022

@carstenblt why do you need useEffect + setState? It's often an anti-pattern.

@carstenblt
Copy link

@TkDodo yes, so I learned. Hence my question if useMemo fixes this. I need to run code with all the combined queries.

@TkDodo
Copy link
Collaborator

TkDodo commented Apr 30, 2022

at least useMemo wouldn't do infinite re-renders, but if you set it up wrongly, it's as if you hadn't used useMemo at all. This would be my first attempt: Just recompute an on every render, and optimize with useMemo when necessary.

@chpio
Copy link

chpio commented Sep 8, 2022

it will just ignore the useMemo, the same way it is ignoring (or re-executing) the useEffect:

const queries = useMemo(
  () => inputs.map(makeQueryFromInput),
  [inputs]
);

const queryResults = useQueries(queries);

const processedData = useMemo(
  () => queryResults.map(processData),
  [queryResults],
);

The useMemo, that processes queries, is run only once. But the processedData useMemo run on each re-render (even if it's triggered by something else in the component).

So my workaround is now:

const processedData = useMemo(
  () => queryResults.map(processData),
  [...queryResults.map((qr) => qr.data)],
);

@sithu951
Copy link

sithu951 commented Sep 21, 2022

const processedData = useMemo(
  () => queryResults.map(processData),
  [...queryResults.map((qr) => qr.data)],
);

I'd rather write like this

const processedData = useMemo(
  () => queryResults.map(processData),
  [queryResults.reduce((acc, cur) => acc + cur.dataUpdatedAt, 0)],
);

dataUpdatedAt is the timestamp for when the query most recently returned the status as "success". It will be updated on refetch. The use of reduce is with the assumption that the sum of all timestamps of query results would be unique.

So, whenever a query is refetched or a new array of queries is fetched, the 'processData' will be re-processed again. Since the return value of 'reduce' is a 'scalar' type - in this case, it is a number type -, as long as the resulting number is the same, useMemo won't be re-rendered.

@TkDodo
Copy link
Collaborator

TkDodo commented Sep 24, 2022

dataUpdatedAt changes on every refetch, but data might not. If the same data is returned from the backend, react-query will re-use the previous data. That concept is called structural sharing:

https://tkdodo.eu/blog/react-query-render-optimizations#structural-sharing

@J3tto
Copy link

J3tto commented Nov 25, 2022

Any suggestions of how to handle this suituation when the number of queries changes between renders?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants