Added AbortController polyfill

[coldhamix]

Closes #547. Used polyfill from https://github.com/mo/abortcontroller-polyfill/. Tests included.

[mislav]

Thanks for the suggestion! This looks like it simply pastes the other polyfill after ours. Could we instead keep the projects separate and instruct the users to compose the polyfills if they need abort functionality?

[coldhamix]

I was concerned about the script order (which might be not really easy to maintain) as the fetch polyfill should always be before AbortController polyfill. Moreover abort controller is a part of fetch standard and it does not feel really good to me to separate its functionality into several polyfills.

[mislav]

Sure, there is some convenience having the complete fetch polyfill in one place. But there are also benefits of keeping them separate, such as better maintainability over time, and loading order shouldn't be a problem for anyone who loads JS files in their app.

@mo @jimmywarting What do you think about this?

[web-devel]

@mislav don't you think that once abort signal is landed in browsers it will be strange not to see it in this polyfill

[jimmywarting]

I think that the abortable controller shouldn't be part of this fetch polyfill
A abortable controller dosen't have anything to do with fetch, it's meant to be used with many other promise based api's

However I think this fetch polyfill should be able to handle a abort if a person send a AbortSignal in as an options and actually call xhr.abort() when a signal is aborted

The problem with this PR is that while it rejects and dose what it is suppose to do... It actually doesn't close the connection when the request is aborted (as mention in the readme.md)

It's up to every promise based api to handle a AbortSignal.

---

And when it comes to order of execution... AbortController should load before fetch.

Personally i would close this PR (unless changed), the tests are grate but not the rest

[jimmywarting]

This is what I think should be implemented instead

function abortError(){
  var err;
  try {
    err = new DOMException('Aborted', 'AbortError');
  } catch (err) {
    // IE 11 does not support calling the DOMException constructor, use a
    // regular error object on it instead.
    err = new Error('Aborted');
    err.name = 'AbortError';
  }

  return err
}

// ...

return new Promise(function(resolve, reject) {
  var request = new Request(input, init)
  var xhr = new XMLHttpRequest()
  
  if (request.signal) {
    // Return early if already aborted, thus avoiding making an HTTP request
    if (request.signal.aborted) {
      return reject(abortError());
    }
    
    request.signal.addEventListener('abort', function () {
      reject(abortError())
      xhr.abort();
    }, {once: true});
  }

  // rest of the xhr code
  // ...
})

And also able to pass the signal into new request when cloning them if that is not taken care of...

IMO i think the abortcontroller is wrong to even patch the fetch api, shouldn't even belong there

[coldhamix]

@jimmywarting @mislav How should it behave in case when there is already a native fetch implementation, but it does not support AbortController? Ignore it and polyfill with XHR?

[jimmywarting]

The only way to abort a request today is by using xhr, You can't even use native fetch implementation. Firefox will be first to release it in there next version (v57)

This code here should really only be implemented by a simple test to see if Request supports signal, that way it can patch native fetch and not doing it for a polyfill like this if it's dose what it's supposed to do

eg:

var controller = new AbortController();
var signal = controller.signal;
if (window.Request) {
  var request = new Request('/', { signal });

  if (!request.signal) {
    // Native implementation don't support signal
    // patch it only then
  }
}

[just-boris]

While I very much appreciate the work is going on here, I would like to mention, that the proposed solution doesn't fully match the spec. Instead of actually aborting request, it just simulates the aborted response into the promise, but the request is being made.

Rather than append the abortable part at the end of the file, I would suggest modifying the original fetch function. Moving change into the main part would give an access to the inner xhr object, so it may become truly abortable.

AbortController can be still shipped separately and can be included into a page in any order because there will be no direct dependencies and relying on object presence.

[jimmywarting]

@just-boris Pretty much what I have been saying already

[mo]

I've prepared an abortcontroller-polyfill branch based on @jimmywarting's patch that broke AbortSignal + AbortController out into a separate file so it can be imported without also getting a patched fetch():

https://github.com/mo/abortcontroller-polyfill/tree/break-out-abortcontroller/src

I will only merge that to abortcontroller-polyfill master if it's useful for you; if you go in another direction I will leave abortcontroller-polyfill master as-is.

[ghost]

The Abort feature is in the fetch spec. therefore I think that making it in another polyfill won't be the correct solution.

https://developers.google.com/web/updates/2017/09/abortable-fetch

[jimmywarting]

@Attrash-Islam the fetch spec is only covering accepting an abort signal. Not how a abortcontroller is made.

A abort controller can be used for so much more then just the fetch api. fetch api so just happens to be the first public usecase where an abort signal can actually take places in any native javascript function.

Next time link to an actual spec and not to some blog post covering it's use cases.

[jamesplease]

I attempted another pass at a resolution to #547 over in #592 based on some of the feedback here.

• It doesn't require an AbortController or AbortSignal polyfill to be included in this library
• It actually aborts the request
• Minimal code changes
• Draws inspiration from the excellent polyfill over at https://github.com/mo/abortcontroller-polyfill/

I'm curious to hear what others think of it!

[mislav]

Abortable fetch requests are now supported after #592. Note that AbortController polyfill needs to be loaded separately in order for this to work in older browsers.