too many re-renders when changing keys

[TkDodo]

Describe the bug

When changing the query key, react-query re-renders the component two times with the exact same states

Your minimal, reproducible example

v4: https://codesandbox.io/s/react-query-re-renders-t2z4u8, v3: https://codesandbox.io/s/react-query-re-renders-v3-cfo8y4

Steps to reproduce

• Open the sandbox and inspect the console
• on page load, you'll see two renders (loading and success): 🆗
• click the next button
• you'll see 3 additional renders
  • loading: 🆗
  • loading: 🚫
  • success: 🆗

The second "loading" render has the exact same state as the previous one and is unnecessary.

This happens on v17 and v18 of react, and in v3 and v4 of react-query.
It also has nothing to do with extra renders in dev-mode and strict effects (the example doesn't use strict mode), and it can also be reproduced when the logging is moved to an effect. The component really renders twice.

Expected behavior

we should see two renders instead of three:

• loading: 🆗
• success: 🆗

How often does this bug happen?

Every time

Screenshots or Videos

Platform

independent (I'm on macos & brave)

react-query version

v3 and v4

TypeScript version

No response

Additional context

first reported on stackoverflow: https://stackoverflow.com/questions/72834988/react-query-makes-component-to-rerender-multiple-times

my best guess is that setState triggers one render, and then useQuery re-renders again. But those should be deduped....

here's a failing test-case to reproduce:

  it('should not render too often when query key changes', async () => {
    const key = queryKey()
    const states: UseQueryResult<{ count: number }>[] = []

    function Page() {
      const [count, setCount] = React.useState(0)
      const state = useQuery([key, count], async () => {
        await sleep(10)
        return { count }
      })
      states.push(state)

      return (
        <div>
          <h1>data: {state.data?.count}</h1>
          <button
            onClick={() => {
              setCount(prev => prev + 1);
            }}
          >
            Next
          </button>
        </div>
      );
    }

    const rendered = renderWithClient(queryClient, <Page />)

    await waitFor(() => rendered.getByText('data: 0'))

    // Initial
    expect(states[0]).toMatchObject({ status: 'loading', data: undefined })
    // loaded
    expect(states[1]).toMatchObject({ status: 'success', data: { count: 0 } })

    expect(states.length).toBe(2)

    fireEvent.click(rendered.getByRole('button', { name: /next/i }))

    await waitFor(() => rendered.getByText('data: 1'))

    // fetching with new key
    expect(states[2]).toMatchObject({ status: 'loading', data: undefined })
    // loaded
    expect(states[3]).toMatchObject({ status: 'success', data: { count: 0 } })

    expect(states.length).toBe(4)
  })

[TkDodo]

Note: this used to work in 3.5.11, but no longer in 3.5.12

see:

• fix: prevent missing renders #1608

[karol-janik]

Hello, is it still available? I would like to try to work on it. Thanks!

[TkDodo]

@karol-janik sure, go ahead. Thank you 🙏

[abacaj]

This appears to be the order of operations when the query key changes:

1. click button -> updates key state
2. build new query (status: 'loading', fetchStatus: 'fetching', optimistic result changes the fetchStatus)
3. dispatch fetch
4. notify listeners -> this seems to be the culprit

states.length counts after each operation:

1. 3
2. 3
3. 3
4. 4 -> this is the extra update where (status: 'loading' and fetchStatus: 'fetching')

The issue is there is a state update that changes key which rerenders
and then also a observer that fires the same state and also rerenders -> which puts at 5 states when the request is successful.

**edit
Can confirm that disabling dispatch call makes your test case pass:

file:query.ts

    // Set to fetching state if not already in it
    if (
      this.state.fetchStatus === 'idle' ||
      this.state.fetchMeta !== context.fetchOptions?.meta
    ) {
      // this.dispatch({ type: 'fetch', meta: context.fetchOptions?.meta })
    }

Seems solution might be to disable the redundant notify on key changes?

[karol-janik]

I noticed that this setState causes the loading status to duplicate itself. By replacing this hook with useMemo it seems that everything works as it should. Unfortunately for this, other test casses are crashing (Probably by defaultedOptions in the dependency array of useMemo).

[abacaj]

@karol-janik I'm not sure we can prevent/memo the change.

Might be easier to not fire the "fetch" event from react-query observer when the key changes since this is an extra event that causes the extra "loading" state to appear.

[TkDodo]

@karol-janik this setState cannot cause any re-renders because the setter of that useState is not used:

query/packages/react-query/src/useBaseQuery.ts

Lines 71 to 77 in 69ba565

	const [observer] = React.useState(
	() =>
	new Observer<TQueryFnData, TError, TData, TQueryData, TQueryKey>(
	queryClient,
	defaultedOptions,
	),
	)

this is the correct way to initialize a value once, when the component mounts. useMemo is not guaranteed to only run once - react can, at any time, throw the computed result away and re-run it.

[farzd]

is this still an issue ? i'm on "@tanstack/react-query": "^4.0.9" and seeing this behaviour on react native

    "react": "17.0.2",
    "react-dom": "17.0.2",
    "react-native": "0.68.2"

[laurentDellaNegra]

It seems to be the same issue in the practice of React Query lesson:
https://codesandbox.io/s/solution-render-optimizations-v1rgki?from-embed

There is 3 re-renders instead of 2

[TkDodo]

There is 3 re-renders instead of 2

remove React.StrictMode from the example and the render count goes down to 2. This is not related.

[suheth]

Is this issue fixed?
I'm facing this issue currently. The loading and success state causing re-render twice

[TkDodo]

@suheth this issue is open for a reason, namely, because it hasn't been fixed.

the issue I linked that re-introduced this extra re-render mentions that:

Reverts the render optimization from #1487 because it is not guaranteed React will actually commit the render when a hook is called (renders might be discarded). Added a test to cover this behavior.

so I'm not sure if we can just easily optimize this

[TkDodo]

@abacaj do all tests pass with your change? Please read the comment above ⬆️ . I think it is not guaranteed that calling setState will actually trigger a re-render, so we need to inform observers. But I think these should actually be batched together. It might just be an issue with useSyncExternalStore:

• Bug: is the current useSyncExternalStore batching & flushing behaviour intended? facebook/react#25191
• Bug: useSyncExternalStore update not batched within unstable_batchedUpdates facebook/react#24831

[TkDodo]

seems like this indeed an upstream issue that rtk-query has as well:

• Dispatching at the same time as setState causes two renderings reduxjs/react-redux#1912

[SaadUrRehmanBaig]

I think I have solved the problem. It was with the dependency array in the useEffect hook. Try giving an empty dependency array.

solution by having an empty dependency array

problem by not having an empty dependency array

[kmarkiewicz]

Is there any workaround to avoid the unnecessary render?

[Ram4GB]

Sorry @SaadUrRehmanBaig but It just happens on the next query. In the author's example, The issue will happens when we click the next button. Please check this again.

On your case, we have:

1. useQuery fetching with loading = true
2. render
3. useEffect with no depedencies
4. useQuery finished with loading = false
5. re-render
6. useEffect with no depedencies

I confirmed I faced this issue on v4.23.0.

[TkDodo]

it should be fixed when the bugs in react are fixed 🤷

[incepter]

I think the same issue is addressed in #5538; and this PR may solve it.

Here is the same sandbox with the built version from the PR that shows the resolve actually: https://codesandbox.io/s/react-query-re-renders-forked-66s9td?file=/src/App.tsx

[TkDodo]

amazing @incepter 🙌

[daniel-ricado]

looks like thePR has been reverted? Using refetchInterval seems to cause full page re renders

[TkDodo]

@daniel-ricado the PR has not been reverted. If you see behaviour you don't expect, please file a separate issue with a new reproduction

[daniel-ricado]

hmmm bumping up the version didn't resolve it, but I'm thinking it's refreshing all children of the component it's used in. yup will setup a reproduction example and new issue if thats the case cheers @TkDodo .