Add guard for aborted requests. #98
Conversation
The `onreadystatechange` callback should proceed only when the request has not been aborted.
|
Should there be a specific unit test for this, or do you think the existing tests already cover this off fully? |
|
Nevermind... I see you comment in #94 about the unit tests. |
| @@ -60,7 +66,7 @@ export default function xhr<T>(url: string, options: XhrRequestOptions = {}): Re | |||
|
|
|||
| let timeoutHandle: Handle; | |||
| request.onreadystatechange = function (): void { | |||
| if (request.readyState === 4) { | |||
| if (!isAborted && request.readyState === 4) { | |||
There was a problem hiding this comment.
Shouldn't we handle (as in reject) isAborted && request.readyState === 4)? Because obviously that is happening at runtime and that should result in a rejected promise, right? With something like resolve(new Error('An aborted request has been resolved'));
There was a problem hiding this comment.
If the request times out, then the underlying promise is rejected. However, since the request object itself is unaware of any promise state, it fires that onreadystatechange callback anyway. But you're right; when Task#cancel is executed, the task itself is not rejected. So I wonder if rejecting the promise as aborted within the local abort function and resetting onreadystatechange to a noop function is the way to go here.
There was a problem hiding this comment.
While the request object is unaware of the promise state, it is within closure that has the related promise, named as promise, but I agree that the best thing to do is reject and set to a noop.
Resolves #94. Adds a guard to the
onreadystatechangecallback so that it only proceeds when the request has not been aborted. As a result, aborted requests will not throw errors in IE9.