retry condition by http error response

[kahirokunn]

https://github.com/reduxjs/redux-toolkit/issues/2175

[kahirokunn]

@phryneas How about this for implementation? One concern is that I was quite confused about the name of the option. If you have a more appropriate name, I would appreciate suggestions. Thx.

[codesandbox-ci[bot]]

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

Sandbox	Source
Vanilla	Configuration
Vanilla Typescript	Configuration
rsk-github-issues-example	Configuration
@examples-query-react/basic	Configuration
@examples-query-react/advanced	Configuration
@examples-action-listener/counter	Configuration

[netlify[bot]]

✅ Deploy Preview for redux-starter-kit-docs ready!

Name	Link
🔨 Latest commit	6d873612c91d66bf8520dc61d083ff3df9a9a240
🔍 Latest deploy log	https://app.netlify.com/sites/redux-starter-kit-docs/deploys/62ca492c9ec006000a1c7c18
😎 Deploy Preview	https://deploy-preview-2239--redux-starter-kit-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[kahirokunn]

@markerikson @msutkowski Can anyone give me a review? 😢

[phryneas]

Sorry I'm just coming back to this so late.

I think we could make this a bit more generic.

What do you think about a method shouldRetry(error, args, {attempt, maxRetries, baseQueryApi, extraOptions}): boolean ?

[kahirokunn]

@phryneas Agree 👍 How about now?

[locothedev]

Hey I was looking for such a function @phryneas can we have some eyes on this nice MR?

[rayyan224]

Nice Feature To have! @phryneas if all is well lets merge 🥳

[kahirokunn]

@phryneas Thx! If the way it looks good, I will add a test at the end.

[phryneas]

It looks good!

Could you please give it a small rename to retryCondition (we just discussed that we want to be a little more consistent 😅 ) and add some tests?

[kahirokunn]

@phryneas Is this perfect?

[jmberrueta]

This is great! I came here looking specifically for what was asked at https://github.com/reduxjs/redux-toolkit/issues/2175.

One comment though, would it make sense to have this still be agnostic of FetchBaseQueryError? In one of our projects we're using a custom baseQuery, and one nice thing about the current retry implementation is it works no matter the baseQuery's error type.

Then (supper minor comment) it felt strange that, if provided, the retryCondition implementation should take care of considering the maxRetries. I was initially expecting maxRetries to be evaluated independently of whether a retry condition was provided or not.

[phryneas]

@jmberrueta

One comment though, would it make sense to have this still be agnostic of FetchBaseQueryError? In one of our projects we're using a custom baseQuery, and one nice thing about the current retry implementation is it works no matter the baseQuery's error type.

That's good input, yes, that should inherit the Error of the baseQuery it is used with, not FetchBaseQueryError by default

Then (supper minor comment) it felt strange that, if provided, the retryCondition implementation should take care of considering the maxRetries. I was initially expecting maxRetries to be evaluated independently of whether a retry condition was provided or not.

I get the initial confusion, but this would be the best escape hatch we could get to allow for a dynamic number of retries based on received data.

[jmberrueta]

I get the initial confusion, but this would be the best escape hatch we could get to allow for a dynamic number of retries based on received data.

@phryneas, yeah, but that could still be achieved by not providing a maxRetries while having the retryCondition still check for a maximum number of retries on its own, right? But I would also agree my thinking here might be just too much of an opinion. Either way, this looks like a great feature to have!

[phryneas]

That is a fair argument - it might make sense to allow specifying either maxRetries or condition, but not both 🤔

[markerikson]

Ok, I think this is updated to match Lenz's requests.

@phryneas final check on this before I merge it?

[kahirokunn]

Sorry for the slow work, I have been too busy with work for the past two months. It is already late, but I was thinking of the following code.

https://github.com/kahirokunn/redux-toolkit/commit/acce9c332ac08486d24c131c430ce7fe3d84f367

The code you wrote instead this time is very helpful.

[markerikson]

Let's go ahead and get this in. @kahirokunn , thanks for the work here!

[locothedev]

@kahirokunn Awesome stuff man

Thanks! @markerikson