-
Notifications
You must be signed in to change notification settings - Fork 23
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
Fix: 204 No Content bug #47
base: master
Are you sure you want to change the base?
Conversation
Fix of "Unexpected end of JSON input" when HTTP response is 204 No Content
Fix of "Unexpected end of JSON input" when HTTP response is 204 No Content
src/ApiClient.js
Outdated
// fix of "Unexpected end of JSON input" when HTTP response is 204 No Content | ||
if (response.status === 204) { | ||
return { json: () => () => null } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, this is great, thanks!
This won't work for clients who aren't using the default json format. We need to generalize this so it handles all of the default formats (arrayBuffer, blob, formData, json, or text). I'd also need to look through https://developer.mozilla.org/en-US/docs/Web/API/Response to make sure we're not breaking any other fields by mocking the object like this.
I can do some thinking on this and push a change next week, but if you're able to get it done before then I'll happily merge it once it's ready 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming you have a good test env set up for this change. I think also that response.json() should return a string (or null), not another function, i.e.:
return { json: () => null }
Could you also do me a favour and see if this code works instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it works :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @dzwiedziu-nkg , this is great, but it still needs to be more closely reflecting an actual response object. This change could have unintended consequences to consumers of the library that are looking for keys like text() or formData() on the response.
I'm not sure what the best solution is. If I get the chance to do more thinking on it I'll post something up, but otherwise are you able to make this more closely match the existing api?
for example, someone might be manually checking the text() value of the response if the status is 204 (this is something I've done before). That would then fail if we pushed this change |
How about something more like
This way we're not losing any methods |
@dzwiedziu-nkg have you gotten a chance to look at this PR again? |
Fix of "Unexpected end of JSON input" when HTTP response is 204 No Content