-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Fix misc hydration bugs #9157
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 misc hydration bugs #9157
Conversation
* Do not hydrate older promises * Do not infinite loop on hydrating failed promises * Hydrate already resolved promises immediately
View your CI Pipeline Execution ↗ for commit fd7a6e0.
☁️ Nx Cloud last updated this comment at |
* Include dehydratedAt and ignore hydrating old promises * This is an approximation of dataUpdatedAt, but is not perfect * Refactor checks for if we should hydrate promises or not to fix infinite loop bug on failed queries * Hydrate already resolved promises synchronously
27d8d67
to
13c3232
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9157 +/- ##
===========================================
+ Coverage 45.00% 59.46% +14.46%
===========================================
Files 206 138 -68
Lines 8219 5482 -2737
Branches 1836 1471 -365
===========================================
- Hits 3699 3260 -439
+ Misses 4080 1925 -2155
+ Partials 440 297 -143 🚀 New features to boost your workflow:
|
packages/query-core/src/thenable.ts
Outdated
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | ||
?.catch(() => {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using noop
from query-core
should get rid of the eslint-disable:
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | |
?.catch(() => {}) | |
?.catch(noop) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't get rid of the lint warning unfortunately, that's warning about the "unnecessary" ?.
here (this is typed as a promise, but might be a kind of thenable without .catch
).
Still a good idea to use the noop
, and I updated the comment to mention .catch
specifically to make it a tiny bit clearer.
Is this going to be a part of next release? |
It's already released |
This PR:
At the point of hydrating a promise, we don't know when the promise will finish, that is, we don't know the
dataUpdatedAt
of the hydrated query, so we can't compare that to the existing query. The PR sidesteps this by introducingdehydratedAt
as an approximation. This is not perfect, but is an improvement.To fix the infinite loop bug I also opted to never hydrate promises if the existing query is already pending, as there is no way to know which of these pending queries will finish first or have the newest data. We could do fancier things like a
Promise.race
and use whatever finishes first, but that seemed like overengineering at this point.