Skip to content

Check for errors in NPM output and reject command#78

Open
shaunburdick wants to merge 1 commit intoAnifacted:masterfrom
shaunburdick:return_npm_errors
Open

Check for errors in NPM output and reject command#78
shaunburdick wants to merge 1 commit intoAnifacted:masterfrom
shaunburdick:return_npm_errors

Conversation

@shaunburdick
Copy link
Copy Markdown

Added some code to check for NPM errors when running a command and reject the promise. (see #49)
From the outside, lernaupdate seemed to be working happily, however there was a permission error in one of my packages that was resulting in an npm error. There was no way to tell that error was happening. I only noticed when I went to commit the updated package.json and noticed there were no changes.

Example of the output:
image
(had to redact the output a little for... reasons)

I think there are a few caveats:

  • This is NPM specific. I don't use Yarn so we would probably need to update the regex to support Yarn errors?
  • NPM outputs to stderr info that is not an error (warnings and such) so I made sure the output contained at least one npm ERR! pattern. This is probably ok but kind of fragile. I can't think of a better way to do it at this point.

@Anifacted
Copy link
Copy Markdown
Owner

@shaunburdick Thank you for your contribution! 🙏 You have indeed spotted a shortcoming that needs fixing. I am tempted to merge in your changes, but I wonder if there are any unforeseen side-effects related to rejecting this promise while installing multiple packages in multiple destinations. Ideally, the error would be isolated to the specific install and not blow up the whole session.

Another point regarding the implementation specifically, is that runCommand is intended to be a generic utility used across the library and it could be argued that having regex checks that are concerned with NPM error messages specifically is unideal.
Maybe runCommand should instead be hooked from the outside based on the use case to which it is being applied. That way we could also separate your concerns around NPM and Yarn based on the installer being used for each specific run respectively.

@shaunburdick
Copy link
Copy Markdown
Author

Yeah, I didn't feel great about this fix. I agree that maybe a better solution would be to update runCommand to return both stdout and stderr and we can check for platform related errors upstream where it makes more sense.

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