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

Fix Error Handling #99

Merged
merged 1 commit into from
Sep 25, 2023
Merged

Fix Error Handling #99

merged 1 commit into from
Sep 25, 2023

Conversation

confused-Techie
Copy link
Member

It was brought to my attention on Discord that a61aa7e broke error handling in PPM.

The clue to find out what had gone wrong was the fact that the unstar command still worked.

This is because in that PR I had told superagent a list of status codes to not error on. This means that when we got, say a 404, the actual error object returned to the callback would be null even though those commands then relied on the error object to determine what had gone wrong.

So what I've done, is provide both the error and body to the request.getErrorMessage() so that we can check for errors in either of them.

Meaning when a request fails we will:

  1. Look first for within the superagent error object for err.response.body then for err.response.error then for err
  2. If that fails, we can then look at the body for body.message then body.error then finally body

This helpfully means we prioritize any errors returned by superagent then any errors returned by the backend. And if all else fails, we just tell the user whatever message was returned to us by the backend, which can be helpful in case the backend's error schema ever changes by accident or on purpose and isn't updated here. So that way we can always inform the user exactly what the backend is thinking.

@confused-Techie
Copy link
Member Author

We are seeing more failures of NodeJS 18 here, like we are elsewhere. So those shouldn't be considered during a final review of the PR.

Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

Appears to work as intended with the ppm star command, which was the command reported as having not-working error handling on the Discord thread.


Current master branch code:

% ./bin/ppm star does-not-exist
⭐  Starring does-not-exist ✗
Starring package failed: null

(Error message comes back as just null)


This branch:

% ./bin/ppm star does-not-exist
⭐  Starring does-not-exist ✗
Starring package failed: Not Found

(Error message is a more useful "Not Found".)

@confused-Techie
Copy link
Member Author

Thanks a ton for the approval @DeeDeeG! I'll go ahead and merge this one

@confused-Techie confused-Techie merged commit beb33c6 into master Sep 25, 2023
8 of 11 checks passed
@confused-Techie confused-Techie deleted the fix-error-handling branch September 25, 2023 23:27
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.

None yet

2 participants