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

Fix: 204 No Content bug #47

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dzwiedziu-nkg
Copy link

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

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 😄

Copy link
Owner

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?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it works :)

Copy link
Owner

@devvmh devvmh left a 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?

@devvmh
Copy link
Owner

devvmh commented Apr 24, 2017

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

@tryangul
Copy link

tryangul commented Jun 28, 2017

How about something more like

   // fix of "Unexpected end of JSON input" when HTTP response is 204 No Content
    if (response.status === 204) {
      return {
        ...response,
        [format]: () => null,
      };
    }

This way we're not losing any methods

@devvmh
Copy link
Owner

devvmh commented Aug 7, 2017

@dzwiedziu-nkg have you gotten a chance to look at this PR again?

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.

3 participants