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
Closed

Status and headers not given to then/success #6

wants to merge 1 commit into from

Conversation

mathbruyen
Copy link

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?

@mzabriskie
Copy link
Member

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

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

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

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

@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 headers branch September 16, 2014 20:52
@axios axios locked and limited conversation to collaborators May 4, 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.

None yet

3 participants