Support abort API #592

jamesplease · 2017-12-07T09:21:19Z

This is a quick pass at an attempt to resolve #547 . The intention is that it would supersede #572

Todo:

Tests
~~Verify that the feature detection works~~
Update documentation

Feedback is welcome! Thanks for reviewing ✌️

jamesplease · 2017-12-07T09:40:24Z

fetch.js

+
+    var abortController = new self.AbortController()
+
+    var request = new self.Request('http://a', {


Presumably, the truthiness of self.fetch from L18 indicates that self.Request exists. To be more explicit, we could add self.Request to L5 if that's preferable.

mislav

I think we should approach this problem in two steps (two separate PRs):

Implement abort functionality in our own implementation of fetch, when native window.fetch isn't present in the browser at all;
Wrap native window.fetch to provide it with faux abort functionality if one doesn't exist.

Can you pick one for this PR?

mislav · 2017-12-07T15:17:55Z

fetch.js

+    return Boolean(request.signal)
+  }
+
+  if (self.fetch && canAbortFetch()) {


Does this entirely overwrite the native window.fetch implementation if fetch isn't abortable?

I don't think that's acceptable. This polyfill should perhaps wrap native fetch to provide it with abort functionality, but not overwrite it entirely.

Correct, that is the current behavior. The intention here was to ensure that the HTTP requests are actually canceled to reduce the amount of data being sent over the network.

jamesplease · 2017-12-07T16:14:39Z

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.

jimmywarting · 2017-12-07T18:27:51Z

fetch.js

@@ -459,6 +463,10 @@
        xhr.setRequestHeader(name, value)
      })

+      if (init.signal && init.signal.addEventListener) {
+        init.signal.addEventListener('abort', xhr.abort)


Should we remove the event listener also in the xhr.onabort function?

Yup. I'll do that.

jimmywarting · 2017-12-07T18:29:37Z

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.

jimmywarting · 2017-12-07T18:57:48Z

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

jamesplease · 2017-12-07T19:00:09Z

Good catch @jimmywarting ! I'll add that.

jimmywarting · 2017-12-07T19:03:05Z

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

jamesplease · 2017-12-07T19:07:42Z

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!

jamesplease · 2017-12-07T20:13:45Z

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

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

jamesplease · 2017-12-08T05:46:48Z

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?

mislav

This is heading in a good direction!

mislav · 2017-12-08T16:43:11Z

fetch.js

@@ -459,6 +469,10 @@
        xhr.setRequestHeader(name, value)
      })

+      if (request.signal && request.signal.addEventListener) {
+        request.signal.addEventListener('abort', xhr.abort)


Here, event object will be passed to xhr.abort() as an argument, which probably doesn't have side-effects, but to avoid it let's create a separate function for the event handler that calls into xhr.abort().

mislav · 2017-12-08T16:45:00Z

fetch.js

@@ -424,6 +425,10 @@
      var request = new Request(input, init)
      var xhr = new XMLHttpRequest()

+      if (request.signal && request.signal.aborted) {
+        return reject(new DOMException('Aborted', 'AbortError'));


Is this style of DOMException interface supported in all browsers that fetch aims to support?

I can do some investigating this weekend and get back to you on that.

mislav · 2017-12-08T16:46:38Z

fetch.js

@@ -459,6 +469,10 @@
        xhr.setRequestHeader(name, value)
      })

+      if (request.signal && request.signal.addEventListener) {


Can we assume that if request.signal isn't null, it will always define addEventListener?

Sure thing ✌️

mislav · 2017-12-08T16:49:32Z

README.md

+})
+  .then(
+    function(response) {
+      console.log('request succeeded', response)


Could we shorten the fetch invocation example to a minimum possible code that checks for aborted requests?

fetch('/avatars', { signal: controller.signal }).catch(function(error) { if (error.name === 'AbortError') { // request was aborted } })

mislav · 2017-12-08T16:53:19Z

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

Please:

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

jamesplease · 2017-12-08T17:27:41Z

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? :)

mislav · 2018-02-26T21:15:00Z

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.

mo · 2018-02-27T07:30:30Z

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?

mislav · 2018-02-27T09:06:20Z

@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.

mo · 2018-02-27T10:27:13Z

Sounds excellent.

dysonpro · 2018-03-02T00:10:26Z

@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!

mislav · 2018-03-06T14:08:47Z

@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.

joaovieira · 2018-03-06T16:28:21Z

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.

mislav · 2018-03-08T16:07:07Z

@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.

jayphelps · 2018-03-23T19:46:23Z

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

mislav · 2018-03-24T12:34:01Z

@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! ✨

jayphelps · 2018-03-24T18:23:17Z

I’m on it 👍

Edit: ready #616

phil-lgr · 2018-05-03T16:14:05Z

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

mislav · 2018-05-07T19:12:15Z

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

mislav · 2018-05-23T14:17:39Z

Thank you for your patience!

jackywq · 2018-05-24T06:00:01Z

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

jamesplease · 2018-05-24T06:05:08Z

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

Github/fetch has merged PR for UMD distribution: JakeChampion/fetch#616 Also abort support has been added: JakeChampion/fetch#592 We can use Github fetch instead of yetch now. But AbortableFetch is still not supported, see comment in test.

newmanw · 2018-08-10T14:33:17Z

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.

jamesplease force-pushed the abort-polyfill branch from e200748 to 8ea9da7 Compare December 7, 2017 09:22

jamesplease mentioned this pull request Dec 7, 2017

Added AbortController polyfill #572

Closed

Support abort API

93e9cec

jamesplease force-pushed the abort-polyfill branch from 8ea9da7 to 93e9cec Compare December 7, 2017 09:29

jamesplease mentioned this pull request Dec 7, 2017

Implement abort #547

Closed

jamesplease commented Dec 7, 2017

View reviewed changes

mislav reviewed Dec 7, 2017

View reviewed changes

Support abort without replacing native fetch

53565c1

jimmywarting reviewed Dec 7, 2017

View reviewed changes

Remove event listener; use request.signal

9a7d232

Skip creation of XHR if signal is already aborted

fd5c261

jamesplease force-pushed the abort-polyfill branch from 89620fb to fd5c261 Compare December 7, 2017 20:36

jimmywarting approved these changes Dec 7, 2017

View reviewed changes

jamesplease mentioned this pull request Dec 7, 2017

XMLHttpRequest is discouraged axios/axios#484

Closed

jamesplease force-pushed the abort-polyfill branch from a17b192 to 773c4b0 Compare December 8, 2017 05:44

First pass at documentation

318e95f

jamesplease force-pushed the abort-polyfill branch from 773c4b0 to 318e95f Compare December 8, 2017 05:44

mislav suggested changes Dec 8, 2017

View reviewed changes

More tweaks based on feedback

0e1b651

Remove some semicolons

d47f6c9

jayphelps mentioned this pull request Mar 29, 2018

build/distribute as UMD #616

Merged

mo mentioned this pull request Apr 26, 2018

AbortController does not seem to abort blob requests mo/abortcontroller-polyfill#21

Closed

This comment has been minimized.

Sign in to view

mislav added 4 commits May 23, 2018 15:53

Merge remote-tracking branch 'origin/master' into abort-polyfill

c31fca9

Scope abortcontroller to polyfill only

f035860

Export DOMException for instanceof checks

2bd7f22

Polish documentation for aborting requests

c873843

mislav merged commit da07fa1 into JakeChampion:master May 23, 2018

jamesplease deleted the abort-polyfill branch May 24, 2018 06:04

xg-wang mentioned this pull request Jul 21, 2018

Switch back to github/fetch pretenderjs/pretender#232

Closed

joaovieira mentioned this pull request Oct 2, 2018

Add support for AbortController (#54) developit/unfetch#68

Open

github-actions bot locked as resolved and limited conversation to collaborators Oct 1, 2020


		var abortController = new self.AbortController()

		var request = new self.Request('http://a', {

Support abort API #592

Support abort API #592

Conversation

jamesplease commented Dec 7, 2017 • edited

Choose a reason for hiding this comment

mislav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesplease commented Dec 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmywarting commented Dec 7, 2017 • edited

jimmywarting commented Dec 7, 2017

jamesplease commented Dec 7, 2017

jimmywarting commented Dec 7, 2017 • edited

jamesplease commented Dec 7, 2017 • edited

jamesplease commented Dec 7, 2017

jamesplease commented Dec 8, 2017

mislav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mislav commented Dec 8, 2017

jamesplease commented Dec 8, 2017 • edited

mislav commented Feb 26, 2018

mo commented Feb 27, 2018

mislav commented Feb 27, 2018 • edited

mo commented Feb 27, 2018

dysonpro commented Mar 2, 2018

mislav commented Mar 6, 2018

joaovieira commented Mar 6, 2018

mislav commented Mar 8, 2018

jayphelps commented Mar 23, 2018

mislav commented Mar 24, 2018

jayphelps commented Mar 24, 2018 • edited

phil-lgr commented May 3, 2018

This comment has been minimized.

mislav commented May 7, 2018

mislav commented May 23, 2018

jackywq commented May 24, 2018

jamesplease commented May 24, 2018

newmanw commented Aug 10, 2018

jamesplease commented Dec 7, 2017 •

edited

jamesplease commented Dec 7, 2017 •

edited

jimmywarting commented Dec 7, 2017 •

edited

jimmywarting commented Dec 7, 2017 •

edited

jamesplease commented Dec 7, 2017 •

edited

jamesplease commented Dec 8, 2017 •

edited

mislav commented Feb 27, 2018 •

edited

jayphelps commented Mar 24, 2018 •

edited