Closed confused-Techie closed 1 year ago
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.
Thanks a ton for the approval @DeeDeeG! I'll go ahead and merge this one
It was brought to my attention on Discord that a61aa7e121bc93723f675198778b6006fdd66738 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 actualerror
object returned to the callback would benull
even though those commands then relied on theerror
object to determine what had gone wrong.So what I've done, is provide both the
error
andbody
to therequest.getErrorMessage()
so that we can check for errors in either of them.Meaning when a request fails we will:
superagent
error object forerr.response.body
then forerr.response.error
then forerr
body.message
thenbody.error
then finallybody
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.