fix(query-core): correct placeholderData prevData value with select fn

[ardeora]

Howdy!
This PR fixes an issue in v5 when we use placeholderData fn in conjunction with the select fn. The current bug
is that the previousData available in the params of the placeholderData fn is the selected data from the previous query and not the cached data from the previous query. This causes an issue that would be better explained with the following example:

https://codesandbox.io/s/gallant-gates-dslrlm?file=/src/App.js

I have also added one test case that shows the desired (fixed) behavior. Please let me know if I can add any other test cases to make this bulletproof!

Thanks,
Aryan

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
query	⬜️ Ignored (Inspect)			Apr 18, 2023 9:10pm

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 24ed7f9. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

---

✅ Successfully ran 4 targets

• nx affected --target=test:eslint --parallel=5 --base=67a563cb421331c72a7619136eac5758b5befed9
• nx affected --target=test:lib --parallel=5 --base=67a563cb421331c72a7619136eac5758b5befed9
• nx run-many --target=test:build --projects=root
• nx affected --target=test:types --parallel=5 --base=67a563cb421331c72a7619136eac5758b5befed9

---

Sent with 💌 from NxCloud.

[codesandbox-ci]

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 24ed7f9:

Sandbox	Source
@tanstack/query-example-react-basic-typescript	Configuration
@tanstack/query-example-solid-basic-typescript	Configuration
@tanstack/query-example-svelte-basic	Configuration
@tanstack/query-example-vue-basic	Configuration
inspiring-snowflake-nhg50c	PR

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and project coverage change: +3.91 🎉

Comparison is base (4933346) 86.16% compared to head (24ed7f9) 90.07%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##            alpha    #5227      +/-   ##
==========================================
+ Coverage   86.16%   90.07%   +3.91%     
==========================================
  Files          22       75      +53     
  Lines         824     2681    +1857     
  Branches      304      742     +438     
==========================================
+ Hits          710     2415    +1705     
- Misses         95      234     +139     
- Partials       19       32      +13     

Impacted Files	Coverage Δ
packages/query-core/src/mutationCache.ts	100.00% <100.00%> (ø)
packages/query-core/src/queryCache.ts	100.00% <100.00%> (ø)
packages/query-core/src/queryClient.ts	96.85% <100.00%> (ø)
packages/query-core/src/queryObserver.ts	86.59% <100.00%> (ø)

... and 65 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TkDodo]

your fix looks good, but interestingly, in the sandbox, even with the fixed version, we see a render cycle where data is undefined:

https://codesandbox.io/s/admiring-mahavira-w3zgzd

I'm not really sure why that would happen :/

[TkDodo]

hm I think I got something: the prevResultState tracks the previous render basically, so once after we've selected and kept previous data, the prevResultState is now pending with empty data. With the next render. The prevQueryResult that we had, on the other hand, was only changing when a query update came in.

So I think maybe we still need to keep the prevQueryResult value, but make sure it doesn't store "selected" results, but raw results only?

[ardeora]

@TkDodo Thanks for the code review! I think I have a fix in place that makes this example works now 🚀

https://codesandbox.io/s/romantic-moon-lrkjge?file=/package.json

Please let me know if there is anything else that I can improve on this PR.

Thanks so much!
Hope you have a great weekend!

[ardeora]

Also added a test to react-query that uses select and placeholderData function together!

[TkDodo]

I can sadly still see a difference to v4:

if we click twice on next, the data will switch to Loading... on the second click.

I think it's because if I go from id1 to id2, it keeps data of id1. But then I switch to id3, it tries to keep data from id2, which is still undefined, so we get a loading state ...

This wasn't the case in v4. I can click on next as often as I want, and it will never go into an intermediate loading state ...

[ardeora]

Ah makes sense! One question I had was do you think it would be better to create another private property that stores the last succcessful raw data value? Or do you believe there could be an easier way to achieve this?

[TkDodo]

we can see the issue if we log in the placholderData function:

placeholderData: (prev) => {
  console.log(prev);
  return prev;
},

if I click the button once, it will log the prev data correctly. If I click it again while we're still loading (and showing previous data), I will get undefined passed.

---

I think the prevQueryResult is only used for passing to placeholderData. I compared this with main, and this is the same property that we used for keepPreviousData:

query/packages/query-core/src/queryObserver.ts

Lines 475 to 478 in 317fe08

	data = prevQueryResult.data
	dataUpdatedAt = prevQueryResult.dataUpdatedAt
	status = prevQueryResult.status
	isPreviousData = true

so, I don't really understand why this can work on main but not on alpha 🤔

[ardeora]

It doesn't work on main too! If you remove keepPreviousData and just keep select and placeholderData you will see the same issue on v4 too! 😅

[ardeora]

query/packages/query-core/src/queryObserver.ts

Lines 475 to 478 in 317fe08

	data = prevQueryResult.data
	dataUpdatedAt = prevQueryResult.dataUpdatedAt
	status = prevQueryResult.status
	isPreviousData = true

The code snippet you shared above it works because with keepPreviousData the status is set to success!

query/packages/query-core/src/queryObserver.ts

Line 513 in 317fe08

status === 'loading'

and the placeholderData is never run! So we just keep using the previously selected data between renders!

[TkDodo]

If you remove keepPreviousData and just keep select and placeholderData you will see the same issue on v4 too! 😅

What I mean is that on main, it works with keepPreviousData: true:

https://codesandbox.io/s/focused-dawn-pnis3t?file=/src/App.js

Here, you can click the next button as often as you want and you will never see a loading state.

If we do the same thing in v5, which means, replacing:

- keepPreviousData: true
+ placeholderData: (prev) => prev

which should yield the same result, but it doesn't, as when you click twice, it will go to a loading state. Or is that not what you are observing, too?

[ardeora]

If you remove keepPreviousData and just keep select and placeholderData you will see the same issue on v4 too! 😅

What I mean is that on main, it works with keepPreviousData: true:

https://codesandbox.io/s/focused-dawn-pnis3t?file=/src/App.js

Here, you can click the next button as often as you want and you will never see a loading state.

If we do the same thing in v5, which means, replacing:

- keepPreviousData: true
+ placeholderData: (prev) => prev

which should yield the same result, but it doesn't, as when you click twice, it will go to a loading state. Or is that not what you are observing, too?

Yes I see the same result as you! It's just that in v4 with keepPreviousData we never run select/placeholderData functions again and just keep passing the previous result.data to the next render! See #5227 (comment)

With just placeholderData we cant use the previousResult.data because it was the last selected value. So we somehow need to keep track of the raw value somewhere to make this work

[TkDodo]

With just placeholderData we cant use the previousResult.data because it was the last selected value. So we somehow need to keep track of the raw value somewhere to make this work

oh, I see, so this is still select specific? If that's the case, yes, storing the last select result and re-using that should fix it, if we have both placeholderData and select defined.

---

I don't know why, but I also can't get it to work with just placeholderData, without select:

https://codesandbox.io/s/beautiful-pine-70shth?file=/src/App.js

clicking next twice will show the loading state.

and interestingly, this exact example works if I switch back to a previous alpha version:

https://codesandbox.io/s/upbeat-alex-gkmwcl?file=/src/App.js

🤔

[TkDodo]

I have to say this is pretty puzzling. From what I can tell, on current alpha, every scenario works without select.

But I don't think we can just solve it by storing the previous select result, because we can't pass the previous select result to placeholderData. The idea is that it always runs through the placeholderData function, and then through select..

[TkDodo]

The current bug
is that the previousData available in the params of the placeholderData fn is the selected data from the previous query and not the cached data from the previous query.

okay, I see now, we already store the selected result on alpha, and that is actually the source of the bug. What we could do on main is decide based on the flag and just return that previously selected data. But now, we can't do that: we have to gain access to the "last non-undefined raw data" somehow, pass that through placeholderData function, and then (potentially) through select again

[ardeora]

Yep! Makes sense. Thanks for confirming! This might require a lot more refactor. I'll take a look at this again tomorrow morning/afternoon to see if I can make that change 😅 We still might need to hold onto the #selectResult to keep it memoized between renders

[ardeora]

@TkDodo Okay I think this update works now

https://codesandbox.io/s/infallible-moore-srt2x1?file=/src/App.js

If you click multiple times, it should still show the previous data correctly. I added another test that emulates this case in the react-query test suite!

Changes Made

• I removed #previousQueryResult since it is no longer needed
• Instead, I added another property called #lastDefinedQueryData that keeps track of the last defined query data to pass into the placeholderData function!
• Added tests

Please let me know if this doesn't work for a use case you can think of!
Thanks for all the feedback and taking time to review this!

[TkDodo]

I'll test it later today. Thank you for working on this ❤️

[TkDodo]

this is great, thank you 🙏