-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 AbortError
being triggered incorrectly on createApi
endpoint timeout
#4628
Fix AbortError
being triggered incorrectly on createApi
endpoint timeout
#4628
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
✅ Deploy Preview for redux-starter-kit-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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 d9a9106:
|
…OUT_ERROR` instead of generic `AbortError`.
b6c5d1f
to
d9a9106
Compare
Could you please explain the reasoning here a bit? It's not immediately obvious to me. |
Sure thing! As I alluded to in #2915, the issue exists because of a bit of a race condition happening.
So even though redux-toolkit/packages/toolkit/src/query/fetchBaseQuery.ts Lines 282 to 286 in bc0bf0f
Since The solution I applied was to create an redux-toolkit/packages/toolkit/src/query/fetchBaseQuery.ts Lines 228 to 233 in d9a9106
To sum it up, in this PR, we avoid the race condition in Of note: As far as I understand best practices for using
|
I see, thank you for the explanation. It's a shame that diff --git a/packages/toolkit/src/query/fetchBaseQuery.ts b/packages/toolkit/src/query/fetchBaseQuery.ts
index 9dc14d97..f2a3b732 100644
--- a/packages/toolkit/src/query/fetchBaseQuery.ts
+++ b/packages/toolkit/src/query/fetchBaseQuery.ts
@@ -225,12 +225,8 @@ export function fetchBaseQuery({
...rest
} = typeof arg == 'string' ? { url: arg } : arg
- let abortController: AbortController | undefined, signal = api.signal
- if (timeout) {
- abortController = new AbortController()
- api.signal.addEventListener('abort', abortController.abort)
- signal = abortController.signal
- }
+ const timeoutSignal = timeout ? AbortSignal.timeout(timeout) : undefined
+ const signal = timeoutSignal ? AbortSignal.any(api.signal, timeoutSignal) : api.signal
let config: RequestInit = {
...baseFetchOptions,
@@ -277,27 +273,17 @@ export function fetchBaseQuery({
const requestClone = new Request(url, config)
meta = { request: requestClone }
- let response,
- timedOut = false,
- timeoutId =
- abortController &&
- setTimeout(() => {
- timedOut = true
- abortController!.abort()
- }, timeout)
+ let response
try {
response = await fetchFn(request)
} catch (e) {
return {
error: {
- status: timedOut ? 'TIMEOUT_ERROR' : 'FETCH_ERROR',
+ status: timeoutSignal?.aborted ? 'TIMEOUT_ERROR' : 'FETCH_ERROR',
error: String(e),
},
meta,
}
- } finally {
- if (timeoutId) clearTimeout(timeoutId)
- abortController?.signal.removeEventListener('abort', abortController.abort)
}
const responseClone = response.clone() |
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.
Looks good to me :)
Ah, yes, that's a thing of beauty! |
thanks! |
This PR:
createApi
endpoint would return a genericAbortError
instead ofTIMEOUT_ERROR
.