-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
fix(query-core): make retry failureCount 1-based #10020
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(query-core): make retry failureCount 1-based #10020
Conversation
Signed-off-by: kkh <[email protected]>
🦋 Changeset detectedLatest commit: b3e528d The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughThe PR changes retry behavior to pass a 1-based Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/framework/react/guides/query-retries.md (1)
10-13: Documentation content is accurate; consider varying sentence structure for readability.The new explanation of the function-based retry behavior is clear and correct. However, four successive list items beginning with "Setting" creates repetitive phrasing that could be improved. Consider rewording one or more items to vary the sentence structure while preserving the clarity of each option.
♻️ Suggested refactor for improved readability
- Setting `retry = false` will disable retries. - Setting `retry = 6` will retry failing requests 6 times before showing the final error thrown by the function. - Setting `retry = true` will infinitely retry failing requests. - Setting `retry = (failureCount, error) => ...` allows for custom logic based on why the request failed. The `failureCount` starts at `1` for the first failure. + - `retry = false` disables retries. + - `retry = 6` will retry failing requests 6 times before showing the final error thrown by the function. + - `retry = true` will infinitely retry failing requests. + - `retry = (failureCount, error) => ...` allows for custom logic based on why the request failed. The `failureCount` starts at `1` for the first failure.Alternatively, vary the opening phrases further (e.g., "Use
retry = 6to..." or "To disable retries, setretry = false...") for even more variety.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/bright-rabbits-jump.mddocs/framework/react/guides/query-retries.mddocs/framework/react/reference/useMutation.mddocs/framework/react/reference/useQuery.mddocs/framework/solid/reference/useQuery.mdpackages/query-core/src/__tests__/query.test.tsxpackages/query-core/src/retryer.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-22T09:06:05.219Z
Learnt from: sukvvon
Repo: TanStack/query PR: 9892
File: packages/solid-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx:331-335
Timestamp: 2025-11-22T09:06:05.219Z
Learning: In TanStack/query test files, when a queryFn contains side effects (e.g., setting flags for test verification), prefer async/await syntax for clarity; when there are no side effects, prefer the .then() pattern for conciseness.
Applied to files:
packages/query-core/src/__tests__/query.test.tsx
🧬 Code graph analysis (1)
packages/query-core/src/__tests__/query.test.tsx (1)
packages/query-core/src/query.ts (1)
promise(198-200)
🪛 LanguageTool
docs/framework/react/guides/query-retries.md
[style] ~13-~13: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ll infinitely retry failing requests. - Setting retry = (failureCount, error) => ... ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (6)
docs/framework/solid/reference/useQuery.md (1)
279-279: ✓ Clear and accurate documentation update.The note accurately documents the 1-based
failureCountbehavior whenretryis a function, with appropriate placement in the parameter reference section.docs/framework/react/reference/useMutation.md (1)
89-89: ✓ Consistent documentation across frameworks.The addition mirrors the Solid documentation update and clearly states the 1-based
failureCountbehavior, maintaining consistency across framework-specific references..changeset/bright-rabbits-jump.md (1)
1-5: Changeset message clearly reflects the behavioral fixThe summary correctly states that the
retrycallback’sfailureCountis now 1-based on the first failure, and it’s scoped as a patch for@tanstack/query-core, which matches the implementation and test coverage in this PR.packages/query-core/src/retryer.ts (1)
169-178: 1-basedfailureCountfor functionretryis consistent with the rest of the APIPassing
failureCount + 1into the functionretrybranch makes the callback see[1, 2, …]for successive failures, which now aligns with howonFailand the publicfailureCounton query state are interpreted (1-based on first retryable failure).One thing to double-check:
retryDelaystill receives the rawfailureCount(0 on first failure) while the functionretrynow gets 1-based values. If the intention is thatretryAttemptinretryDelaystays 0-based for backward compatibility, the current code is correct; if we eventually want both callbacks to be 1-based, that would require a separate, more breaking change (and doc update).packages/query-core/src/__tests__/query.test.tsx (1)
156-181: New test accurately locks in 1-basedfailureCountbehaviorThis test exercises a function
retrywith a side-effectfulqueryFnand correctly asserts thatfailureCountvalues passed to the callback are[1, 2]before the successful third attempt. Timer usage (retryDelay: 1+advanceTimersByTimeAsync(10)) is sufficient and keeps the test fast. Based on learnings, the async/await style here is appropriate given the side effects in the queryFn.docs/framework/react/reference/useQuery.md (1)
83-88: Doc note correctly documents 1-basedfailureCountfor functionretryThe added bullet clearly states that, for function
retry, thefailureCountargument now starts at1on the first failed attempt, which matches the implementation and tests in this PR. Just make sure the same wording (or equivalent) is present in the other retry-related docs (useMutation, SoliduseQuery, retries guide) so behavior is documented consistently across frameworks.
Signed-off-by: kkh <[email protected]>
|
Hi! Thank you for the thoughtful review and checks. The Docstring Coverage warning appears to come from CodeRabbit's internal pre-merge checks. This PR is mostly documentation updates with a small runtime tweak, and the touched files follow the existing style that generally doesn't use JSDoc, so it shows 0%. If maintainers would like this addressed, I'm happy to add docstrings in the relevant spots. Could you please review the changes, and if everything looks good, approve? Thank you! |
🎯 Changes
failureCountto start at 1 on the first failure (query-core)failureCountin retry docsFixes #10017
✅ Checklist
pnpm run test:pr. (Rannpx nx run @tanstack/query-core:test:lib)🚀 Release Impact
Summary by CodeRabbit
Bug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.