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

Quiet down or silence non-errors in npm install #77

Closed
wants to merge 1 commit into from

Conversation

wesbos
Copy link
Contributor

@wesbos wesbos commented Dec 17, 2019

npm install output scares people

Summary

npm install shows too much low level information which is unhelpful to most developers.

Motivation

There is no way to tell if a npm install has worked or not. Many of my students are confused at the output as it looks like something has gone wrong when everything runs just fine.

Detailed Explanation

Just npm install something and look at the output. This was from 100% success:

Rationale and Alternatives

Some people have suggested --silent as a good solution, but that will hide good errors as well, no?

Implementation

If everything worked, then don't show a bunch of scary things on the screen.

Prior Art

yarn, pnpm

@darcyclarke
Copy link
Contributor

@wesbos Just as a quick information gathering, can you show the output of both pnpm & yarn against this same project?

@darcyclarke darcyclarke added Agenda will be discussed at the Open RFC call Enhancement new feature or improvement Needs Discussion is pending a discussion labels Dec 17, 2019
@wesbos
Copy link
Contributor Author

wesbos commented Dec 17, 2019

Seems like Yarn has even worse output in this case - same but not highlighted. pnpm is similar so apologies there.

I think a lot of this has to do with native modules puking on the screen - is there something that can be done to test if it worked or not and suppress these?

@darcyclarke
Copy link
Contributor

darcyclarke commented Dec 17, 2019

@wesbos you might try changing the loglevel; There's some options for you & shorthands here: https://docs.npmjs.com/misc/config#shorthands-and-other-cli-nicities (one of which includes --silent but that, as you noted/guessed, would hide errors)

Try npm install --quiet OR shorthand npm i -q (may/may not help in this case)

@wesbos
Copy link
Contributor Author

wesbos commented Dec 17, 2019

Yeah, the problem isn't that I don't know how to fix it, it's that I get hundreds of emails/tweets/dms/slacks from people who think it broke on install when in reality I have to say "No, no 400 lines of errors, warnings is completely normal".

Would some sort of better default be good here?

@darcyclarke
Copy link
Contributor

darcyclarke commented Dec 17, 2019

@wesbos totally. Might want to update that "Prior Art" section though...

@wesbos
Copy link
Contributor Author

wesbos commented Dec 17, 2019

:) Also once all the issues with native modules go away, this is still the output:

Screen Shot 2019-12-17 at 10 15 55 AM

Everything but the security is unhelpful to me.

Even with --silent, I still get some stuff:

Screen Shot 2019-12-17 at 10 18 31 AM

It should also say "It worked!" or "successful install" once it's all done, no?

@darcyclarke
Copy link
Contributor

darcyclarke commented Dec 17, 2019

@wesbos appreciate these screens/added context. Definitely helps round things out. I do like/want sane defaults so I think that we'll focus on that as we go forward.

Hoping we can bring this up/prioritize early in the New Year at our first Open RFC call in 2020; Would love to have you hop on, if you can make it, to discuss this & we'll get any work associated prioritized accordingly.

@isaacs
Copy link
Contributor

isaacs commented Dec 17, 2019

We are already planning on running install scripts in a background process and only printing the results if the installation meaningfully fails. (Ie, exits non-zero for a non-optional installation.)

We held back on doing that because it would make the current fundraising approaches impossible, which was a bit more opinionated than we wanted to get at this time. With the arrival of npm fund, I think we're unblocked and ready to move forward on it, though.

@wesbos
Copy link
Contributor Author

wesbos commented Dec 17, 2019

Great - looking forward to that

@isaacs
Copy link
Contributor

isaacs commented Dec 17, 2019

@rbeer It's not a legit install error, though, because it's an optional dependency. It's probably fine to just show a warning that an optional dep was omitted. And, since we do installations in parallel, it gets pretty gnarly if you have multiple builds going on simultaneously sharing the same stdio. We plan to push all lifecycle scripts to a stdio-piped process, and print stdout/stderr only if it's a failure that we care about (ie, non-zero exit code for non-optional dependency). This will also allow us to prevent interleaved output.

@wesleytodd
Copy link

Is there an rfc for this background behavior @isaacs? This might break more than just the fund use case. Not that I think it is bad, I would just like to read and maybe give feedback on it.

@isaacs
Copy link
Contributor

isaacs commented Dec 18, 2019

@wesleytodd Hm, I don't see one. It looks like it was added to the roadmap before I switched back to a technical role with the CLI, could've been before the RFC process even started. We can write one up, though.

@darcyclarke darcyclarke added Backlog a "backlogged" item that will be tracked in a Project Board Release 7.x and removed Agenda will be discussed at the Open RFC call labels Jan 22, 2020
@isaacs isaacs closed this in 2737ac7 Mar 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backlog a "backlogged" item that will be tracked in a Project Board Enhancement new feature or improvement Needs Discussion is pending a discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants