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(react-query): ensureSuspenseTimers should ALWAYS set staleTime to 1000 when it is below 1000. #8398

Merged
merged 11 commits into from
Jan 3, 2025
Merged

fix(react-query): ensureSuspenseTimers should ALWAYS set staleTime to 1000 when it is below 1000. #8398

merged 11 commits into from
Jan 3, 2025
+203 −60

Conversation

minseong0324
Copy link
Contributor

issue link

Changes

  • Modified ensureSuspenseTimers to enforce a minimum staleTime of 1000ms in Suspense mode
  • Added support for both direct values and function-based staleTime configurations
  • Maintained the existing behavior for values greater than 1000ms

@nx-cloud Nx Cloud
Copy link

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

View your CI Pipeline Execution ↗ for commit e9b6f96.

Command Status Duration Result
nx affected --targets=test:sherif,test:knip,tes... ✅ Succeeded 2m 4s View ↗
nx run-many --target=build --exclude=examples/*... ✅ Succeeded 22s View ↗

☁️ Nx Cloud last updated this comment at 2025-01-03 17:25:53 UTC

TkDodo
TkDodo reviewed Dec 4, 2024
View reviewed changes
Copy link
Collaborator

@TkDodo TkDodo left a comment

Choose a reason for hiding this comment

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

can you please add a test for this scenario?

@minseong0324
Copy link
Contributor Author

minseong0324 commented Dec 4, 2024 •
edited
Loading

changes:

  1. Fixed logic in ensureSuspenseTimers to properly handle all cases where staleTime needs to be enforced (values below 1000ms, undefined values, and functions returning values below 1000ms)
  2. Added comprehensive test coverage for these scenarios to ensure the behavior works as intended
  • Testing when using a short numeric staleTime value
  • Testing when using a function that returns a short staleTime value
  • Testing when staleTime is explicitly undefined
  • Testing when staleTime is set above 1000ms
  • Testing when using a function that returns a value above 1000ms
  1. Removed the "should refetch when re-mounting" test as it no longer aligns with the new implementation
image

@minseong0324 minseong0324 requested a review from TkDodo December 4, 2024 15:37
@pkg-pr-new pkg.pr.new
Copy link

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

Open in Stackblitz

More templates

@tanstack/angular-query-devtools-experimental

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

@tanstack/angular-query-experimental

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

@tanstack/eslint-plugin-query

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

@tanstack/query-async-storage-persister

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

@tanstack/query-broadcast-client-experimental

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

@tanstack/query-core

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

@tanstack/query-devtools

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

@tanstack/query-persist-client-core

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

@tanstack/query-sync-storage-persister

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

@tanstack/react-query

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

@tanstack/react-query-devtools

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

@tanstack/react-query-next-experimental

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

@tanstack/react-query-persist-client

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

@tanstack/solid-query

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

@tanstack/solid-query-devtools

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

@tanstack/solid-query-persist-client

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

@tanstack/svelte-query

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

@tanstack/svelte-query-devtools

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

@tanstack/svelte-query-persist-client

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

@tanstack/vue-query

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

@tanstack/vue-query-devtools

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

commit: e9b6f96

TkDodo
TkDodo reviewed Dec 11, 2024
View reviewed changes
packages/react-query/src/__tests__/useSuspenseQuery.test.tsx
@@ -327,61 +327,6 @@ describe('useSuspenseQuery', () => {
consoleMock.mockRestore()
})

it('should refetch when re-mounting', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why was this test removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we now enforce a minimum staleTime of 1000ms in Suspense mode for better performance and consistency, the "should refetch when re-mounting" test is no longer valid. This test expected immediate refetching on remount with staleTime: 0, but this behavior would conflict with our implementation that ensures queries remain fresh for at least 1000ms when using Suspense.

The test was designed to verify a behavior that we've intentionally changed to prevent unnecessary refetches and provide a more stable user experience in Suspense mode. Removing this test ensures our test suite accurately reflects our current design decision of enforcing minimum staleTime.

packages/react-query/src/__tests__/suspense.test.tsx Outdated Show resolved Hide resolved
@minseong0324 minseong0324 requested a review from TkDodo December 13, 2024 11:59
@TkDodo TkDodo merged commit 0503282 into TanStack:main Jan 3, 2025
6 checks passed
@kivvvvv kivvvvv mentioned this pull request Jan 5, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TkDodo TkDodo TkDodo approved these changes

Assignees
No one assigned
Labels
package: react-query
Projects
None yet
Milestone
No milestone
Development

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