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.

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

feat(query-core): Expose retry error as failureReason #4315

Merged
merged 5 commits into from
Oct 22, 2022

Conversation

Kuirak
Copy link
Contributor

@Kuirak Kuirak commented Oct 13, 2022

Added failureReason: TError | null to query as suggested here:
#4305

Open Question: Would it make sense to add this to mutations as well?

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 13, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 4b47b01:

Sandbox Source
@tanstack/query-example-react-basic-typescript Configuration
@tanstack/query-example-solid-basic-typescript Configuration
@tanstack/query-example-vue-basic Configuration

@TkDodo
Copy link
Collaborator

TkDodo commented Oct 13, 2022

Open Question: Would it make sense to add this to mutations as well?

I would say so, yes.

@Kuirak
Copy link
Contributor Author

Kuirak commented Oct 13, 2022

Open Question: Would it make sense to add this to mutations as well?

I would say so, yes.

While implementing it I stumbled over:

  1. failureCount was not documented for useMutation
  2. failureCount implementation differs as it doesn't reset to 0 after a successful mutation, this led to not using the failureCount from the action but incrementing the one in the state

@Kuirak Kuirak requested a review from TkDodo October 13, 2022 15:51
docs/reference/useMutation.md Outdated Show resolved Hide resolved
packages/query-core/src/mutation.ts Outdated Show resolved Hide resolved
@TkDodo
Copy link
Collaborator

TkDodo commented Oct 13, 2022

failureCount implementation differs as it doesn't reset to 0 after a successful mutation

I'd say this is a bug, isn't it? Why would the failureCount behave differently for mutations

@TkDodo
Copy link
Collaborator

TkDodo commented Oct 13, 2022

@ardeora @DamianOsipiuk if you have some time, please take a look at the PR and let me know if you agree with the api decision

@Kuirak Kuirak requested a review from TkDodo October 13, 2022 19:35
@DamianOsipiuk
Copy link
Contributor

Looks good to me 👍

@Kuirak Kuirak requested a review from TkDodo October 16, 2022 18:40
@TkDodo
Copy link
Collaborator

TkDodo commented Oct 16, 2022

seems like the new test fails and there are also lint errors.

@Kuirak
Copy link
Contributor Author

Kuirak commented Oct 16, 2022

seems like the new test fails and there are also lint errors.

Oh I am sorry, I forgot to run prettier. Haven't setup format on save in this repo yet.

=> Fixed Tests and linting

@codecov-commenter
Copy link

Codecov Report

Base: 96.36% // Head: 92.27% // Decreases project coverage by -4.09% ⚠️

Coverage data is based on head (4b47b01) compared to base (eab6e2c).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4315      +/-   ##
==========================================
- Coverage   96.36%   92.27%   -4.10%     
==========================================
  Files          45       87      +42     
  Lines        2281     3558    +1277     
  Branches      640      899     +259     
==========================================
+ Hits         2198     3283    +1085     
- Misses         80      259     +179     
- Partials        3       16      +13     
Impacted Files Coverage Δ
src/core/onlineManager.ts
src/react/reactBatchedUpdates.ts
src/devtools/useLocalStorage.ts
src/core/mutation.ts
src/devtools/theme.tsx
src/react/QueryErrorResetBoundary.tsx
src/react/useIsFetching.ts
src/core/focusManager.ts
src/devtools/useMediaQuery.ts
...rc/createWebStoragePersistor-experimental/index.ts
... and 122 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@TkDodo TkDodo merged commit 07c9631 into TanStack:main Oct 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants