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

Handle empty response (204) #78

Merged
merged 1 commit into from
Jun 30, 2015

Conversation

connyay
Copy link

@connyay connyay commented Jun 26, 2015

No description provided.

@floatdrop
Copy link
Contributor

Technically - blank response (at least with 200 status code) is invalid response.

http://stackoverflow.com/questions/11970962/valid-json-in-response

@connyay
Copy link
Author

connyay commented Jun 27, 2015

The situation I hit this with was on a 204 response. I started this PR only
checking if the status code was 204, but figured an empty 2xx would be
possible as well.

Thoughts?
On Fri, Jun 26, 2015 at 4:27 PM Vsevolod Strukchinsky <
[email protected]> wrote:

Technically - blank response (at least with 200 status code) is invalid
response.

http://stackoverflow.com/questions/11970962/valid-json-in-response


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

@floatdrop
Copy link
Contributor

@connyay I think it is expected behaviour never less empty string is invalid JSON. request is behaving something like this.

@sindresorhus what do you think?

@sindresorhus
Copy link
Owner

I'm not sure tbh. Empty response on 200 is invalid, so we shouldn't do anything there. 204 is valid with blank response, so we should maybe handle that somehow (suggestions?). Maybe we should only try to parse JSON when content-type is "application/json"? I assume servers don't send that on 204?.

@floatdrop
Copy link
Contributor

@sindresorhus a lot of endpoints do not send application/json content-type in json responses. This was main point of strict json option - to parse body in any case.

@connyay could you do a 204 check instead?

Related: ladjs/superagent#255

@connyay
Copy link
Author

connyay commented Jun 30, 2015

@floatdrop switched to check for 204

@connyay connyay changed the title Handle empty json response Handle empty response (204) Jun 30, 2015
@floatdrop
Copy link
Contributor

@connyay cool! Can you squash it into one commit?

@connyay connyay force-pushed the handle-empty-json-response branch from d30d74a to 3864556 Compare June 30, 2015 19:03
@connyay connyay force-pushed the handle-empty-json-response branch from 3864556 to 312c80d Compare June 30, 2015 19:03
@connyay
Copy link
Author

connyay commented Jun 30, 2015

@floatdrop should be good.

🍻

floatdrop added a commit that referenced this pull request Jun 30, 2015
@floatdrop floatdrop merged commit 2a155a8 into sindresorhus:master Jun 30, 2015
@floatdrop floatdrop added this to the 4.0.0 milestone Jun 30, 2015
@floatdrop
Copy link
Contributor

🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants