Feature/abortable fetch

[akmjenkins]

Adds #3

Hi there maintainer! I've got an abortable fetch API for you.

The following is important to note:

In order to get the following pattern to work:

const req = frisbee.get('/',{abortToken:'some token'});
...
frisbee.abort('some token')
...
try {
  const res = await req;
}

the abort-y things cannot be registered asynchronously (i.e. after interceptors), which is why this change appears so drastic - it's really not. The majority of it simply moved the async function call in _setup down so I could do some sync stuff first.

Because of the change above, I also had to slightly change the (internal) behavior of registering interceptors and change a few test stubs in interceptor.test.js. Other than adding a dozen tests or so, no other tests were modified and they all still pass, yay!

Coles notes:

• It supports Abortable Fetches using AbortController and signal
• abortAll() - does exactly what it says - aborts any running or queued (await-ing in an interceptor*) requests
• An "enhanced" abort API that uses "tokens" - pass in an abortToken (any symbol) on your options and use it to cancel any running request(s) - yes, it can be more than one - that use that token. The ability to cancel more than one request is useful if you want to make a request in an interceptor, just use the same abortToken that's being passed in with the request options.

You've got a good library going here. Quite light weight (as it should), couple of nifty features ("automagic" jwt and auth). I hope abortable fetches, especially this new abort-by-token API will further people's interest in it.

Feedback very much welcome and appreciated. I'm not too big of an open-source contributor so I hope I'm doing it right.

[niftylettuce]

@akmjenkins thank you so much for your efforts here in improving this package and contributing to open source. I plan to review this tomorrow. If you don't hear from me by tomorrow feel free to ping me here or at niftylettuce@gmail.com. Thanks!!!! 👏

[akmjenkins]

@akmjenkins thank you so much for your efforts here in improving this package and contributing to open source. I plan to review this tomorrow. If you don't hear from me by tomorrow feel free to ping me here or at niftylettuce@gmail.com. Thanks!!!! 👏

I didn't feel like running prettier over everything and reformatting all your test and src files, but it fails the CI if I don't. You might want to consider a) removing the lint from the npm test or b) just linting and updating all your files so we can issue PRs that pass CI.

Like I said, I'm not a big open source person, so I might've messed something up, too.

[akmjenkins]

I'd also like to propose throwing an error (or maybe something less drastic, like just logging a warning?) if the user tries to abort anything other than a GET request. Aborting a non-GET request is usually a developer mistake.

EDIT: And by "tries to abort" I mean if they pass in an abortToken to anything other than frisbee.get. Also, prevent adding the signal from the frisbee.abortController to non-GET fetches. so non-GET fetches can't be aborted when calling frisbee.abortAll(). The former might be a good time to log a warning, the latter can be argued to be too heavy handed, but I'm still in favour because I've seen a lot of people shoot themselves in the foot because they thought that "cancelling" a post meaning the end-result of the post on the server wouldn't happen if they cancelled the request on the client.

EDIT 2 - Frisbee can still let developers pass in their own signals to non-GET requests so if a dev really knows what they're doing, they can cancel their non-GET, but Frisbee's abort() or abortAll() will only abort GETs

[niftylettuce]

Hmm, I really don't think this is very user-friendly. Curious what others think... the purpose of the API is to be incredibly simple and as close to fetch as possible.

[akmjenkins]

@niftylettuce - no problem, although I'm not really sure what you mean. Frisbee's existing API hasn't been changed at all.

As far as frisbee supporting abortable fetch's goes, right now, frisbee actually does "support" abortable fetches - by simply passing in a signal from an AbortController (which is a WHATWG API that has been developed with significant contention, to put it mildly), but it doesn't do anything to make it easier for the user to abort requests, which is what this PR was about (the token didn't do it a a whole lot, but it automated the creation and management of AbortControllers, plus the abortAll) method.

Anyway, there's no offence taken here, if you don't feel like it's right for frisbee that's totally cool. I did just want to clear up that the ugly abort API isn't actually mine, it's WHATWGs.

[davidgovea]

First of all @akmjenkins -- I want to say that I like the design approach taken here: complies with the standard, and adds some convenience on top. Looks like the abortToken is similar to axios' cancelToken, and the abortAll is comfortable API.

However, one thing I'm concerned about is that this isn't true cancellation: the abortcontroller-polyfill says it's a:

stub that calls catch, doesn't actually abort request

This, plus the limitations on cancelling GET requests only, and the observation that you made about frisbee already supporting AbortController signals, makes me think that maybe this shouldn't go into frisbee.

Cancellation & concurrency is harddd -- I've been using generator-based solutions, because generators can actually be cancelled, unlike async functions or promise chains. ember-concurrency is my favorite. I'm getting used to redux-saga on the react side, and I've also done a bit of work with posterus, which is also pretty fantastic. However, it's clear that there is a lot of responsibility still left to the developer, with or without this change.

Even things like timeouts might be best handled outside of frisbee.

---

Still going to do some thinking here, but wanted to jot these thoughts down

[akmjenkins]

First of all on my side, no offence will be taken if you shut this PR down :)

Secondly, the "true cancellation" argument is kind of out of scope of this PR. If you've chosen to use fetch (and frisbee) then you're not going to get "true cancellation" as the way the spec is currently written doesn't support it (this is another great read on, again, how contentious the fetch cancellation spec was to settle on). The polyfill only comes into play when the native AbortController is absent anyway, so if you're in a "good" environment that has fetch and AbortController natively, you're still not going to get "true cancellation".

On to cancelling XHRs. The spec for abort() (in the article mentioned above) wasn't written precisely so vendors took liberties in how they implemented it. Some destroyed the actual connection, others left the connection open but just didn't callback when it returned.

The POST issue also exists in XHR as well. Once a request gets to the server, you can cancel it until the cows come home on the client, it's still going to "do the work" it was meant to do on the server (update the database, write sessions, etc), regardless of when the client cancels it.

So if you're not a fan of the implementation of the PR, that's absolutely fine. Let's shut this one down and rewrite it or, just decide to not include it in frisbee for now. But discussing cancellation not working correctly is, like I said, outside the scope.

Finally, AbortController is gross to work with (much like fetch - hence frisbee). If people are going to write wrappers to simplify fetch, they're going to write wrappers to simplify creation and management of AbortControllers.

I do appreciate you taking your time to review the PR!

[davidgovea]

You bring up some great points. Thanks for the link - I haven't kept up on these topics.

I'm in favor of this PR. I think it's quite user-friendly, considering the sophistication of cancellation in general. It's faithful to the standards, enables AbortController use on all platforms, and provides some nice convenience on top.

My last consideration is making sure everything works on react-native, since that's one of the main selling points of frisbee over other libraries.

[niftylettuce]

@akmjenkins @davidgovea @tb9-Mason I've merged this PR, fixed all tests, and cleaned up the entire codebase. Please try out v2.0.10 just released to NPM! 🎉 🎉 🎉

npm install frisbee@latest

yarn add frisbee@latest