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

Fixing #211 #307

Merged
merged 1 commit into from Jun 1, 2016

Conversation

@dublx
Copy link
Contributor

commented Apr 22, 2016

Fixing #211

@dublx

This comment has been minimized.

Copy link
Owner Author

commented on a04077a Apr 22, 2016

Fixes axios#211

(response.status >= 200 && response.status < 300 ?
var minCode = 200;
var maxCode = 300;
if (response.config.hasOwnProperty('maxRedirects')) {

This comment has been minimized.

Copy link
@mzabriskie

mzabriskie Apr 22, 2016

Member

Why change the maxCode? Anything in the 300 range should be followed, unless maxRedirects occurs, in which case I assume that would be considered an error. Thoughts?

This comment has been minimized.

Copy link
@nickuraltsev

nickuraltsev Apr 22, 2016

Member

I agree with @mzabriskie . On a side note, I'm going to submit a PR that adds support for a config option that allows specifying status codes for which an error should be thrown.

This comment has been minimized.

Copy link
@nickuraltsev
@nickuraltsev

This comment has been minimized.

Copy link
Member

commented Apr 22, 2016

@dublx Could you please update README.md?

@dublx

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2016

Hi, I'm not too experienced with PR's.. do I update README and then submit new one?
Anyway, reason I change max code was because then the promise would be rejected.
I will change it then: check if maxRedirects === 0, the idea being that there should be no redirects, but also no promise rejection. Agree?

@nickuraltsev

This comment has been minimized.

Copy link
Member

commented Apr 22, 2016

You can just push a new commit to the same branch. It will be automatically added to your PR.

As for max code, my suggestion is to not change the status code error range at all. If you don't want axios to reject the promise for 3XX responses, you will be able to do that via validateStatus config option when #308 is merged.

@nickuraltsev

This comment has been minimized.

Copy link
Member

commented Apr 26, 2016

@dublx Please let me know if you need any help

@cspray

This comment has been minimized.

Copy link

commented May 11, 2016

@nickuraltsev Hi, I've really enjoyed using axios in the chatbot I'm creating for my company. I need this PR to support some needed functionality and currently plan to fork my own copy until this is available. If it is ok with @dublx I would be comfortable with cleaning this branch up to not change the status code error range, update the README and fix any conflicts. Alternatively I could simply create a new PR.

In short, I'd like to see the work I'm about to do be put back into axios to share with others and so I don't have to maintain my own fork. Please let me know if I can be assistance.

@nickuraltsev

This comment has been minimized.

Copy link
Member

commented May 11, 2016

@cspray Yes, I think the right way to deal with this is to merge @dublx commits into your own branch, make necessary changes, and submit a new PR. If you could do that, it would be great. Thank you!

@cspray

This comment has been minimized.

Copy link

commented May 11, 2016

@nickuraltsev Will do!

@brbsk

This comment has been minimized.

Copy link

commented May 23, 2016

Hi guys @dublx @nickuraltsev @cspray @mzabriskie, is there a chance that this PR would be merged soon? I'm really looking forward to it :) Can I help with it somehow?
Cheers :)

@nickuraltsev nickuraltsev merged commit a04077a into axios:master Jun 1, 2016

1 check passed

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

This comment has been minimized.

Copy link
Member

commented Jun 1, 2016

Merged

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.