Conversation
stereobooster
force-pushed
the
abort-controller
branch
from
79265be to
5b907e4
Compare
| 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
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
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
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?
| @@ -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
| 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
|
|
||
| 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
|
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 |
|
@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 |
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 |
2 participants
Related: