Open
Conversation
Member
|
Hey! Thanks a lot for the PR. There might be a slight delay in processing it, but I'll get back to you as soon as possible and let you know if any changes are needed. And I really like the issue this is geared towards resolving. I know that the error handling / reporting is not top-notch currently, and it's great to have a contrib related to that. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
Currently, fetch response is going to
apiResponse.json()without checking whether the HTTP response is OK or not. The check is only performed inread.jswhich the code handles response differently whenapiResponse.okis false.The problem is: if there is a problem in the server, and it returns HTTP status 500 with non-JSON document type (such as nginx), the user (application code) will receive invalid JSON error, not HTTP 500 error. This makes it difficult to identify errors in production, and the error log is not useful.
The solution
In this PR, every fetch response is handled by
processFetchResponsefunction. This function checks whether the response is OK or not. When it is not OK, it decides what error message for the error object. Error message from Stein server will be the same like inread.jswhile other errors will be generic HTTP error message, such asHTTP 404 Not Found. Response object is included in the error object so users can access it (for example, in my case I need to read the page content because my web server includes helpful informations in the error page).And also, I move helpers to its own folder, which is
lib/to make the code more organized. Existing helper,argIsRequired.js, is moved there.Apologize if my PR is not following contributing guidelines. I could not find this project guideline.