Skip to content

Commit

Permalink
chore: retry > options.maxRetries has been eliminated and only should…
Browse files Browse the repository at this point in the history
…Retry is used to determine if a retry is needed.
  • Loading branch information
kahirokunn authored and markerikson committed Aug 21, 2022
1 parent 89cab7a commit 0f62792
Showing 1 changed file with 15 additions and 8 deletions.
23 changes: 15 additions & 8 deletions packages/toolkit/src/query/retry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,12 @@ const retryWithBackoff: BaseQueryEnhancer<
RetryOptions,
RetryOptions | void
> = (baseQuery, defaultOptions) => async (args, api, extraOptions) => {
const defaultShouldRetry: Exclude<RetryOptions['shouldRetry'], undefined> = (_, __, {attempt, maxRetries}) => attempt <= maxRetries

const options = {
maxRetries: 5,
backoff: defaultBackoff,
shouldRetry: () => true,
shouldRetry: defaultShouldRetry,
...defaultOptions,
...extraOptions,
}
Expand All @@ -69,25 +71,30 @@ const retryWithBackoff: BaseQueryEnhancer<
try {
const result = await baseQuery(args, api, extraOptions)
// baseQueries _should_ return an error property, so we should check for that and throw it to continue retrying
if (result.error && options.shouldRetry(result.error as FetchBaseQueryError, args, {
attempt: retry,
maxRetries: options.maxRetries,
baseQueryApi: api,
extraOptions
})) {
if (result.error) {
throw new HandledError(result)
}
return result
} catch (e: any) {
retry++
if (e.throwImmediately || retry > options.maxRetries) {

if (e.throwImmediately) {
if (e instanceof HandledError) {
return e.value
}

// We don't know what this is, so we have to rethrow it
throw e
}

if (e instanceof HandledError && !options.shouldRetry(e.value as FetchBaseQueryError, args, {
attempt: retry,
maxRetries: options.maxRetries,
baseQueryApi: api,
extraOptions
})) {
return e.value
}
await options.backoff(retry, options.maxRetries)
}
}
Expand Down

2 comments on commit 0f62792

@eloiqs
Copy link

@eloiqs eloiqs commented on 0f62792 Jan 14, 2024

Choose a reason for hiding this comment

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

This change looks like it stops supporting throwing the error once the max retries have been reached in the case that the error thrown is not of type HandledError, which kind of means pretty much every errors thrown by a back-end, or a graphql client unless we catch everything in base query and transform into HandledError. I can confirm that maxRetries have no effect whatsoever in my project, it keeps retrying indefinitely. Weird that no one has reported this however?

@markerikson
Copy link
Collaborator

Choose a reason for hiding this comment

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

@eloiqs comments on commits aren't useful and we don't keep track of those. If you have specific concerns, please file an actual issue.

Please sign in to comment.