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

Status and headers not given to then/success #6

Closed
wants to merge 1 commit into from

Conversation

@mathbruyen
Copy link

commented Sep 16, 2014

The readme mentions that promises resolve with multiple arguments giving the status code and response headers in addition to data, but most promises library do not handle multiple arguments.

I added a spec to show it, but I think it requires an API change to work, thus I did not look at solutions yet.

Any advice there?

Mathieu Bruyen
@mzabriskie

This comment has been minimized.

Copy link
Member

commented Sep 16, 2014

@mathbruyen that's what I get for not having proper tests. Thanks for catching this for me. I will see what can be done.

@mathbruyen

This comment has been minimized.

Copy link
Author

commented Sep 16, 2014

The simplest would be resolve the promise with the whole response object build in adapters (xhr & http) but this breaks backward compatibility and is not nice for most use cases when one just needs data.

Another one is to resolve with an array and recommend the usage of spread but same arguments.

Don't know if it's a good idea but maybe having a config option which changes the resolve type, like:

axios({ url : '/foo' }).then(function (data) {
  /* ... */
}
axios({ url : '/foo', responseDetails : true }).then(function (response) {
  /* response.data|headers|status */
}

Backward compatibility is fine, and only users actually using headers/status will receive it.

@kentcdodds

This comment has been minimized.

Copy link

commented Sep 16, 2014

I know that HTTP an angular response with the full response object
On Sep 16, 2014 7:33 AM, "Mathieu Bruyen" notifications@github.com wrote:

The simplest would be resolve the promise with the whole response object
build in adapters (xhr
https://github.com/mzabriskie/axios/blob/f2fd9f7dd3a644ddbd25b4bfe59c86313b24a443/lib/adapters/xhr.js#L33-L42
& http
https://github.com/mzabriskie/axios/blob/f2fd9f7dd3a644ddbd25b4bfe59c86313b24a443/lib/adapters/http.js#L41-L50)
but this breaks backward compatibility and is not nice for most use cases
when one just needs data.

Another one is to resolve with an array and recommend the usage of spread
but same arguments.

Don't know if it's a good idea but maybe having a config option which
changes the resolve type, like:

axios({ url : '/foo' }).then(function (data) {
/* ... /
}
axios({ url : '/foo', responseDetails : true }).then(function (response) {
/
response.data|headers|status */
}

Backward compatibility is fine, and only users actually using
headers/status will receive it.


Reply to this email directly or view it on GitHub
#6 (comment).

@kentcdodds

This comment has been minimized.

Copy link

commented Sep 16, 2014

So, while it's an inconvenience to force people to deal with another object to get the actual data, for consistency's sake I think you should resolve with the data.

@mzabriskie

This comment has been minimized.

Copy link
Member

commented Sep 16, 2014

@mathbruyen thanks again for helping catch this bug. This has been resolved (1fa35ce) and the fix is available with version 0.3.0.

@mzabriskie mzabriskie closed this Sep 16, 2014

@mathbruyen mathbruyen deleted the mathbruyen:headers branch Sep 16, 2014

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