Thanks for the feedback, @mislav . Sure, I can update this PR. I’ll go for item 1 that you listed.

One concern I have is that if we wrap a native fetch, we will not be able to actually cancel the request, which is an important feature, I think.

Update: This PR has been updated to implement item 1 that was listed in @mislav's comment above.

Should also bail early if the abort signal that you pass in is already aborted.

// Return early if already aborted, thus avoiding making an HTTP request
if (request.signal.aborted) {
  return reject(abortError);
}

ie. if someone where to do:

var controller = new AbortController();
var signal = controller.signal;
controller.abort();
fetch(url, {signal})

But i don't know anyone in there right mind who would do this, but that is the right thing to do.

Just thought about another thing.

You are using init.signal instead of request.signal

Fetch can also be called as followed:

var req = new Request(url, {signal})
fetch(req)

Guessing you have to dig down to the Request constructor and set this.signal = options.signal

Good catch @jimmywarting ! I'll add that.

One thing that baffle me was that Firefox (that supports AbortController) sets a default abortSignal to the Request even if no signal was passed in.

console.log(new Request('http://a').signal) // a signal 😕

I don't know if this is intentional by spec. haven't looked at the spec lately
I also expected Request.prototype.signal to be null. Same goes for
new Request('http://a').signal == null // false

I don't know if this is intentional by spec.

This is intentional. From this page:

This is great, because it makes the feature detection code so small!

@jimmywarting , this has been updated with your suggestions. I think this covers the two cases you brought up:

1. Passing in a Request as the first argument
2. Bailing early if the Controller was aborted before calling fetch

I added a section to the documentation about this. I wasn't too sure where to put it, so I threw it into the caveats. Happy to move that around or reword it or whatever.

When it comes to testing, what would folks like to see here?

When it comes to testing, what would folks like to see here?

Please:

1. Create a “slow” route in script/server, make a request, abort it before it hand a chance to complete;
2. Create a Request with already aborted controller, verify that it's immediately rejected.

Latest changes:

• xhr.abort is now called without any arguments
• simplified example code for aborting fetches
• assumptions are made that request.signal, when it exists, implements the EventTarget interface

To do:

• Add some tests according to @mislav's specifications above
• Investigate that all target browsers support DOMException(message, name). The big ? here is the second argument support. IE9 does not support it afaik, so it'll be interesting to see if 10 does!
• Clean up commits once the above is done

@mislav, assuming IE10 does not support that API of DOMException, what would you propose as the workaround? I wonder if:

var ex = new DOMException(...);
if (!ex.name) {
  ex.name = 'AbortError';
}

would work. If so, is that too gross for your liking? :)

I have discussed overriding native fetch to gain abort functionality with some on my team and here's a compromise we came down to: we will avoid ever overriding native fetch, but we will make module exports consumable in a way such as import {fetch, Request} from 'whatwg-fetch' that allows you access to our own implementations even when browser native equivalents were available. That way, if you use fetch as exported from our module and you have already polyfilled AbortController/Signal, you will always have access to truly abortable requests no matter the browser, because you will be using our own XHR implementation.

So to get "abort means tcp socket is closed" polyfilled globally you would do:

import 'abortcontroller-polyfill/dist/abortcontroller-polyfill-only'
import {fetch, Request} from 'whatwg-fetch'
window.fetch = fetch;
window.Request = Request;

But if you did the following instead you'd get "abort means tcp socket is closed" on non-fetch browsers but "abort means promise is rejected and response is ignored but tcp socket is not closed" for browsers with non-abortable fetch:

import 'abortcontroller-polyfill/dist/abortcontroller-polyfill-only'
import 'whatwg-fetch'

And people who upgrade "whatwg-fetch" without reading release note and thus keep just importing "whatwg-fetch" alone like they did before; these people will get the same behavior as in the case above so it will work for them too (it's only if you really want "proper abort" that you need to modify you imports to fit the first case):

import 'whatwg-fetch'

@mislav did I understand that correctly?

@mo I would not suggest ever overriding window.fetch in one's application, since there's no need when someone uses a module system. Here's what I'm suggesting:

// Use browser native fetch or polyfilled fetch for older browsers. If the browser
// implements fetch, you are at the mercy of its implementation. Using abort
// functionality would not be safe (i.e. it would not work most of the time)
import 'whatwg-fetch'
window.fetch('/').then(...)

// Use polyfilled fetch regardless of browser implementation existing or not. This
// buys you consistent behavior across browsers, which is useful for accessing
// newer features such as aborting.
import 'abortcontroller-polyfill/dist/abortcontroller-polyfill-only'
import {fetch, Request} from 'whatwg-fetch'
fetch(new Request('/', { signal: controller.signal })).then(...)

Yes, the latter approach is a bit more manual and more conscious, but we'd like to take the conservative approach with our polyfill and not ever override browser's native implemenation.

Sounds excellent.

@mislav Is that going to be a part of the same PR? I'm looking forward to taking advantage of the great changes so far!

@dysonpro First we are going to set up the exports (currently whatwg-fetch is not a module) in a separate PR, and then we are going to merge this PR.

Hm, since you don't want to patch fetch in whatwg-fetch, then wouldn't this be enough without this PR?:

// Option 1. Support for older browsers, conservative fetch polyfill (import order matters)
import 'whatwg-fetch'; // as-is
import 'abortcontroller-polyfill'; // patches native or polyfilled fetch with aborting

// Option 2. No need to polyfill fetch (you're only targeting modern browsers)
import 'abortcontroller-polyfill'; // patches native fetch with aborting

Though what I was looking for was:

// Option 1. Support for older browsers, conservative fetch polyfill (import order does not matter)
import 'abortcontroller-polyfill'; // no patch
import 'whatwg-fetch'; // polyfills whole fetch or patch native fetch with aborting

// Option 2. No need to polyfill fetch (you're only targeting modern browsers)
import 'abortcontroller-polyfill'; // no patch
import 'abortcontroller-polyfill/fetch-signal-polyfill'; // patches native fetch with aborting

And ultimately:

// Option 1. Support for older browsers + node (import order does not matter)
import 'abortcontroller-polyfill'; // no patch
import 'isomorphic-fetch'; // both whatwg-fetch and node-fetch polyfill whole fetch or patch native fetch with aborting

or

// Option 1. Support for older browsers + node (import order matters)
import 'isomorphic-fetch'; // no patch
import 'abortcontroller-polyfill'; // no patch
import 'abortcontroller-polyfill/fetch-signal-polyfill'; // patches native or polyfilled fetch with aborting

All is safe and transparent to the user - i.e. no need to choose between module exports vs side-effects, know what is safe and not safe and/or know how to patch fetch with an imperative API.

@joaovieira Your examples are valid. However, in this project, as well as our documentation and examples, we've resolved to avoid any venue where native window.fetch or window.Request are ovewritten. This comes at a cost of users not being able to use latest spec features in a transparent way, as you pointed out, but comes at a benefit of clearer intents when reading code and debugging.

I have cycles to push this over the finish line if there's anything I can PR? 😄Happy to help!

@jayphelps Well, basically what we need to do right now (in a separate PR) is make fetch.js be a UMD module and declare its exports. Regardless of the module stuff, it needs to keep polyfilling window.fetch to keep backwards compatibility. If you want to help set this up, PR welcome! ✨

I’m on it 👍

Edit: ready #616

Let us know if we can help in any way, I think aborting request is a pretty important feature to have!

Please don't advertise your projects on our issue tracker. 🙇

Thank you for your patience!

@joaovieira， thank you for your advice, I have solved the problem!

Thank you @mislav @jayphelps @joaovieira @jimmywarting and everyone else!

Any idea on when the next release will be pushed? I see this was merged but doesn't look like there has been a release in a while.