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

fix: move run-script banners to stderr when in json mode #7439

Merged
merged 3 commits into from Apr 29, 2024

Conversation

lukekarrys
Copy link
Contributor

Fixes #7354

@lukekarrys lukekarrys requested a review from a team as a code owner April 29, 2024 19:16
@@ -16,7 +16,7 @@ const run = require('../lib/build.js')
const { paths } = require('../lib/index')

run(paths)
.then((res) => console.log(`Wrote ${res.length} files`))
.then((res) => console.error(`Wrote ${res.length} files`))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was made so that our prepack script doesn't log anything to stdout, so that a smoke-test could be added that running node . pack --json only ends up with json parseable output on stdout.

This highlights an important note about this change, as it only redirects the banner from @npmcli/run-script to stderr when --json is set. The user's run-script is spawned with stdio: inherit so if that process sends output to stdout it could still break in json mode. This is not something npm should change since we want to allow spawned process to write to whatever stream they want.

If a user really wants to ensure that the script cannot output anything, they should also use --foreground-scripts=false or --silent.

Base automatically changed from lk/spinner to latest April 29, 2024 23:34
@lukekarrys lukekarrys merged commit bcc781a into latest Apr 29, 2024
38 checks passed
@lukekarrys lukekarrys deleted the lk/run-script-json-output branch April 29, 2024 23:42
@github-actions github-actions bot mentioned this pull request Apr 29, 2024
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.

[BUG] npm-pack --json emits prepare script commands, making json invalid
2 participants