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

PP-682/review error handler #79

Merged
merged 5 commits into from
Mar 10, 2023
Merged

PP-682/review error handler #79

merged 5 commits into from
Mar 10, 2023

Conversation

ironFe93
Copy link
Contributor

What

check if an 'error' property is found

Why

In order to throw the correct error

Refs

  • (optional: include links to other issues, PRs, tickets, etc)

@ironFe93
Copy link
Contributor Author

I took the liberty to make a further analysis, here it goes:
There is no endpoint in the server that returns a proper status code when an error happens. All of them catch the error and then send either { message: 'some error' } or { error: 'some error' }

No status code is ever set to 400 or 500, so this catch will never (or rarely) trigger (and if we do get into this catch, we return the request??). This means that we have to explicitly check for an 'error' or 'message' property when handling the response and then throw the error.

We could avoid all of this by returning proper status codes in the server, standardizing the error object (either { error } or { message } ) then letting the request reject (we can catch the error, log it and then throw it).

For the sake of the task, I am checking for { error } or { message } in this PR, which will solve the issue, but I think we can do better.

Copy link
Collaborator

@antomor antomor left a comment

Choose a reason for hiding this comment

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

@ironFe93 Thank you for the in-depth analysis. I'm going to create a task to analyse if we can implement a proper error handler in the server also.
I've just left a small comment, but LGTM 👏🏽

@ironFe93 ironFe93 changed the title Pp 682/review error handler PP-682/review error handler Mar 10, 2023
@ironFe93 ironFe93 merged commit a11522b into main Mar 10, 2023
@ironFe93 ironFe93 deleted the PP-682/review-error-handler branch March 10, 2023 13:38
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