Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: no hydration when new promise comes in #8383
fix: no hydration when new promise comes in #8383
Changes from 23 commits
050c9d5
524f5ff
cf37452
d82cfb2
6eaf6fc
8c7ccf2
4efa642
f32f6e3
77362c4
5fa1a33
7eec72d
edda7ba
85ee5bc
4d9645b
a6d28f5
fb114b9
c5eccf5
6dbdf34
3c1b221
c691765
b7d9755
9d3b116
9ec623a
9ebf869
7584928
cef39db
a346f39
8448f7b
71a4453
9f555a2
4239947
9ef516c
a1d1c91
4c1dc96
8c2023e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
this might actually be working
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.
What if we prefetch a query without
await
in a page that is static or cached, either fully or partially, and then client navigate to it with newer data in the cache? My guess is that there are/can be situations where we get a promise that will resolve to data that is actually older than the one we have on the client already?Maybe the entirely correct way to do this would be to inspect the data the query resolves to before determining whether to update the cache with that data? Implementation-wise that's a lot tricker though unfortunately. :(
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.
this is something only users could verify though. Without query meta-data like
dataUpdatedAt
, react-query doesn’t know the age of the data. Are you suggesting we provide a way for users to extract a timestamp (age) from the promise data in some sort of callback option?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.
Is it? If we are using the fetch timestamp from the server to determine this when hydrating data, we can surely do that for promises too, just after they resolved? I'm sure it might be a big painful thing to implement, but is there some inherent thing about user/library land that blocks us from doing this in the library using the same logic?
To be clear, if we already have this query in the cache, this might require us to wait for the promise outside of the cache and only put the data in. Or even worse, to support fetching states properly, it might requires us to have some sort of "possibleUpdatePromise" or something that would not commit it's result to the cache if it's older.
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.
@juliusmarminge the tests failures are happening because the tests hang indefinitely. Even locally,
vitest
never terminates. If I remove this line of code, everything is back to normal :/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.
How do you run the tests locally? When I run
vitest
from thequery-core
directory all tests pass?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’s the
react-query
tests that are failing. So go topackages/react-query
and dopnpm run test:lib
there.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.
Oh - I wonder if that's cause we're not testing with the same type of promises that RSCs are sending down 🥹🥹
Can look later tonight when I get home
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.
Fixed: 9ef516c