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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 8 commits into from
+203 −60

Conversation

minseong0324
Copy link

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

☁️ Nx Cloud Report

CI is running/has finished running commands for commit b277d28. 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.

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
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

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/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/eslint-plugin-query

npm i https://pkg.pr.new/@tanstack/eslint-plugin-query@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: b277d28

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
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
Comment on lines 223 to 226
// Wait longer than 1000ms but less than returned staleTime
await act(async () => {
await sleep(2000)
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

this will make for a very long test. fakeTimers from vitest are probably the way to go here. We have some examples already, e.g. here:

query/packages/react-query/src/__tests__/useSuspenseQueries.test.tsx

Line 37 in 1eebfc9

vi.useFakeTimers()

Copy link
Author

Choose a reason for hiding this comment

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

image

I improved the test code through a lot of thought to improve the test time, and the useFakeTimers you gave me were especially helpful. Thanks.

46057ca

@minseong0324 minseong0324 requested a review from TkDodo December 13, 2024 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TkDodo TkDodo Awaiting requested review from TkDodo

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