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

Do not print error banner for shell proxy commands #2742

Closed
wants to merge 1 commit into from

Conversation

isaacs
Copy link
Contributor

@isaacs isaacs commented Feb 20, 2021

There are a few commands (exec, run-script, and the run-script proxies)
where essentially npm is acting like a very fancy shell. It is peculiar
and noisy for npm to print a verbose error banner at the end of these
commands, since presumably the command itself already did whatever it
had to do to report the error appropriately.

For example, npm test runs a test script, usually outputting test
results. Having npm then tell me that my tests failed with exit status
1 and print a debug log, is unnecessary and unwanted.

When the error encountered for these commands does not have a non-zero
numeric 'code', then we still print the standard npm error reporting
messages, because presumably something went wrong OTHER than a process
exiting with a non-zero status code.

Not sure if this should be considered a breaking change or a bugfix or a feature. It is a change to our output, so anyone relying on that for test failures will be broken. However, it also removes a lot of clutter that is providing no value in the vast majority of uses of npm test, and allows you to use npm exec with commands that may communicate something interesting both in stderr and the exit status code. For example, a shell script that outputs JSON to stderr would not be usable with npm exec right now because of this extra debugging output.

References

There are a few commands (exec, run-script, and the run-script proxies)
where essentially npm is acting like a very fancy shell.  It is peculiar
and noisy for npm to print a verbose error banner at the end of these
commands, since presumably the command itself already did whatever it
had to do to report the error appropriately.

For example, `npm test` runs a test script, usually outputting test
results.  Having npm then tell me that my tests failed with exit status
1 and print a debug log, is unnecessary and unwanted.

When the error encountered for these commands does not have a non-zero
numeric 'code', then we still print the standard npm error reporting
messages, because presumably something went wrong OTHER than a process
exiting with a non-zero status code.
@isaacs isaacs requested a review from a team as a code owner February 20, 2021 04:50
@darcyclarke darcyclarke added Release 7.x work is associated with a specific npm 7 release release: next These items should be addressed in the next release semver:patch semver patch level for changes labels Feb 22, 2021
isaacs added a commit that referenced this pull request Feb 22, 2021
There are a few commands (exec, run-script, and the run-script proxies)
where essentially npm is acting like a very fancy shell.  It is peculiar
and noisy for npm to print a verbose error banner at the end of these
commands, since presumably the command itself already did whatever it
had to do to report the error appropriately.

For example, `npm test` runs a test script, usually outputting test
results.  Having npm then tell me that my tests failed with exit status
1 and print a debug log, is unnecessary and unwanted.

When the error encountered for these commands does not have a non-zero
numeric 'code', then we still print the standard npm error reporting
messages, because presumably something went wrong OTHER than a process
exiting with a non-zero status code.

PR-URL: #2742
Credit: @isaacs
Close: #2742
Reviewed-by: @nlf
@isaacs isaacs closed this Feb 22, 2021
@nlf nlf deleted the isaacs/quieter-exec branch March 28, 2022 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: next These items should be addressed in the next release Release 7.x work is associated with a specific npm 7 release semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants