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 responses in json #17

Closed
wants to merge 1 commit into from
Closed

Conversation

jstcki
Copy link
Contributor

@jstcki jstcki commented Mar 25, 2018

Responses with status 204 and 205 don't contain a body which causes response.json() to blow up (unlike response.text() et al).

I think the correct way to handle this is to return null.

Copy link

@curran curran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@mbostock
Copy link
Member

This would need to apply to d3.text and probably every other fetch helper too, but I haven’t thought through yet whether this is the right approach.

@jstcki
Copy link
Contributor Author

jstcki commented Mar 26, 2018

@mbostock I've checked and none of the other Response methods throw an exception but returns an empty object of the respective type.

👉 https://beta.observablehq.com/@herrstucki/204

@mbostock
Copy link
Member

The spec seems clear that throwing is the desired behavior here, which would make it your responsibility to handle this condition. Related: whatwg/fetch#113 JakeChampion/fetch#123 node-fetch/node-fetch#165

The downside is that you’d have to handle it at the Fetch layer rather than at the d3-fetch layer, since presumably you’d want to check the response status code rather than catching any SyntaxError. So that might be an argument to return undefined rather than throwing… (I think undefined is more appropriate than null here.)

@jstcki
Copy link
Contributor Author

jstcki commented Mar 26, 2018

Yeah, that was my thinking too … and since d3-fetch already is an abstraction (and checks for response.ok), I think this would be the right place to handle it.

@mbostock mbostock mentioned this pull request Mar 26, 2018
@mbostock
Copy link
Member

Superseded by #18.

@mbostock mbostock closed this Mar 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants