AbortController ponyfill - #94
stereobooster wants to merge 1 commit into
Conversation
Sorry, something went wrong.
| request.onerror = reject; | ||
|
|
||
| if (options.signal) options.signal.onabort = () => { request.abort(); } | ||
| request.onabort = () => reject(new DOMException('The user aborted a request.')); |
There was a problem hiding this comment.
this is to simulate what Chrome does, but i don't insist on it
Sorry, something went wrong.
There was a problem hiding this comment.
per the spec (even that chrome exception has a name of AbortError):
reject(new DOMException('Aborted', 'AbortError'));https://dom.spec.whatwg.org/#aborting-ongoing-activities
https://dom.spec.whatwg.org/#aborting-ongoing-activities-example
https://github.com/github/fetch/blob/master/fetch.js#L446
Sorry, something went wrong.
There was a problem hiding this comment.
Also, don't think DOMException is globally available:
https://developer.mozilla.org/en-US/docs/Web/API/DOMException
https://github.com/github/fetch/pull/592/files#diff-37e6c8c3d38d10cb0fc896766eb1b692R427
Sorry, something went wrong.
There was a problem hiding this comment.
I wonder what browsers don't have it. I can't find data about this in caniuse. Shall we use Error instead?
Sorry, something went wrong.
| @@ -0,0 +1,6 @@ | |||
| export default function() { | |||
| this.signal = { onabort: () => {} }; | |||
There was a problem hiding this comment.
signal has an aborted flag indicating it's been aborted or not, which can be handy:
https://dom.spec.whatwg.org/#abortsignal-aborted-flag
https://github.com/mo/abortcontroller-polyfill/blob/master/src/abortcontroller.js#L42
https://github.com/mysticatea/abort-controller/blob/master/src/abort-signal.mjs#L28
Sorry, something went wrong.
| request.onerror = reject; | ||
|
|
||
| if (options.signal) options.signal.onabort = () => { request.abort(); } | ||
| request.onabort = () => reject(new DOMException('The user aborted a request.')); |
There was a problem hiding this comment.
per the spec (even that chrome exception has a name of AbortError):
reject(new DOMException('Aborted', 'AbortError'));https://dom.spec.whatwg.org/#aborting-ongoing-activities
https://dom.spec.whatwg.org/#aborting-ongoing-activities-example
https://github.com/github/fetch/blob/master/fetch.js#L446
Sorry, something went wrong.
|
|
||
| request.onerror = reject; | ||
|
|
||
| if (options.signal) options.signal.onabort = () => { request.abort(); } |
There was a problem hiding this comment.
if signal is already aborted it should reject promise, first thing:
https://dom.spec.whatwg.org/#aborting-ongoing-activities-spec-example
https://github.com/github/fetch/blob/master/fetch.js#L445
https://github.com/bitinn/node-fetch/pull/437/files#diff-1fdf421c05c1140f6d71444ea2b27638R43
Sorry, something went wrong.
|
Thanks for links to fetch spec. I got the idea of passing aborted signal to the fetch, but I wonder what the use case? In what kind of API you would create N-signals and attach to fetch later? I create signal right before sending request and save it until I got response, to be able to cancel if I need to create another fetch (and only one allowed at the same time). Should be trivial to implement, I just wonder do we need it or not |
Sorry, something went wrong.
|
@stereobooster I also didn't have that use case yet. But I can think of using it as a hand-rolled circuit-breaking system where you use a signal to reject all requests going to a troubled/struggling downstream service. Anyway, many people will use unfetch with isomorphic-unfetch so having consistent functionality with node-fetch (and thus, the spec) sounds right. Specially if it's easy :) |
Sorry, something went wrong.
The point of unfetch it is minimal implementation, it is not fully compatible with spec and never meant to be. It is just subset of spec. The question is where to draw the line. I would say we need to support popular case only |
Sorry, something went wrong.
Related: