Skip to content

Commit d5c3289

Browse files
lukekarryslukekarrys
authored
fix(refactor): use output buffer and error for more commands (#7513)
This changes a bunch of commands to use the new `output.buffer` capabilities from `proc-log` as well as the `outputError` helper from `utils/output-error.js`. This also adds a few comments about future display related breaking changes that npm should make. There is some new behavior around `run-script` and how it outputs errors. It now displays the error for each workspace similar to how that error would get displayed when the process exits.
1 parent 12f103c commit d5c3289

12 files changed

Lines changed: 342 additions & 375 deletions

File tree

‎lib/commands/ls.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -554,7 +554,7 @@ const jsonOutput = ({ path, problems, result, rootError, seenItems }) => {
554554
}
555555
}
556556

557-
return JSON.stringify(result, null, 2)
557+
return result
558558
}
559559

560560
const parseableOutput = ({ global, long, seenNodes }) => {

‎lib/commands/pack.js‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -53,18 +53,16 @@ class Pack extends BaseCommand {
5353
prefix: this.npm.localPrefix,
5454
workspaces: this.workspacePaths,
5555
})
56-
const pkgContents = await getContents(manifest, tarballData)
57-
tarballs.push(pkgContents)
56+
tarballs.push(await getContents(manifest, tarballData))
5857
}
5958

60-
if (json) {
61-
output.standard(JSON.stringify(tarballs, null, 2))
62-
return
63-
}
64-
65-
for (const tar of tarballs) {
66-
logTar(tar, { unicode })
67-
output.standard(tar.filename.replace(/^@/, '').replace(/\//, '-'))
59+
for (const [index, tar] of Object.entries(tarballs)) {
60+
// XXX(BREAKING_CHANGE): publish outputs a json object with package
61+
// names as keys. Pack should do the same here instead of an array
62+
logTar(tar, { unicode, json, key: index })
63+
if (!json) {
64+
output.standard(tar.filename.replace(/^@/, '').replace(/\//, '-'))
65+
}
6866
}
6967
}
7068

‎lib/commands/pkg.js‎

Lines changed: 29 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,7 @@ class Pkg extends BaseCommand {
2525
static workspaces = true
2626
static ignoreImplicitWorkspace = false
2727

28-
async exec (args, { prefix } = {}) {
29-
if (!prefix) {
30-
this.prefix = this.npm.localPrefix
31-
} else {
32-
this.prefix = prefix
33-
}
34-
28+
async exec (args, { path = this.npm.localPrefix, workspace } = {}) {
3529
if (this.npm.global) {
3630
throw Object.assign(
3731
new Error(`There's no package.json file to manage on global mode`),
@@ -42,57 +36,54 @@ class Pkg extends BaseCommand {
4236
const [cmd, ..._args] = args
4337
switch (cmd) {
4438
case 'get':
45-
return this.get(_args)
39+
return this.get(_args, { path, workspace })
4640
case 'set':
47-
return this.set(_args)
41+
return this.set(_args, { path, workspace }).then(p => p.save())
4842
case 'delete':
49-
return this.delete(_args)
43+
return this.delete(_args, { path, workspace }).then(p => p.save())
5044
case 'fix':
51-
return this.fix(_args)
45+
return PackageJson.fix(path).then(p => p.save())
5246
default:
5347
throw this.usageError()
5448
}
5549
}
5650

5751
async execWorkspaces (args) {
5852
await this.setWorkspaces()
59-
const result = {}
60-
for (const [workspaceName, workspacePath] of this.workspaces.entries()) {
61-
this.prefix = workspacePath
62-
result[workspaceName] = await this.exec(args, { prefix: workspacePath })
53+
for (const [workspace, path] of this.workspaces.entries()) {
54+
await this.exec(args, { path, workspace })
6355
}
64-
// when running in workspaces names, make sure to key by workspace
65-
// name the results of each value retrieved in each ws
66-
output.standard(JSON.stringify(result, null, 2))
6756
}
6857

69-
async get (args) {
70-
const pkgJson = await PackageJson.load(this.prefix)
71-
72-
const { content } = pkgJson
73-
let result = !args.length && content
58+
async get (args, { path, workspace }) {
59+
this.npm.config.set('json', true)
60+
const pkgJson = await PackageJson.load(path)
7461

75-
if (!result) {
76-
const q = new Queryable(content)
77-
result = q.query(args)
62+
let unwrap = false
63+
let result = pkgJson.content
7864

65+
if (args.length) {
66+
result = new Queryable(result).query(args)
7967
// in case there's only a single result from the query
8068
// just prints that one element to stdout
8169
if (Object.keys(result).length === 1) {
70+
unwrap = true
8271
result = result[args]
8372
}
8473
}
8574

86-
// only outputs if not running with workspaces config
87-
// execWorkspaces will handle the output otherwise
88-
if (!this.workspaces) {
89-
output.standard(JSON.stringify(result, null, 2))
75+
if (workspace) {
76+
// workspaces are always json
77+
output.buffer({ [workspace]: result })
78+
} else {
79+
// if the result was unwrapped, stringify as json which will add quotes around strings
80+
// TODO: https://github.com/npm/cli/issues/5508 a raw mode has been requested similar
81+
// to jq -r. If that was added then it would conditionally not call JSON.stringify here
82+
output.buffer(unwrap ? JSON.stringify(result) : result)
9083
}
91-
92-
return result
9384
}
9485

95-
async set (args) {
86+
async set (args, { path }) {
9687
const setError = () =>
9788
this.usageError('npm pkg set expects a key=value pair of args.')
9889

@@ -102,7 +93,7 @@ class Pkg extends BaseCommand {
10293

10394
const force = this.npm.config.get('force')
10495
const json = this.npm.config.get('json')
105-
const pkgJson = await PackageJson.load(this.prefix)
96+
const pkgJson = await PackageJson.load(path)
10697
const q = new Queryable(pkgJson.content)
10798
for (const arg of args) {
10899
const [key, ...rest] = arg.split('=')
@@ -114,19 +105,18 @@ class Pkg extends BaseCommand {
114105
q.set(key, json ? JSON.parse(value) : value, { force })
115106
}
116107

117-
pkgJson.update(q.toJSON())
118-
await pkgJson.save()
108+
return pkgJson.update(q.toJSON())
119109
}
120110

121-
async delete (args) {
111+
async delete (args, { path }) {
122112
const setError = () =>
123113
this.usageError('npm pkg delete expects key args.')
124114

125115
if (!args.length) {
126116
throw setError()
127117
}
128118

129-
const pkgJson = await PackageJson.load(this.prefix)
119+
const pkgJson = await PackageJson.load(path)
130120
const q = new Queryable(pkgJson.content)
131121
for (const key of args) {
132122
if (!key) {
@@ -136,13 +126,7 @@ class Pkg extends BaseCommand {
136126
q.delete(key)
137127
}
138128

139-
pkgJson.update(q.toJSON())
140-
await pkgJson.save()
141-
}
142-
143-
async fix () {
144-
const pkgJson = await PackageJson.fix(this.prefix)
145-
await pkgJson.save()
129+
return pkgJson.update(q.toJSON())
146130
}
147131
}
148132

‎lib/commands/publish.js‎

Lines changed: 35 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,26 @@ class Publish extends BaseCommand {
4343
throw this.usageError()
4444
}
4545

46+
await this.#publish(args)
47+
}
48+
49+
async execWorkspaces () {
50+
await this.setWorkspaces()
51+
52+
for (const [name, workspace] of this.workspaces.entries()) {
53+
try {
54+
await this.#publish([workspace], { workspace: name })
55+
} catch (err) {
56+
if (err.code !== 'EPRIVATE') {
57+
throw err
58+
}
59+
// eslint-disable-next-line max-len
60+
log.warn('publish', `Skipping workspace ${this.npm.chalk.cyan(name)}, marked as ${this.npm.chalk.bold('private')}`)
61+
}
62+
}
63+
}
64+
65+
async #publish (args, { workspace } = {}) {
4666
log.verbose('publish', replaceInfo(args))
4767

4868
const unicode = this.npm.config.get('unicode')
@@ -61,7 +81,7 @@ class Publish extends BaseCommand {
6181
// you can publish name@version, ./foo.tgz, etc.
6282
// even though the default is the 'file:.' cwd.
6383
const spec = npa(args[0])
64-
let manifest = await this.getManifest(spec, opts)
84+
let manifest = await this.#getManifest(spec, opts)
6585

6686
// only run scripts for directory type publishes
6787
if (spec.type === 'directory' && !ignoreScripts) {
@@ -84,15 +104,17 @@ class Publish extends BaseCommand {
84104
workspaces: this.workspacePaths,
85105
})
86106
const pkgContents = await getContents(manifest, tarballData)
107+
const logPkg = () => logTar(pkgContents, { unicode, json, key: workspace })
87108

88109
// The purpose of re-reading the manifest is in case it changed,
89110
// so that we send the latest and greatest thing to the registry
90111
// note that publishConfig might have changed as well!
91-
manifest = await this.getManifest(spec, opts, true)
112+
manifest = await this.#getManifest(spec, opts, true)
92113

93-
// JSON already has the package contents
114+
// If we are not in JSON mode then we show the user the contents of the tarball
115+
// before it is published so they can see it while their otp is pending
94116
if (!json) {
95-
logTar(pkgContents, { unicode })
117+
logPkg()
96118
}
97119

98120
const resolved = npa.resolve(manifest.name, manifest.version)
@@ -126,6 +148,12 @@ class Publish extends BaseCommand {
126148
await otplease(this.npm, opts, o => libpub(manifest, tarballData, o))
127149
}
128150

151+
// In json mode we dont log until the publish has completed as this will
152+
// add it to the output only if completes successfully
153+
if (json) {
154+
logPkg()
155+
}
156+
129157
if (spec.type === 'directory' && !ignoreScripts) {
130158
await runScript({
131159
event: 'publish',
@@ -142,62 +170,15 @@ class Publish extends BaseCommand {
142170
})
143171
}
144172

145-
if (!this.suppressOutput) {
146-
if (!silent && json) {
147-
output.standard(JSON.stringify(pkgContents, null, 2))
148-
} else if (!silent) {
149-
output.standard(`+ ${pkgContents.id}`)
150-
}
151-
}
152-
153-
return pkgContents
154-
}
155-
156-
async execWorkspaces () {
157-
// Suppresses JSON output in publish() so we can handle it here
158-
this.suppressOutput = true
159-
160-
const results = {}
161-
const json = this.npm.config.get('json')
162-
const { silent } = this.npm
163-
await this.setWorkspaces()
164-
165-
for (const [name, workspace] of this.workspaces.entries()) {
166-
let pkgContents
167-
try {
168-
pkgContents = await this.exec([workspace])
169-
} catch (err) {
170-
if (err.code === 'EPRIVATE') {
171-
log.warn(
172-
'publish',
173-
`Skipping workspace ${
174-
this.npm.chalk.cyan(name)
175-
}, marked as ${
176-
this.npm.chalk.bold('private')
177-
}`
178-
)
179-
continue
180-
}
181-
throw err
182-
}
183-
// This needs to be in-line w/ the rest of the output that non-JSON
184-
// publish generates
185-
if (!silent && !json) {
186-
output.standard(`+ ${pkgContents.id}`)
187-
} else {
188-
results[name] = pkgContents
189-
}
190-
}
191-
192-
if (!silent && json) {
193-
output.standard(JSON.stringify(results, null, 2))
173+
if (!json && !silent) {
174+
output.standard(`+ ${pkgContents.id}`)
194175
}
195176
}
196177

197178
// if it's a directory, read it from the file system
198179
// otherwise, get the full metadata from whatever it is
199180
// XXX can't pacote read the manifest from a directory?
200-
async getManifest (spec, opts, logWarnings = false) {
181+
async #getManifest (spec, opts, logWarnings = false) {
201182
let manifest
202183
if (spec.type === 'directory') {
203184
const changes = []

0 commit comments

Comments
 (0)