Skip to content

Handle fetch error#6

Open
sumartoyo wants to merge 1 commit intoSteinHQ:masterfrom
sumartoyo:fix/handle-fetch-error
Open

Handle fetch error#6
sumartoyo wants to merge 1 commit intoSteinHQ:masterfrom
sumartoyo:fix/handle-fetch-error

Conversation

@sumartoyo
Copy link

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 in read.js which the code handles response differently when apiResponse.ok is 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 processFetchResponse function. 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 in read.js while other errors will be generic HTTP error message, such as HTTP 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.

@sumartoyo sumartoyo marked this pull request as draft January 15, 2021 14:03
@sumartoyo sumartoyo marked this pull request as ready for review January 15, 2021 14:03
@shivensinha4
Copy link
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.

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.

2 participants