-
Notifications
You must be signed in to change notification settings - Fork 6
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
Return an error for empty response body #100
Conversation
Check for response.body before checking for response.body.error or response.body.errors. Don't sniff the response for <DOCTYPE. Report 'unknown error' with status and status text if the response body is empty.
|
Trying this out with zooniverse/pandora, I now get shown the 500 error, when I save a tutorial, rather than |
Code change looks good, though I have questions:
...shouldn't this clause not have triggered if |
Wait, never mind what I said about Question 2 - I just read Issue #99 and see that the |
The translations API is broken at the moment, so saving a translation returns 500: Internal Server Error with no response body. In the current version of the client, the error handler crashes with |
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.
PR Review
This PR prevents errors from occurring in the error-catching mechanism of the API client. The client now properly reports when, e.g. a 500 error is received from the Panoptes server.
While I'm unable to test this properly on my apps (I can't get things to crash when I want to ) the code changes looks legit.
Also, please note the difference between response.body.error
and response.body.errors
; the later else if (Array.isArray(response.body.errors))
line threw me for a loop during my review.
Status
👍 LGTM
The nested if statements in that error handler still bug me, to be honest. |
Check for response.body before checking for response.body.error or response.body.errors.
Don't sniff the response for <DOCTYPE.
Report 'unknown error' with status and status text if the response body is empty.
Fixes a bug where the client crashes on 500 errors, because
response.body
is null.Closes #99.