Bail out of query update if data is undefined or same as previous data #2905

phatmann · 2021-11-08T23:47:01Z

vercel · 2021-11-08T23:47:06Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/tanstack/react-query/CBKueudpHjREPb43TRovHj8nsHS2
✅ Preview: https://react-query-git-fork-phatmann-pr-set-query-data-366e44-tanstack.vercel.app

codesandbox-ci · 2021-11-08T23:49:51Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 9e77be1:

Sandbox	Source
tannerlinsley/react-query: basic	Configuration
tannerlinsley/react-query: basic-typescript	Configuration

codesandbox-ci · 2021-11-08T23:51:36Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 7d15c84:

Sandbox	Source
tannerlinsley/react-query: basic	Configuration
tannerlinsley/react-query: basic-typescript	Configuration

TkDodo · 2021-11-09T13:59:54Z

src/core/query.ts


-        // Notify cache callback
-        this.cache.config.onSuccess?.(data, this as Query<any, any, any, any>)
+        if (typeof updatedData !== 'undefined' && !Object.is(prevData, updatedData)) {


I think we would a polyfill for that, please see: https://github.com/facebook/react/blob/cae635054e17a6f107a39d328649137b83f25972/packages/shared/objectIs.js

ignore this if we go the route suggested in a later comment: #2905 (comment)

I will add polyfill.

TkDodo · 2021-11-09T14:11:18Z

src/core/query.ts

+      case 'ignore':
+        return {
+          ...state,
+          error: null,
+          fetchFailureCount: 0,
+          isFetching: false,
+          isInvalidated: false,
+          isPaused: false,
+          status: typeof state.data === 'undefined' ? 'idle' : 'success',
+        }


why do we need this please? Can we just not dispatch anything in case of bail-out and let the query be in that state as if setQueryData was never called?

If a query function returns undefined or the same data, we need to reset state after bailing out. Otherwise the query will stay in fetching state.

TkDodo · 2021-11-09T14:12:30Z

src/core/queryObserver.ts

@@ -68,7 +68,7 @@ export class QueryObserver<
    TQueryKey
  >
  private previousQueryResult?: QueryObserverResult<TData, TError>
-  private previousSelectError: Error | null
+  private previousSelectError: TError | null


this change seems unrelated. Why is it necessary please?

These are type errors in the latest version of Typescript. I can fix in separate PR if you prefer.

yes lets please do this in a different branch, where we also update our TS version to the latest (4.5 as of a couple of minutes ago)

TkDodo · 2021-11-09T14:13:13Z

src/core/tests/queryClient.test.tsx

@@ -137,13 +137,13 @@ describe('queryClient', () => {
    test('should create a new query if query was not found', () => {
      const key = queryKey()
      queryClient.setQueryData(key, 'bar')
-      expect(queryClient.getQueryData(key)).toBe('bar')
+      expect(queryCache.find(key)).not.toBe(undefined)


why do we need to change that test - what was wrong with the previous assertion?

getQueryData returns undefined for existing idle queries or non-existing queries. I wanted to ensure query was actually created.

TkDodo · 2021-11-09T14:18:00Z

src/react/tests/useQuery.test.tsx


    function Page() {
-      const state = useQuery(key, () => 'test', {
+      const state = useQuery(key, () => queryReturnValue++, {


this test is named:

'should re-render when dataUpdatedAt changes but data remains the same'

now, data no longer stays the same. I was wondering if we're not breaking this functionality: Apparently, we would still expect dataUpdatedAt to be updated if a new fetch comes in, which now likely doesn't happen because of the bail out...

sorry for the troubles, but I think the easiest solution would be to remove the Object.is related bailouts and stick with the undefined ones?

Your instinct to combine undefined and same-data bailout was good, since the code changes happen in the same places. Let's stick with this. It is not hard to add code to update dataUpdatedAt. I just need to enhance the 'ignore' action handler to do this.

Hm, I don't really think so. When I wrote this suggestion, I only had setQueryData in mind, because that's where the issue stems from: That you cannot bail out of it even if you get undefined passed in and can't create valid data.

I really think we should keep all changes confined to setQueryData. If we fetch data, we can just put it in the cache without bailing out. structural sharing will make sure that data stays referentially stable, and RQ won't even re-render observers if the data doesn't change. So what's the point in bailing out here? I think it would also remove lots of code.
Additionally, we should in fact call the onSuccess callback of observers that the fetch was successful, even if the data stays the same. I think bailing out here does more harm than good.

The structural sharing takes place in Query.setData, which is used by setQueryData and fetching. So if we are going to give up on the Object.is bailout for fetching, we should do so for setQueryData as well.

Similarly, the call to onSuccess takes place in Query.setData as well. So if we want to return onSuccess for fetching but not setQueryData, it would require some kind of flag, which is smelly.

Since these changes are for React Query 4, I am going to venture my opinion. A large part of the confusion here is that we are using onSuccess in two very different ways. We use it to indicate a successful fetch, and we use it to indicate a data change. Instead, we need two different callbacks, onSuccess when a fetch succeeds and onDataChanged whenever cache is updated with new data via setQueryData or the fetch. I can move this discussion to Discussions if that seems more appropriate.

The structural sharing takes place in Query.setData, which is used by setQueryData and fetching.

this is what I meant when I said we'd need some internal restructurings. calling the functional updater + performing structural sharing / isDataEqual check can be a pure function - it only depends on data and options - both of which we have available. This only leaves us with

// Set data and mark it as cached this.dispatch({ data, type: 'success', dataUpdatedAt: options?.updatedAt, })

that we would need to somehow put into a function on the query, like setSuccessData and call it in two places: from query.setData and from queryClient.setQueryData, where we could only do it after we've computed the new data by calling the aforementioned function, so potentially, not calling query.setData from queryClient.setQueryData, thus keeping the separation between "setting data after fetching" and "setting data from setQueryData"

i like the idea of an extra callback - it was confusing to ppl that onSuccess is also called after setQueryData, but it's a different topic I think

Ok, this all makes sense. I will get to work on it.

If we ever split out onDataChanged and onSuccess, we will need to do the kind of code split you are proposing above anyway. I will propose this change that in a discussion.

Only one issue to resolve: what do we do if the query function returns undefined? If we are not bailing out, I suggest we treat this as an error.

Proposed onDataChanged callback: #2911

Only one issue to resolve: what do we do if the query function returns undefined? If we are not bailing out, I suggest we treat this as an error.

this is also what Tanner proposed yesterday for v4: we throw an error and put the query in error state. In development mode, we could additionally warn, and if possible, we should exclude undefined on type level :)

Sounds good, I will make this change.

TkDodo · 2021-11-09T14:18:48Z

src/core/tests/queryObserver.test.tsx

@@ -143,7 +144,7 @@ describe('queryObserver', () => {
    await sleep(1)
    await observer.refetch()
    unsubscribe()
-    expect(count).toBe(2)
+    expect(count).toBe(3)


why did this change please? not sure why this test needed to change at all :/

If the query function returned a constant value, then only the first call would have any effect because of the bail out change. So now it returns a different value on each call. This results in one extra call to select.

TkDodo · 2021-11-11T16:49:46Z

I've created an issue for this here: #2925 and assigned you to it.

/edit: ok, I can't assign you to it 🙈

please also change the base of your PR to the v4 branch

TkDodo · 2021-11-26T21:03:12Z

@phatmann any updates here please?

phatmann · 2021-11-27T16:22:56Z

Sorry for the activity gap. I'm preparing an app release for my employer and that is sucking up my time. I'll be back on this in about a week.

phatmann · 2021-12-20T14:14:51Z

I should finally have time soon to get back to this. I have not forgotten about it!

TkDodo · 2022-01-03T16:18:14Z

@phatmann hope you're having a great new year. Let me know if I can help you with the PR :)

phatmann · 2022-01-09T23:35:26Z

Still waiting for that magical moment when my new app release has landed with no outstanding issues. Really hoping that is any day now. And then I can get back to this PR.

TkDodo · 2022-02-06T08:51:00Z

@phatmann since we are aiming for a release soon, I would really like to get this over the finish line. If I'm not hearing back from you, I will close the PR and start a new one.

phatmann · 2022-02-07T12:47:43Z

@TkDodo I will work on it today!

phatmann · 2022-02-07T12:51:01Z

@TkDodo after reviewing the comments, I see that we will be better off making a new (narrower) PR and closing this one. So I will do that. Expect a new PR today.

phatmann · 2022-02-07T13:51:13Z

Opened #3271

phatmann added 8 commits November 2, 2021 13:33

Bail out if cache set to undefined or previous data

85d1795

Fix some tests

d509752

Update state when bailing out

9142d09

Bail out on Object.is

0e5f85f

Fix type errors

282f511

Test query.setData

5868036

Tests and fixes

21fa1ec

Call cache onSuccess when data changed via setQueryData

9e77be1

vercel bot deployed to Preview November 8, 2021 23:47 View deployment

Update cache docs

65f8676

vercel bot deployed to Preview November 8, 2021 23:49 View deployment

Handle bail out of idle queries

7d15c84

vercel bot deployed to Preview November 9, 2021 00:53 View deployment

TkDodo requested changes Nov 9, 2021

View reviewed changes

TkDodo added the v4 label Feb 6, 2022

phatmann closed this Feb 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bail out of query update if data is undefined or same as previous data #2905

Bail out of query update if data is undefined or same as previous data #2905

phatmann commented Nov 8, 2021 •

edited by TkDodo

Loading

vercel bot commented Nov 8, 2021 •

edited

Loading

codesandbox-ci bot commented Nov 8, 2021

codesandbox-ci bot commented Nov 8, 2021 •

edited

Loading

TkDodo Nov 9, 2021

TkDodo Nov 9, 2021

phatmann Nov 10, 2021

TkDodo Nov 9, 2021

phatmann Nov 10, 2021

TkDodo Nov 9, 2021

phatmann Nov 10, 2021

TkDodo Nov 17, 2021

TkDodo Nov 9, 2021

phatmann Nov 10, 2021

TkDodo Nov 9, 2021

phatmann Nov 10, 2021

TkDodo Nov 10, 2021

phatmann Nov 10, 2021

TkDodo Nov 10, 2021

phatmann Nov 11, 2021

phatmann Nov 11, 2021

TkDodo Nov 11, 2021

phatmann Nov 11, 2021

TkDodo Nov 9, 2021

phatmann Nov 10, 2021

TkDodo commented Nov 11, 2021 •

edited

Loading

TkDodo commented Nov 26, 2021

phatmann commented Nov 27, 2021

phatmann commented Dec 20, 2021

TkDodo commented Jan 3, 2022

phatmann commented Jan 9, 2022

TkDodo commented Feb 6, 2022

phatmann commented Feb 7, 2022

phatmann commented Feb 7, 2022

phatmann commented Feb 7, 2022 •

edited

Loading

Bail out of query update if data is undefined or same as previous data #2905

Bail out of query update if data is undefined or same as previous data #2905

Conversation

phatmann commented Nov 8, 2021 • edited by TkDodo Loading

vercel bot commented Nov 8, 2021 • edited Loading

codesandbox-ci bot commented Nov 8, 2021

codesandbox-ci bot commented Nov 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TkDodo commented Nov 11, 2021 • edited Loading

TkDodo commented Nov 26, 2021

phatmann commented Nov 27, 2021

phatmann commented Dec 20, 2021

TkDodo commented Jan 3, 2022

phatmann commented Jan 9, 2022

TkDodo commented Feb 6, 2022

phatmann commented Feb 7, 2022

phatmann commented Feb 7, 2022

phatmann commented Feb 7, 2022 • edited Loading

phatmann commented Nov 8, 2021 •

edited by TkDodo

Loading

vercel bot commented Nov 8, 2021 •

edited

Loading

codesandbox-ci bot commented Nov 8, 2021 •

edited

Loading

TkDodo commented Nov 11, 2021 •

edited

Loading

phatmann commented Feb 7, 2022 •

edited

Loading