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.

Sign up for GitHub

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

Jump to bottom

fix: no hydration when new promise comes in #8383

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

fix: no hydration when new promise comes in #8383

wants to merge 13 commits into from

Conversation

juliusmarminge
Copy link
Contributor

No description provided.

@nx-cloud Nx Cloud
Copy link

nx-cloud bot commented Dec 2, 2024 •
edited
Loading

☁️ Nx Cloud Report

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

📂 See all runs for this CI Pipeline Execution

🟥 Failed Commands
nx affected --targets=test:sherif,test:knip,test:eslint,test:lib,test:types,test:build,build --parallel=3
✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@pkg-pr-new pkg.pr.new
Copy link

pkg-pr-new bot commented Dec 2, 2024 •
edited
Loading

Open in Stackblitz

More templates

@tanstack/angular-query-devtools-experimental

npm i https://pkg.pr.new/@tanstack/angular-query-devtools-experimental@8383

@tanstack/eslint-plugin-query

npm i https://pkg.pr.new/@tanstack/eslint-plugin-query@8383

@tanstack/angular-query-experimental

npm i https://pkg.pr.new/@tanstack/angular-query-experimental@8383

@tanstack/query-async-storage-persister

npm i https://pkg.pr.new/@tanstack/query-async-storage-persister@8383

@tanstack/query-broadcast-client-experimental

npm i https://pkg.pr.new/@tanstack/query-broadcast-client-experimental@8383

@tanstack/query-core

npm i https://pkg.pr.new/@tanstack/query-core@8383

@tanstack/query-devtools

npm i https://pkg.pr.new/@tanstack/query-devtools@8383

@tanstack/query-persist-client-core

npm i https://pkg.pr.new/@tanstack/query-persist-client-core@8383

@tanstack/query-sync-storage-persister

npm i https://pkg.pr.new/@tanstack/query-sync-storage-persister@8383

@tanstack/react-query

npm i https://pkg.pr.new/@tanstack/react-query@8383

@tanstack/react-query-devtools

npm i https://pkg.pr.new/@tanstack/react-query-devtools@8383

@tanstack/react-query-next-experimental

npm i https://pkg.pr.new/@tanstack/react-query-next-experimental@8383

@tanstack/react-query-persist-client

npm i https://pkg.pr.new/@tanstack/react-query-persist-client@8383

@tanstack/solid-query

npm i https://pkg.pr.new/@tanstack/solid-query@8383

@tanstack/solid-query-devtools

npm i https://pkg.pr.new/@tanstack/solid-query-devtools@8383

@tanstack/solid-query-persist-client

npm i https://pkg.pr.new/@tanstack/solid-query-persist-client@8383

@tanstack/svelte-query

npm i https://pkg.pr.new/@tanstack/svelte-query@8383

@tanstack/svelte-query-devtools

npm i https://pkg.pr.new/@tanstack/svelte-query-devtools@8383

@tanstack/svelte-query-persist-client

npm i https://pkg.pr.new/@tanstack/svelte-query-persist-client@8383

@tanstack/vue-query

npm i https://pkg.pr.new/@tanstack/vue-query@8383

@tanstack/vue-query-devtools

npm i https://pkg.pr.new/@tanstack/vue-query-devtools@8383

commit: 85ee5bc

@juliusmarminge
Copy link
Contributor Author

really weird, queryFn is getting called and returns the updated value, but the serializer is called only with teh initial count

CleanShot 2024-12-02 at 17 16 59

@juliusmarminge
Copy link
Contributor Author

juliusmarminge commented Dec 2, 2024 •
edited
Loading

before digging deeper, can you confirm if this is a bug or expected @TkDodo ?

I think we might not be in a pending state so the promise doesn't get sent down?

https://github.com/juliusmarminge/query/blob/cf37452e72e44609e49600f8b7c124cc5a197af5/packages/query-core/src/hydration.ts#L83-L92

juliusmarminge
juliusmarminge commented Dec 2, 2024
View reviewed changes
packages/query-core/src/__tests__/hydration.test.tsx Outdated
Comment on lines 1121 to 1125
// --- server ---
countRef.current++
const promise2 = serverQueryClient.prefetchQuery(query)

const dehydrated2 = dehydrate(serverQueryClient)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assume this is a server fn calling revalidatePath() so the page re-renders

@TkDodo
Copy link
Collaborator

TkDodo commented Dec 2, 2024

you’re right: we only send the promise when we are in pending state. The fix is likely to send the promise when we are in fetchStatus: 'fetching' instead 💡

@juliusmarminge
Copy link
Contributor Author

i believe another issue is we're getting dataUpdatedAt: 0 so it does't rehydrate

CleanShot 2024-12-02 at 19 54 15@2x

@juliusmarminge
Copy link
Contributor Author

feels like we need some other state that would let us know if the promise is fresher then we need to set the queries' promise or something.. this goes deeper than I thought

@TkDodo
Copy link
Collaborator

TkDodo commented Dec 2, 2024

I'd say if we get a promise, we should always assume it's newer and hydrate its data, no?

@juliusmarminge
Copy link
Contributor Author

juliusmarminge commented Dec 2, 2024 •
edited
Loading

how do we check for that wihtout causing infinite re-renders in <HydrationBoundary> ?

@juliusmarminge
Copy link
Contributor Author

juliusmarminge commented Dec 2, 2024 •
edited
Loading

struggling to figuring out how to properly filter out the queries to put in the hydration queue

CleanShot.2024-12-02.at.21.01.26.mp4

@TkDodo
Copy link
Collaborator

TkDodo commented Dec 3, 2024

@juliusmarminge have you widened this check?

query/packages/react-query/src/HydrationBoundary.tsx

Lines 74 to 76 in 8fe9010

const hydrationIsNewer =
dehydratedQuery.state.dataUpdatedAt >
existingQuery.state.dataUpdatedAt

to something like:

const hydrationIsNewer =
  dehydratedQuery.state.dataUpdatedAt >
    existingQuery.state.dataUpdatedAt || dehydratedQuery.promise

so that whenever a promise is included, we put it the queue.

@juliusmarminge
Copy link
Contributor Author

yea that causes infinite rerenders

@TkDodo
Copy link
Collaborator

TkDodo commented Dec 3, 2024

interesting; let’s debug that together today if you have some time?

@juliusmarminge
Copy link
Contributor Author

juliusmarminge commented Dec 3, 2024 •
edited
Loading

interesting; let’s debug that together today if you have some time?

sure! what time? I can sneak in a short seesion anytime after 10am gmt+1.

@TkDodo
Copy link
Collaborator

TkDodo commented Dec 3, 2024

I pinged you on discord ;)

juliusmarminge
juliusmarminge commented Dec 3, 2024
View reviewed changes
packages/react-query/src/HydrationBoundary.tsx
existingQuery.state.dataUpdatedAt
existingQuery.state.dataUpdatedAt ||
// @ts-expect-error
dehydratedQuery.promise?.status !== existingQuery.promise?.status
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
package: query-core package: react-query
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@juliusmarminge @TkDodo