-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Support abort API #592
Support abort API #592
Changes from 1 commit
93e9cec
53565c1
9a7d232
fd5c261
318e95f
0e1b651
d47f6c9
529c9fa
8ba0cdb
074c89e
b832229
c6aff30
d9aa590
886a076
9f9a00f
bab0f3d
0bed2ba
66ed8ba
738d51a
8f778f3
f0e24f8
36bf692
7f6b8d2
c31fca9
f035860
2bd7f22
c873843
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,21 @@ | ||
(function(self) { | ||
'use strict'; | ||
|
||
if (self.fetch) { | ||
function canAbortFetch() { | ||
if (!self.AbortController || !self.AbortSignal) { | ||
return false | ||
} | ||
|
||
var abortController = new self.AbortController() | ||
|
||
var request = new self.Request('http://a', { | ||
signal: abortController.signal | ||
}) | ||
|
||
return Boolean(request.signal) | ||
} | ||
|
||
if (self.fetch && canAbortFetch()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this entirely overwrite the native I don't think that's acceptable. This polyfill should perhaps wrap native fetch to provide it with abort functionality, but not overwrite it entirely. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct, that is the current behavior. The intention here was to ensure that the HTTP requests are actually canceled to reduce the amount of data being sent over the network. |
||
return | ||
} | ||
|
||
|
@@ -443,6 +457,10 @@ | |
reject(new TypeError('Network request failed')) | ||
} | ||
|
||
xhr.onabort = function() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need to attach There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mislav I believe this is the most correct as mentioned here. Calling abort on the XHR will set the right status code and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fine with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see.. xhr is never exposed so it can't be aborted from the outside. Is there any other way it could be aborted (e.g. by the browser)? If not, there shouldn't be any difference. I don't mind this way though, reads well :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dysonpro That's correct, but we aren't interested in whether There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @mislav for your explanation. I am excited about this functionality so I have been following this PR carefully. I am already using this branch code in a project that requires aborting no longer needed requests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dysonpro Your concerns are valid, but we have different priorities when thinking about this. At the point when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
These are rather obscure use cases, but they're real nonetheless. I think a general There's nothing wrong with immediately rejecting after calling |
||
reject(new DOMException('Aborted', 'AbortError')) | ||
} | ||
|
||
xhr.open(request.method, request.url, true) | ||
|
||
if (request.credentials === 'include') { | ||
|
@@ -459,6 +477,10 @@ | |
xhr.setRequestHeader(name, value) | ||
}) | ||
|
||
if (init.signal && init.signal.addEventListener) { | ||
init.signal.addEventListener('abort', xhr.abort) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we remove the event listener also in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup. I'll do that. |
||
} | ||
|
||
xhr.send(typeof request._bodyInit === 'undefined' ? null : request._bodyInit) | ||
}) | ||
} | ||
|
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.
Presumably, the truthiness of
self.fetch
from L18 indicates thatself.Request
exists. To be more explicit, we could addself.Request
to L5 if that's preferable.