Skip to content
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

fix: 🐛 Make sure queries which have dataUpdatedAt of 0 will be run #1556

Merged
merged 3 commits into from
Jan 5, 2021

Conversation

meesvandongen
Copy link
Contributor

When initialDataUpdatedAt is a falsy numeric value (e.g. 0), it will cause it to be replaced with Date.now(). This leads to unexpected behaviour, as one would expect the query to be invalidated instantly, if staleTime is set to an amount less than the result of Date.now(). This change will check if the value is numeric, and if so, always use this numeric value.

BREAKING: This changes the behaviour of the plugin for those users that have specified initialDataUpdated at <= 0.

When initialDataUpdatedAt is a falsy numeric value (e.g. 0), it will cause it to be replaced with Date.now(). This leads to unexpected behaviour, as one would expect the query to be invalidated instantly, if staleTime is set to an amount less than the result of Date.now(). This change will check if the value is numeric, and if so, always use this numeric value.

This changes the behaviour of the plugin for those users that have specified initialDataUpdated at <= 0.
@vercel
Copy link

vercel bot commented Jan 2, 2021

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/tannerlinsley/react-query/el66wgeg4
✅ Preview: https://react-query-git-fork-meesvandongen-fix-0-hasinitialdata.tannerlinsley.vercel.app

@meesvandongen
Copy link
Contributor Author

meesvandongen commented Jan 2, 2021 via email

@TkDodo
Copy link
Collaborator

TkDodo commented Jan 2, 2021

The problem is that a valid timestamp, 0 (number, 1970) is falsy, and
therefore gets converted to date now, the current timestamp (2020)

Yes, I think I got the problem, and using ?? instead of || would solve it with minimal changes. This is one of the prime reasons why nullish coalesce was introduced - so that you can easily get "fallback values" without trapping into the js falsy-ness of the number 0 / empty string. It will only convert undefined or null to new Date() and nothing else, which I think was the intention of the original code.

@meesvandongen
Copy link
Contributor Author

Ah sorry, I misunderstood nullish coalescing, and therefore your comment. I see now, it makes a lot of sense. I will change the PR accordingly.

@meesvandongen
Copy link
Contributor Author

Alright, I updated the PR, was thinking whether I should have 0 or Date.now() on line 471, but ultimately this value would never be used anyway in the current implementation and is only used to satisfy typescript.

@boschni
Copy link
Collaborator

boschni commented Jan 3, 2021

Nice! Could you add a test to cover this bug?

@meesvandongen
Copy link
Contributor Author

Test added

@boschni boschni merged commit 1299ad6 into TanStack:master Jan 5, 2021
@tannerlinsley
Copy link
Collaborator

🎉 This PR is included in version 3.5.10 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

5 participants