-
Notifications
You must be signed in to change notification settings - Fork 285
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
More correct http statuses in the README.md #106
base: master
Are you sure you want to change the base?
Conversation
@@ -35,10 +35,13 @@ require('isomorphic-fetch'); | |||
|
|||
fetch('//offline-news-api.herokuapp.com/stories') | |||
.then(function(response) { | |||
if (response.status >= 400) { | |||
if (response.status >= 200 || response.status < 400) { | |||
return response.json(); |
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.
Not all requests with code >=200 and <400 will have valid JSON body.
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.
as well >=400
did not pass this requirement
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.
if (response.status >= 400) { | ||
if (response.status >= 200 || response.status < 400) { | ||
return response.json(); | ||
|
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.
Is this empty line is unnecessary?
I mean I guess it is true that the status codes between 200 and 400 would be ‘good’ … but if you are in control of your backend you would know what valid statuses it would reply with and in this case it would only ever reply with 200 if it's healthy… (In some ways it's safer to treat unexpected statuses as errors on the front end, even if they're technically valid HTTP status codes because you may want to have different behaviour) In any case if you wanted to have such a broad check for “ok” statuses I think I think I will close this without change but if you disagree please let me know. |
No description provided.