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

Throw custom errors from spawn #32

Merged
merged 7 commits into from
Jun 18, 2021
Merged

Throw custom errors from spawn #32

merged 7 commits into from
Jun 18, 2021

Conversation

lukekarrys
Copy link
Contributor

@lukekarrys lukekarrys commented Jun 15, 2021

Implements some custom error subclasses that get thrown by spawn when known or unknown git errors occur.

Then in packages like pacote we can do something like:

if (er instanceof git.errors.GitPathspecError) {
  // We know what happened so we can rethrow knowing we shouldn't try anything else
  // and that we'll get a more helpful error message
  throw er
}

Details

  • Each custom error class message is replaced with a helpful message (eg The git reference could not be found.) followed by the full error form @npmcli/promise-spawn available via er.stderr (which gets printed by the cli error handler)
  • The rest of the enumerable properties from @npmcli/promise-spawn are assigned to the custom error
  • Wrapped the previous retry logic into a new error subclass which might be useful to handle differently in other error handlers in the future

TODO

  • Finish implementing tests

@lukekarrys lukekarrys requested a review from nlf June 15, 2021 18:26
@lukekarrys lukekarrys marked this pull request as ready for review June 15, 2021 21:31
@lukekarrys lukekarrys requested review from a team and removed request for nlf June 16, 2021 00:01
@wraithgar
Copy link
Member

I like this. It solves two problems, the first is "how do we tell the user what actually went wrong in a clear way" and the other is "how do we allow pactote to programmatically know how to react to certain errors.

Copy link
Member

@wraithgar wraithgar left a comment

Choose a reason for hiding this comment

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

I think this is shippable

Copy link
Contributor

@nlf nlf left a comment

Choose a reason for hiding this comment

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

this looks great

@lukekarrys lukekarrys merged commit 766de2f into main Jun 18, 2021
@lukekarrys lukekarrys deleted the lk/custom-error branch June 18, 2021 16:17
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.

3 participants