Add support for AbortController (#54)

[simonbuerger]

Resolves #54

[simonbuerger]

I'm not sure this failed because of my PR? Test seems unrelated?

[simonbuerger]

Right, I see PR #67 resolves the failing tests

[stereobooster]

function AbortController() {
  this.signal = {onabort: () => {}}
  this.abort = () => {
    this.signal.onabort()
  }
}

[developit]

@stereobooster I'm kinda wondering if that piece is required to make this PR usable TBH. I love the idea of keeping things split up, but we should probably just make a call on either supporting or not supporting abort. If we say it's important enough, probably best to install that polyfill (though it pains me).

[stereobooster]

@developit yes it is required, if browser doesn't provide fetch, it will not provide AbortController either. Anybody who would want to use this feature will need to write one anyway.

On the other hand this polyfill (AbortController), will have no effect for browsers which support fetch, because if native fetch doesn't support AbortController this polyfill will not help either.

no fetch	native fetch without AbortController	native fetch with AbortController
AbortController works	AbortController doesn't work	AbortController works

Because of this inconsistency I end up using your code as ponyfill.

[simonbuerger]

I'm not convinced that it's required that we actually provide the AbortController polyfill. I know it's required for the abort feature to actually work, but adding those extra bytes for everyone for only a maybe use case kind of goes against the purpose of a tiny fetch polyfill/ponyfill. We can just say that support for abort requires an appropriate polyfill (like libs do with Promises) and point them elsewhere OR provide the above as a separate export in this module?

I have used https://www.npmjs.com/package/abortcontroller-polyfill with this modified version of unfetch as a ponyfill to ensure consistent abort behaviour.

[developit]

Another option would be to export AbortController separately:

import fetch from 'unfetch';
import AbortController from 'unfetch/AbortController';

I've been thinking of doing the same for Headers, Request and Response.

[simonbuerger]

@developit how are you thinking of doing that? I mean would you import and use the Request, Response and Headers implementations directly in the main unfetch function? Seems like that could be a lot of extra bytes... Or are you imagining them as completely separate entities?

[joaovieira]

This has been discussed in whatwg-fetch as well before, starting from JakeChampion/fetch#592 (comment).

You might also want to look at https://github.com/Netflix/yetch/blob/master/polyfill.js. It polyfills fetch wether it doesn't exist or wether it doesn't support abort controller.

My personal opinion as a user is what I mentioned in JakeChampion/fetch#592 (comment) - the fetch polyfill to implement entire fetch or add the missing parts (options.signal).

Lastly, why re-implement AbortSignal/AbortController (#94) when there are some fully WHATWG compliant polyfills out there already (e.g. https://github.com/mysticatea/abort-controller - which is what node-fetch will probably use - node-fetch/node-fetch#437).

[MrLoh]

Will this ever be merged, seems like the discussion got stuck.

[nfantone]

Don't mean to be impetuous here, but - maybe we are overthinking this? We've been discussing it for almost two years. This is, literally, adding two lines of code to support something all major browser already do.

[developit]

@nfantone AbortController is pretty well-supported now, yes. At the time of this original discussion it was only supported in Chrome. I think now we can merge this - unfetch is likely to get a little bump up in size in order to accomodate the addition of Headers, Response and Request interfaces, which see increased usage nowadays.

[prk3]

options.signal.onabort = () => request.abort();

There might be a problem with this implementation. With native fetch, many requests can be cancelled with one abort call. If we override onabort, only one fetch will be canceled. Consider this example:

const ctl = new AbortController();
fetch('foo', { signal: ctl.signal }).then(action1).catch(() => {});
fetch('bar', { signal: ctl.signal }).then(action2).catch(() => {});
ctl.abort();

action1 may still be executed, as signal's 'abort' handler was overridden by the second call to fetch.
I'd recommend replacing .onabort = ... with .addEventListener('abort', ...). Alternatively, we could chain aborts:

const current = options.signal.onabort || () => {};
options.signal.onabort = () => { current(); request.abort(); }

I am not sure about side-effects of this one though.

[sayjeyhi]

Any update?
node-fetch added support to AbortController , now if we want to use isomorphic-unfetch with abortable request , unfetch do not let us...
node-fetch/node-fetch#95

[prk3]

@developit @sayjeyhi I submitted PR #137 with alternative implementation. It solves problems I mentioned earlier at a cost of build size. If you decide to merge this (#68) PR without changes, please add a note to the README explaining what to expect from abort behaviour.

[developit]

I think the two issues addressed in #137 make it preferable here - AbortController should be usable across multiple fetch() calls, and it should really reject with a value, since promise handlers are likely to be checking for such a value.

[mfuentesg]

Will this ever be merged? :(