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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Apr 29, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d9cf7fc
feat: add spinner
lukekarrys Apr 27, 2024
0ced0b7
fix: move run-script banners to stderr when in json mode
lukekarrys Apr 29, 2024
0662852
Merge branch 'latest' into lk/run-script-json-output
lukekarrys Apr 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 0 additions & 2 deletions DEPENDENCIES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,6 @@ graph LR;
npm-->pacote;
npm-->parse-conflict-json;
npm-->proc-log;
npm-->proggy;
npm-->read;
npm-->semver;
npm-->ssri;
Expand Down Expand Up @@ -527,7 +526,6 @@ graph LR;
npm-->pacote;
npm-->parse-conflict-json;
npm-->proc-log;
npm-->proggy;
npm-->qrcode-terminal;
npm-->read;
npm-->remark-gfm;
Expand Down
2 changes: 1 addition & 1 deletion docs/bin/build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

.catch((err) => {
process.exitCode = 1
console.error(err)
Expand Down
8 changes: 2 additions & 6 deletions lib/commands/init.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ const npa = require('npm-package-arg')
const libexec = require('libnpmexec')
const mapWorkspaces = require('@npmcli/map-workspaces')
const PackageJson = require('@npmcli/package-json')
const { log, output } = require('proc-log')
const { log, output, input } = require('proc-log')
const updateWorkspaces = require('../utils/update-workspaces.js')
const BaseCommand = require('../base-cmd.js')

Expand Down Expand Up @@ -148,8 +148,6 @@ class Init extends BaseCommand {
}

async template (path = process.cwd()) {
log.pause()

const initFile = this.npm.config.get('init-module')
if (!this.npm.config.get('yes') && !this.npm.config.get('force')) {
output.standard([
Expand All @@ -167,7 +165,7 @@ class Init extends BaseCommand {
}

try {
const data = await initJson(path, initFile, this.npm.config)
const data = await input.read(() => initJson(path, initFile, this.npm.config))
log.silly('package data', data)
return data
} catch (er) {
Expand All @@ -176,8 +174,6 @@ class Init extends BaseCommand {
} else {
throw er
}
} finally {
log.resume()
}
}

Expand Down