Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support abort API #592

Merged
merged 27 commits into from
May 23, 2018
Merged

Support abort API #592

merged 27 commits into from
May 23, 2018

Conversation

jamesplease
Copy link
Contributor

@jamesplease jamesplease commented Dec 7, 2017

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 ✌️

fetch.js Outdated

var abortController = new self.AbortController()

var request = new self.Request('http://a', {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Can you pick one for this PR?

fetch.js Outdated
return Boolean(request.signal)
}

if (self.fetch && canAbortFetch()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

jamesplease commented Dec 7, 2017

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.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. I'll do that.

@jimmywarting
Copy link

jimmywarting commented Dec 7, 2017

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
Copy link

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
Copy link
Contributor Author

Good catch @jimmywarting ! I'll add that.

@jimmywarting
Copy link

jimmywarting commented Dec 7, 2017

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
Copy link
Contributor Author

jamesplease commented Dec 7, 2017

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

This is intentional. From this page:

image

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

@jamesplease
Copy link
Contributor Author

@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

@jamesplease
Copy link
Contributor Author

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?

Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is heading in a good direction!

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

if (request.signal && request.signal.addEventListener) {
request.signal.addEventListener('abort', xhr.abort)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

fetch.js Outdated
@@ -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'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

if (request.signal && request.signal.addEventListener) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing ✌️

README.md Outdated
})
.then(
function(response) {
console.log('request succeeded', response)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
  }
})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course!

@mislav
Copy link
Contributor

mislav commented Dec 8, 2017

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.

@jamesplease
Copy link
Contributor Author

jamesplease commented Dec 8, 2017

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
Copy link
Contributor

mislav commented Feb 26, 2018

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
Copy link

mo commented Feb 27, 2018

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
Copy link
Contributor

mislav commented Feb 27, 2018

@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
Copy link

mo commented Feb 27, 2018

Sounds excellent.

@dysonpro
Copy link

dysonpro commented Mar 2, 2018

@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
Copy link
Contributor

mislav commented Mar 6, 2018

@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
Copy link
Contributor

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
Copy link
Contributor

mislav commented Mar 8, 2018

@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
Copy link
Contributor

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

@mislav
Copy link
Contributor

mislav commented Mar 24, 2018

@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
Copy link
Contributor

jayphelps commented Mar 24, 2018

I’m on it 👍

Edit: ready #616

@phil-lgr
Copy link

phil-lgr commented May 3, 2018

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

@jayphelps

This comment has been minimized.

@mislav
Copy link
Contributor

mislav commented May 7, 2018

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

@mislav mislav merged commit da07fa1 into JakeChampion:master May 23, 2018
@mislav
Copy link
Contributor

mislav commented May 23, 2018

Thank you for your patience!

@jackywq
Copy link

jackywq commented May 24, 2018

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

@jamesplease jamesplease deleted the abort-polyfill branch May 24, 2018 06:04
@jamesplease
Copy link
Contributor Author

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

xg-wang pushed a commit to xg-wang/pretender that referenced this pull request May 24, 2018
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
Copy link

newmanw commented Aug 10, 2018

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement abort