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

Adding support for request cancellation #452

Merged
merged 10 commits into from Oct 10, 2016

Conversation

@nickuraltsev
Copy link
Member

commented Sep 18, 2016

See the issue #333. The API is based on the cancelable promises proposal.

@coveralls

This comment has been minimized.

Copy link

commented Sep 18, 2016

Coverage Status

Coverage increased (+0.4%) to 94.313% when pulling 5efca1e on nickuraltsev:cancel into b8f6f50 on mzabriskie:master.

@rubennorte
Copy link
Member

left a comment

Very good job Nick. I have a couple of comments but I like the implementation.

README.md Outdated
You can also create a cancel token using the `CancelToken.source` factory:

```js
var CancelToken = axios.CancelToken;

This comment has been minimized.

Copy link
@rubennorte

rubennorte Sep 20, 2016

Member

I think I'll end up using this version more frequently, because I find it clearer than the previous one (it's basically a deferred object). I'd put it before the CancelToken constructor version so people will find it more approachable.

Maybe it's just a personal preference and not everybody will see it that way.

This comment has been minimized.

Copy link
@nickuraltsev

nickuraltsev Sep 21, 2016

Author Member

@rubennorte Yeah, I agree. I've just updated README.md.

README.md Outdated
cancel = c;
})
}).catch(function(thrown) {
if (thrown instanceof Cancel) {

This comment has been minimized.

Copy link
@rubennorte

rubennorte Sep 20, 2016

Member

I'd avoid instanceof because it may cause issues working with different versions of axios in the same application (a Cancel instance from a version will not be an instanceof the Cancel constructor from the other version).

I'd define an isCancellation property in Cancel instances to do these checks or any other equivalent if it is defined in the spec.

}).catch(function(thrown) {
 if (thrown.isCancellation) {

This comment has been minimized.

Copy link
@nickuraltsev

nickuraltsev Sep 21, 2016

Author Member

@rubennorte It makes sense, but my goal was to implement Cancel and CancelToken as close to the cancelable promises spec (in its current form) as possible. It's tempting to add isCancellation (or Cancel.isCancel), but it can make future migration path to built-in Cancel and CancelToken harder. Thoughts?

This comment has been minimized.

Copy link
@rubennorte

rubennorte Sep 21, 2016

Member

@nickuraltsev I understand. I wouldn't be a problem with the native objects (but even those may suffer this instanceof problem under certain conditions) but will be with all the polyfills. Perhaps we can define axios.isCancellation(error) that work with both axios-defined classes and native classes if they exist (axios.isTimeout(error) would also be helpful). That way we ease the migration and also protect them from these problems.

This comment has been minimized.

Copy link
@nickuraltsev

nickuraltsev Sep 21, 2016

Author Member

@rubennorte I think adding axios.isCancel(value) is the way to go. We can easily update it to support native Cancel objects (when they are available). I'll update the PR soon.

This comment has been minimized.

Copy link
@nickuraltsev

nickuraltsev Sep 22, 2016

Author Member

@rubennorte Added axios.isCancel, please take a look

* @param {string=} message The message.
*/
function Cancel(message) {
this.message = message;

This comment has been minimized.

Copy link
@rubennorte

rubennorte Sep 20, 2016

Member

As I said in a previous comment, I'd do this.isCancellation = true; here

@coveralls

This comment has been minimized.

Copy link

commented Sep 21, 2016

Coverage Status

Coverage increased (+0.4%) to 94.313% when pulling 920769d on nickuraltsev:cancel into b8f6f50 on mzabriskie:master.

@coveralls

This comment has been minimized.

Copy link

commented Sep 22, 2016

Coverage Status

Coverage increased (+0.4%) to 94.366% when pulling 216e2a6 on nickuraltsev:cancel into b8f6f50 on mzabriskie:master.

@coveralls

This comment has been minimized.

Copy link

commented Sep 23, 2016

Coverage Status

Coverage increased (+0.4%) to 94.393% when pulling 8f30490 on nickuraltsev:cancel into b718ebf on mzabriskie:master.

@rubennorte
Copy link
Member

left a comment

Everything OK for me. Nice work!

* @class
* @param {string=} message The message.
*/
function Cancel(message) {

This comment has been minimized.

Copy link
@mzabriskie

mzabriskie Oct 6, 2016

Member

Any reason why Cancel isn't an instance of Error?

This comment has been minimized.

Copy link
@nickuraltsev

nickuraltsev Oct 6, 2016

Author Member

Cancel must not derive from Error according the cancelable promises spec. Please see this for details.

@mzabriskie

This comment has been minimized.

Copy link
Member

commented Oct 6, 2016

@nickuraltsev overall this looks really great! I am wondering what the migration path looks like at the point that cancelable promises are implemented natively.

@nickuraltsev

This comment has been minimized.

Copy link
Member Author

commented Oct 6, 2016

@mzabriskie I hope users will not have to change anything at all (unless the cancelable promises spec changes).

We will need to update axios though:

  • Export native 'Cancel' and 'CancelToken' if they are available.
  • Update isCancel so that it can detect native Cancel objects.
@coveralls

This comment has been minimized.

Copy link

commented Oct 6, 2016

Coverage Status

Coverage increased (+0.4%) to 94.393% when pulling 8f30490 on nickuraltsev:cancel into b718ebf on mzabriskie:master.

@nickuraltsev

This comment has been minimized.

Copy link
Member Author

commented Oct 8, 2016

@mzabriskie mzabriskie merged commit 4882ce5 into axios:master Oct 10, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@radziksh

This comment has been minimized.

Copy link

commented Oct 11, 2016

@nickuraltsev

This comment has been minimized.

Copy link
Member Author

commented Oct 11, 2016

@radziksh I've just updated the answer. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.