Why useEffect triggers twice after the API response is cached?

[TkDodo]

Discussed in #5537

^{Originally posted by dev-rish June 5, 2023}
Describe the Bug
After switching between 1 & 2 for couple of times (so that react query caches data for each of them), the useEffect still triggers twice.
The expected behavior is seen till v3.8.3 i.e. useEffect only triggers once. Once react query is updated to v3.9.0 every click on 1 or 2 causes useEffect to trigger twice.

Is there a way to prevent the second trigger?

I found related issues but still wanted some more info. if possible.

Minimal, reproducible example
v3: https://codesandbox.io/s/optimistic-firefly-1tk7i5
v4: https://codesandbox.io/s/sad-scott-cidk6s?file=/src/App.js

Steps to reproduce
Click in 1 > Click on 2 > Click in 1 > Click on 2

Expected behavior
useEffect to trigger only once when id is switched.

How often does this bug happen?
Always

Note:

• Seems to be select related, because it works as expected if select is removed
• This is not strict mode related

[incepter]

Isn't the behavior normal since it is about new data coming from API ?

I mean, I inspected the stack traces and found that the selector function is ran twice:

• When useQuery renders and finds data in cache about the id
• When data is refreshed from API, and then the observer needs to re-select data, and obviously the new object doesn't equal the previous one.

[TkDodo]

When data is refreshed from API, and then the observer needs to re-select data, and obviously the new object doesn't equal the previous one.

while this is true, react-query does structural sharing, and it does that also on the result of select. So if it's deeply the same, the same object reference should be reused.

[omermizr]

I'm not too familiar with the internals, but I think the issue is with https://github.com/TanStack/query/blob/main/packages/query-core/src/query.ts#L470, where you call setData with the raw result, rather than the selected result.
That's also incompatible with the structuralSharing function option, because it takes TData, but we pass it TQueryFnData.

I added a structuralSharing function and some logs and it looks like the data coming in is indeed the raw data.
Removing the select callback and moving it into structuralSharing fixes it.
Sandbox for the logs and workaround: https://codesandbox.io/s/ancient-fire-p9pmhc?file=/src/App.js:235-263

[incepter]

I think I know what's happening in there.
I have a fix locally, but im not sure about its impact.

I will followup with a PR and explanations later today.

[incepter]

Following up with my PR.

Issue explanations

useEffect fires twice because data changed.

First, let's observe both stack traces and see from where the selector is called:

// first time:
selector Error
    at Object.selector [as select] (https://h34qgm.csb.app/src/App.js:48:27)
    at QueryObserver.createResult (https://h34qgm.csb.app/node_modules/@tanstack/query-core/build/lib/queryObserver.js:294:26)
    at QueryObserver.getOptimisticResult (https://h34qgm.csb.app/node_modules/@tanstack/query-core/build/lib/queryObserver.js:109:17)
    at Object.useBaseQuery (https://h34qgm.csb.app/node_modules/@tanstack/react-query/build/lib/useBaseQuery.js:61:27)
    at useQuery (https://h34qgm.csb.app/node_modules/@tanstack/react-query/build/lib/useQuery.js:11:23)
    at App (https://h34qgm.csb.app/src/App.js:18:32)

// second time:
index.js:27 selector Error
    at Object.selector [as select] (https://h34qgm.csb.app/src/App.js:48:27)
    at QueryObserver.createResult (https://h34qgm.csb.app/node_modules/@tanstack/query-core/build/lib/queryObserver.js:294:26)
    at QueryObserver.updateResult (https://h34qgm.csb.app/node_modules/@tanstack/query-core/build/lib/queryObserver.js:383:29)
    at QueryObserver.onQueryUpdate (https://h34qgm.csb.app/node_modules/@tanstack/query-core/build/lib/queryObserver.js:459:10)
    at eval (https://h34qgm.csb.app/node_modules/@tanstack/query-core/build/lib/query.js:445:18)
    at Array.forEach (<anonymous>)
    at eval (https://h34qgm.csb.app/node_modules/@tanstack/query-core/build/lib/query.js:444:22)
    at Object.batch (https://h34qgm.csb.app/node_modules/@tanstack/query-core/build/lib/notifyManager.js:24:16)
    at Query.dispatch (https://h34qgm.csb.app/node_modules/@tanstack/query-core/build/lib/query.js:443:33)
    at Query.fetch (https://h34qgm.csb.app/node_modules/@tanstack/query-core/build/lib/query.js:275:12)
    at QueryObserver.executeFetch (https://h34qgm.csb.app/node_modules/@tanstack/query-core/build/lib/queryObserver.js:172:37)
    at QueryObserver.setOptions (https://h34qgm.csb.app/node_modules/@tanstack/query-core/build/lib/queryObserver.js:90:12)
    at eval (https://h34qgm.csb.app/node_modules/@tanstack/react-query/build/lib/useBaseQuery.js:66:14)
    at commitHookEffectListMount (https://h34qgm.csb.app/node_modules/react-dom/cjs/react-dom.development.js:23150:26)
    

The second call is started from the useEffect within useBaseQuery because the query function is inlined, so it encounters a new reference for each queryKey change.

1. So first, when requesting the id: 1, it fetches and renders first with undefined, then Value1.
2. Then, when id is 2, data becomes again undefined, then Value2
3. Now, when we go back to id: 1, useQuery would call observer.getOptimisticResult, which will retrive data for id: 1 from cache. Then renders with Value1 (that's the issue)
4. Then useEffect requests data for id 2, because queryFn is inlined and queryKey changed
5. When data for id 1 arrives, createResult is called again, against a observer.currentResult which is related to id: 2.

So, the culprit is selectData being out of sync from currentResult in the QueryObserver. Let's go back to the previous steps again and only focus on selectData and currentResult:

1. selectData: undefined and currentResult: undefined, then: selectData: Value1 and currentResult: of(id = 1)
2. selectData: undefined and currentResult: of (id = 1), then: selectData: Value2 and currentResult: of(id = 2)
3. selectData: Value1 and currentResult: of (id = 2)
4. selectData: Value1 and currentResult: of (id = 2)
5. selectData: Value1 and currentResult: of (id = 1)

The deep object cloning occurs between observer.currentResult and the new selected data, which turns out to give a new object reference in this case.

Solution

To solve this, the minimal impactful solution I found is to assign the observer's currentResult when an optimistic read occurs, which guarantees that they are in sync.

For more information, take a look at my PR, that adds a regression test for this specific use case.

#5573

Considerations

I am not a react-query pro, so my knowledge on the API is minimal, which may lead my fix to cause other unexpected regressions:

1. Although mutating the observer's properties should not be common during render, which may lead to unexpected behavior, especially with Offscreen, Suspense and Transitions. But maybe this is needed to fix this "unsync" that's caused by mutating during render.
2. The test can be changed to use the states pattern used in the projet (same for fake timers)
3. I do not have enough knowledge to verify that other adapters are safe..

Hopefully this helps.

[mk-nickyang]

Looks like this is still a issue in the latest v4 when keepPreviousData: true, see https://codesandbox.io/p/sandbox/eager-wescoff-x7k9rd

It is fixed in v5 tho even with placeholderData: keepPreviousData, see https://codesandbox.io/p/sandbox/mystifying-dust-7rnfwf