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

Make 'npm install --loglevel warn' Output Quieter #4335

Closed
wants to merge 1 commit into from

Conversation

DabeDotCom
Copy link

What / Why

I know this is kind of trivial, but I was trying to turn off the up to date in 420ms and 14 packages are looking for funding-type messages in my CI/CD pipeline, and was surprised/disappointed to discover that npm install --loglevel error wasn't good enough.

Having to reach for --silent seems a little too heavy-handed, IMHO... "changed", "audit", and "funding" messages aren't errors, per se — or even warnings, for that matter — they don't alert me to imminent breakage — and --silent is a foot-gun waiting to happen; I'm infinitely more likely to miss something important. But I understand wanting to keep them in the default output.

Describe the request in detail. What it does and why it's being changed.

This PR simply makes it so --loglevel warn is sufficient to turn them off, without impacting other, actual npm install errors...

Instead of:

if (log.levels[log.level] > log.levels.error)

it simply does:

if (log.levels[log.level] >= log.levels.warn)

[Note: That also meant setting the fixture from warn to notice in test/lib/utils/reify-output.js:beforeEach]

References

Related to #3311

`npm install --silent` seems *too* heavy-handed; changed, audit, and
funding messages aren't errors -- or even warnings, for that matter.
This makes it so `--loglevel warn` turns them off, but still allows
other errors to be displayed.
@DabeDotCom DabeDotCom requested a review from a team as a code owner January 27, 2022 01:10
@ruyadorno ruyadorno added Needs Discussion is pending a discussion Release 8.x work is associated with a specific npm 8 release labels Jan 27, 2022
@ruyadorno
Copy link
Collaborator

@DabeDotCom if you want to skip audit and disable the fund notifications on your install you can run npm install --no-audit --no-fund or add to your .npmrc file:

audit=false
fund=false

@DabeDotCom
Copy link
Author

@DabeDotCom if you want to skip audit and disable the fund notifications on your install you can run npm install --no-audit --no-fund or add to your .npmrc file

Thanks @ruyadorno!

I considered that, but I still couldn't figure out how to get rid of the up to date in 420ms "error"

@ruyadorno ruyadorno added the Agenda will be discussed at the Open RFC call label Jan 27, 2022
@darcyclarke darcyclarke added Needs Review and removed Agenda will be discussed at the Open RFC call Needs Discussion is pending a discussion labels Feb 3, 2022
@lukekarrys
Copy link
Member

Thanks for this PR @DabeDotCom (with tests too!). I agree with your problem statement that it's important to be able to control what npm writes to the terminal and that --silent is too heavy handed for disabling this particular output.

However I don't think this is the right lever to pull to get this behavior. The cli writes two things to the terminal: logs and output. It tries use stdout for output and stderr for logs (there a few inconsistencies here that I hope to correct, ref: #4724, #2740).

loglevel (as the name implies) controls what gets written to the logs and therefore stderr. But loglevel=silent is kind of magical in that it stops everything from bring written, stdout included. So I'm hesitant to make this change since it would make loglevel=error magical in the same way.

The good news is that since these write streams are already separated you can redirect stdout only. You can still combine that with the flags @ruyadorno suggested to skip audit and fund altogether too.

# hide all stdout output
npm install --loglevel=error > /dev/null
# or write to a file
npm install --loglevel=error > npm-output.log

I think this is the best solution for this particular problem. The future of npm logs/output is a current topic of discussion, so if you have more thoughts you can comment on the linked RFC or open a new one. Thanks!

@DabeDotCom
Copy link
Author

The cli writes two things to the terminal: logs and output. It tries to use stdout for output and stderr for logs

Hi @lukekarrys,

Thanks for taking the time to review my PR — and thanks also for your much-wiser-than-mine interpretation, understanding, and explanation of what I was trying to accomplish! :-D

They say "Hindsight is 20/20" and after reading your message, I just now realized that all this time, I'd been >&-ing stdout/stderr together into one stream! «embarrased»

In all honesty, though, fine tuning what messages are debug vs. verbose vs. info vs. quiet, etc., and figuring out how to customize exactly which ones you want to include and/or exclude at any given stage has got to be at LEAST as hard as Cache Invalidation and Naming Things... I respect the amount of rigor and thought you all have put into it. «hats off»

Cheers! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release 8.x work is associated with a specific npm 8 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants