Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit fcb1c26

Browse files
lukekarryslukekarrys
committedMay 2, 2024··
feat: create exit handler class
1 parent 4ab6cf4 commit fcb1c26

File tree

14 files changed

+655
-451
lines changed

14 files changed

+655
-451
lines changed
 

‎lib/base-cmd.js

+1-28
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
const { log, output } = require('proc-log')
1+
const { log } = require('proc-log')
22

33
class BaseCommand {
44
static workspaces = false
@@ -111,33 +111,6 @@ class BaseCommand {
111111
})
112112
}
113113

114-
async cmdExec (args) {
115-
const { config } = this.npm
116-
117-
if (config.get('usage')) {
118-
return output.standard(this.usage)
119-
}
120-
121-
const hasWsConfig = config.get('workspaces') || config.get('workspace').length
122-
// if cwd is a workspace, the default is set to [that workspace]
123-
const implicitWs = config.get('workspace', 'default').length
124-
125-
// (-ws || -w foo) && (cwd is not a workspace || command is not ignoring implicit workspaces)
126-
if (hasWsConfig && (!implicitWs || !this.constructor.ignoreImplicitWorkspace)) {
127-
if (this.npm.global) {
128-
throw new Error('Workspaces not supported for global packages')
129-
}
130-
if (!this.constructor.workspaces) {
131-
throw Object.assign(new Error('This command does not support workspaces.'), {
132-
code: 'ENOWORKSPACES',
133-
})
134-
}
135-
return this.execWorkspaces(args)
136-
}
137-
138-
return this.exec(args)
139-
}
140-
141114
// Compare the number of entries with what was expected
142115
checkExpected (entries) {
143116
if (!this.npm.config.isDefault('expect-results')) {

‎lib/cli/entry.js

+7-15
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,8 @@ module.exports = async (process, validateEngines) => {
1515
require('graceful-fs').gracefulify(require('fs'))
1616

1717
const satisfies = require('semver/functions/satisfies')
18-
const exitHandler = require('./exit-handler.js')
18+
const ExitHandler = require('./exit-handler.js')
19+
const exitHandler = new ExitHandler({ process })
1920
const Npm = require('../npm.js')
2021
const npm = new Npm()
2122
exitHandler.setNpm(npm)
@@ -28,8 +29,7 @@ module.exports = async (process, validateEngines) => {
2829

2930
// At this point we've required a few files and can be pretty sure we dont contain invalid syntax for this version of node. It's possible a lazy require would, but that's unlikely enough that it's not worth catching anymore and we attach the more important exit handlers.
3031
validateEngines.off()
31-
process.on('uncaughtException', exitHandler)
32-
process.on('unhandledRejection', exitHandler)
32+
exitHandler.registerUncaughtHandlers()
3333

3434
// It is now safe to log a warning if they are using a version of node that is not going to fail on syntax errors but is still unsupported and untested and might not work reliably. This is safe to use the logger now which we want since this will show up in the error log too.
3535
if (!satisfies(validateEngines.node, validateEngines.engines)) {
@@ -42,13 +42,13 @@ module.exports = async (process, validateEngines) => {
4242
const { exec, command, args } = await npm.load()
4343

4444
if (!exec) {
45-
return exitHandler()
45+
return exitHandler.exit()
4646
}
4747

4848
if (!command) {
4949
output.standard(npm.usage)
5050
process.exitCode = 1
51-
return exitHandler()
51+
return exitHandler.exit()
5252
}
5353

5454
// Options are prefixed by a hyphen-minus (-, \u2d).
@@ -72,16 +72,8 @@ module.exports = async (process, validateEngines) => {
7272
updateNotifier(npm).then((msg) => (npm.updateNotification = msg))
7373

7474
await execPromise
75-
return exitHandler()
75+
return exitHandler.exit()
7676
} catch (err) {
77-
if (err.code === 'EUNKNOWNCOMMAND') {
78-
const didYouMean = require('../utils/did-you-mean.js')
79-
const suggestions = await didYouMean(npm.localPrefix, err.command)
80-
output.standard(`Unknown command: "${err.command}"${suggestions}\n`)
81-
output.standard('To see a list of supported npm commands, run:\n npm help')
82-
process.exitCode = 1
83-
return exitHandler()
84-
}
85-
return exitHandler(err)
77+
return exitHandler.exit(err)
8678
}
8779
}

‎lib/cli/exit-handler.js

+138-125
Original file line numberDiff line numberDiff line change
@@ -1,160 +1,173 @@
11
const { log, output, META } = require('proc-log')
2-
const errorMessage = require('../utils/error-message.js')
3-
const { redactLog: replaceInfo } = require('@npmcli/redact')
2+
const { errorMessage, getExitCodeFromError } = require('../utils/error-message.js')
43

5-
let npm = null // set by the cli
6-
let exitHandlerCalled = false
7-
let showLogFileError = false
4+
class ExitHandler {
5+
#npm = null
6+
#process = null
7+
#exited = false
8+
#exitErrorMessage = false
89

9-
process.on('exit', code => {
10-
const hasLoadedNpm = npm?.config.loaded
10+
#noNpmError = false
1111

12-
if (!code) {
13-
log.info('ok')
14-
} else {
15-
log.verbose('code', code)
12+
get #hasNpm () {
13+
return !!this.#npm
1614
}
1715

18-
if (!exitHandlerCalled) {
19-
process.exitCode = code || 1
20-
log.error('', 'Exit handler never called!')
21-
log.error('', 'This is an error with npm itself. Please report this error at:')
22-
log.error('', ' <https://github.com/npm/cli/issues>')
23-
// eslint-disable-next-line no-console
24-
console.error('')
25-
showLogFileError = true
16+
get #loaded () {
17+
return !!this.#npm?.loaded
2618
}
2719

28-
// npm must be loaded to know where the log file was written
29-
if (hasLoadedNpm) {
30-
npm.finish({ showLogFileError })
31-
// This removes any listeners npm setup, mostly for tests to avoid max listener warnings
32-
npm.unload()
20+
get #showExitErrorMessage () {
21+
if (!this.#loaded) {
22+
return false
23+
}
24+
if (!this.#exited) {
25+
return true
26+
}
27+
return this.#exitErrorMessage
3328
}
3429

35-
// these are needed for the tests to have a clean slate in each test case
36-
exitHandlerCalled = false
37-
showLogFileError = false
38-
})
30+
get #notLoadedOrExited () {
31+
return !this.#loaded && !this.#exited
32+
}
3933

40-
const exitHandler = err => {
41-
exitHandlerCalled = true
34+
setNpm (npm) {
35+
this.#npm = npm
36+
}
4237

43-
const hasLoadedNpm = npm?.config.loaded
38+
constructor ({ process }) {
39+
this.#process = process
40+
this.#process.on('exit', this.#handleProcesExitAndReset)
41+
}
4442

45-
if (!npm) {
46-
err = err || new Error('Exit prior to setting npm in exit handler')
47-
// Don't use proc-log here since npm was never set
48-
// eslint-disable-next-line no-console
49-
console.error(err.stack || err.message)
50-
return process.exit(1)
43+
registerUncaughtHandlers () {
44+
this.#process.on('uncaughtException', this.#handleExit)
45+
this.#process.on('unhandledRejection', this.#handleExit)
5146
}
5247

53-
if (!hasLoadedNpm) {
54-
err = err || new Error('Exit prior to config file resolving.')
55-
// Don't use proc-log here since npm was never loaded
56-
// eslint-disable-next-line no-console
57-
console.error(err.stack || err.message)
48+
exit (err) {
49+
this.#handleExit(err)
5850
}
5951

60-
// only show the notification if it finished.
61-
if (typeof npm.updateNotification === 'string') {
62-
log.notice('', npm.updateNotification, { [META]: true, force: true })
52+
#handleProcesExitAndReset = (code) => {
53+
this.#handleProcessExit(code)
54+
55+
// Reset all the state. This is only relevant for tests since
56+
// in reality the process fully exits here.
57+
this.#process.off('exit', this.#handleProcesExitAndReset)
58+
this.#process.off('uncaughtException', this.#handleExit)
59+
this.#process.off('unhandledRejection', this.#handleExit)
60+
if (this.#loaded) {
61+
this.#npm.unload()
62+
}
63+
this.#npm = null
64+
this.#exited = false
65+
this.#exitErrorMessage = false
6366
}
6467

65-
let exitCode = process.exitCode || 0
66-
let noLogMessage = exitCode !== 0
67-
let jsonError
68-
69-
if (err) {
70-
exitCode = 1
71-
// if we got a command that just shells out to something else, then it
72-
// will presumably print its own errors and exit with a proper status
73-
// code if there's a problem. If we got an error with a code=0, then...
74-
// something else went wrong along the way, so maybe an npm problem?
75-
const isShellout = npm.isShellout
76-
const quietShellout = isShellout && typeof err.code === 'number' && err.code
77-
if (quietShellout) {
78-
exitCode = err.code
79-
noLogMessage = true
80-
} else if (typeof err === 'string') {
81-
// XXX: we should stop throwing strings
82-
log.error('', err)
83-
noLogMessage = true
84-
} else if (!(err instanceof Error)) {
85-
log.error('weird error', err)
86-
noLogMessage = true
87-
} else {
88-
const os = require('node:os')
89-
const fs = require('node:fs')
90-
if (!err.code) {
91-
const matchErrorCode = err.message.match(/^(?:Error: )?(E[A-Z]+)/)
92-
err.code = matchErrorCode && matchErrorCode[1]
93-
}
68+
#handleProcessExit (code) {
69+
// Force exit code to a number if it has not been set
70+
const exitCode = typeof code === 'number' ? code : (this.#exited ? 0 : 1)
71+
this.#process.exitCode = exitCode
9472

95-
for (const k of ['type', 'stack', 'statusCode', 'pkgid']) {
96-
const v = err[k]
97-
if (v) {
98-
log.verbose(k, replaceInfo(v))
99-
}
100-
}
73+
if (this.#notLoadedOrExited) {
74+
// Exit handler was not called and npm was not loaded so we have to log something
75+
this.#logConsoleError(new Error(`Process exited unexpectedly with code: ${exitCode}`))
76+
return
77+
}
10178

102-
log.verbose('cwd', process.cwd())
103-
log.verbose('', os.type() + ' ' + os.release())
104-
log.verbose('node', process.version)
105-
log.verbose('npm ', 'v' + npm.version)
79+
if (this.#logNoNpmError()) {
80+
return
81+
}
10682

107-
for (const k of ['code', 'syscall', 'file', 'path', 'dest', 'errno']) {
108-
const v = err[k]
109-
if (v) {
110-
log.error(k, v)
111-
}
112-
}
83+
const os = require('node:os')
84+
log.verbose('cwd', this.#process.cwd())
85+
log.verbose('os', `${os.type()} ${os.release()}`)
86+
log.verbose('node', this.#process.version)
87+
log.verbose('npm ', `v${this.#npm.version}`)
11388

114-
const { summary, detail, json, files = [] } = errorMessage(err, npm)
115-
jsonError = json
116-
117-
for (let [file, content] of files) {
118-
file = `${npm.logPath}${file}`
119-
content = `'Log files:\n${npm.logFiles.join('\n')}\n\n${content.trim()}\n`
120-
try {
121-
fs.writeFileSync(file, content)
122-
detail.push(['', `\n\nFor a full report see:\n${file}`])
123-
} catch (logFileErr) {
124-
log.warn('', `Could not write error message to ${file} due to ${logFileErr}`)
125-
}
126-
}
89+
// only show the notification if it finished
90+
if (typeof this.#npm.updateNotification === 'string') {
91+
log.notice('', this.#npm.updateNotification, { [META]: true, force: true })
92+
}
12793

128-
for (const errline of [...summary, ...detail]) {
129-
log.error(...errline)
94+
if (!this.#exited) {
95+
log.error('', 'Exit handler never called!')
96+
log.error('', 'This is an error with npm itself. Please report this error at:')
97+
log.error('', ' <https://github.com/npm/cli/issues>')
98+
if (this.#npm.silent) {
99+
output.error('')
130100
}
101+
}
131102

132-
if (typeof err.errno === 'number') {
133-
exitCode = err.errno
134-
} else if (typeof err.code === 'number') {
135-
exitCode = err.code
136-
}
103+
log.verbose('exit', exitCode)
104+
105+
if (exitCode) {
106+
log.verbose('code', exitCode)
107+
} else {
108+
log.info('ok')
109+
}
110+
111+
if (this.#showExitErrorMessage) {
112+
log.error('', this.#npm.exitErrorMessage())
137113
}
138114
}
139115

140-
if (hasLoadedNpm) {
141-
output.flush({ [META]: true, jsonError })
116+
#logConsoleError (err) {
117+
// Run our error message formatters on all errors even if we
118+
// have no npm or an unloaded npm. This will clean the error
119+
// and possible return a formatted message about EACCESS or something.
120+
const { summary, detail } = errorMessage(err, this.#npm)
121+
const formatted = [...new Set([...summary, ...detail].flat().filter(Boolean))].join('\n')
122+
// If we didn't get anything from the formatted message then just display the full stack
123+
// eslint-disable-next-line no-console
124+
console.error(formatted === err.message ? err.stack : formatted)
142125
}
143126

144-
log.verbose('exit', exitCode || 0)
127+
#logNoNpmError (err) {
128+
if (this.#hasNpm) {
129+
return false
130+
}
131+
// Make sure we only log this error once
132+
if (!this.#noNpmError) {
133+
this.#noNpmError = true
134+
this.#logConsoleError(
135+
new Error(`Exit prior to setting npm in exit handler`, err ? { cause: err } : {})
136+
)
137+
}
138+
return true
139+
}
140+
141+
#handleExit = (err) => {
142+
this.#exited = true
143+
144+
// No npm at all
145+
if (this.#logNoNpmError(err)) {
146+
return this.#process.exit(this.#process.exitCode || getExitCodeFromError(err) || 1)
147+
}
145148

146-
showLogFileError = (hasLoadedNpm && npm.silent) || noLogMessage
147-
? false
148-
: !!exitCode
149+
// npm was never loaded but we still might have a config loading error or
150+
// something similar that we can run through the error message formatter
151+
// to give the user a clue as to what happened.s
152+
if (!this.#loaded) {
153+
this.#logConsoleError(new Error('Exit prior to config file resolving', { cause: err }))
154+
return this.#process.exit(this.#process.exitCode || getExitCodeFromError(err) || 1)
155+
}
156+
157+
this.#exitErrorMessage = err?.suppressError === true ? false : !!err
149158

150-
// explicitly call process.exit now so we don't hang on things like the
151-
// update notifier, also flush stdout/err beforehand because process.exit doesn't
152-
// wait for that to happen.
153-
let flushed = 0
154-
const flush = [process.stderr, process.stdout]
155-
const exit = () => ++flushed === flush.length && process.exit(exitCode)
156-
flush.forEach((f) => f.write('', exit))
159+
// Prefer the exit code of the error, then the current process exit code,
160+
// then set it to 1 if we still have an error. Otherwise we call process.exit
161+
// with undefined so that it can determine the final exit code
162+
const exitCode = err?.exitCode ?? this.#process.exitCode ?? (err ? 1 : undefined)
163+
164+
// explicitly call process.exit now so we don't hang on things like the
165+
// update notifier, also flush stdout/err beforehand because process.exit doesn't
166+
// wait for that to happen.
167+
this.#process.stderr.write('', () => this.#process.stdout.write('', () => {
168+
this.#process.exit(exitCode)
169+
}))
170+
}
157171
}
158172

159-
module.exports = exitHandler
160-
module.exports.setNpm = n => (npm = n)
173+
module.exports = ExitHandler

‎lib/npm.js

+211-71
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ const usage = require('./utils/npm-usage.js')
77
const LogFile = require('./utils/log-file.js')
88
const Timers = require('./utils/timers.js')
99
const Display = require('./utils/display.js')
10-
const { log, time, output } = require('proc-log')
10+
const { log, time, output, META } = require('proc-log')
1111
const { redactLog: replaceInfo } = require('@npmcli/redact')
1212
const pkg = require('../package.json')
1313
const { deref } = require('./utils/cmd-list.js')
@@ -73,76 +73,14 @@ class Npm {
7373
})
7474
}
7575

76-
get version () {
77-
return this.constructor.version
78-
}
79-
80-
// Call an npm command
81-
async exec (cmd, args = this.argv) {
82-
const Command = Npm.cmd(cmd)
83-
const command = new Command(this)
84-
85-
// since 'test', 'start', 'stop', etc. commands re-enter this function
86-
// to call the run-script command, we need to only set it one time.
87-
if (!this.#command) {
88-
this.#command = command
89-
process.env.npm_command = this.command
90-
}
91-
92-
return time.start(`command:${cmd}`, () => command.cmdExec(args))
93-
}
94-
9576
async load () {
96-
return time.start('npm:load', () => this.#load())
97-
}
98-
99-
get loaded () {
100-
return this.config.loaded
101-
}
102-
103-
// This gets called at the end of the exit handler and
104-
// during any tests to cleanup all of our listeners
105-
// Everything in here should be synchronous
106-
unload () {
107-
this.#timers.off()
108-
this.#display.off()
109-
this.#logFile.off()
110-
}
111-
112-
finish ({ showLogFileError } = {}) {
113-
this.#timers.finish({
114-
id: this.#runId,
115-
command: this.#argvClean,
116-
logfiles: this.logFiles,
117-
version: this.version,
118-
})
119-
120-
if (showLogFileError) {
121-
if (!this.silent) {
122-
// just a line break if not in silent mode
123-
output.error('')
124-
}
125-
126-
if (this.logFiles.length) {
127-
return log.error('', `A complete log of this run can be found in: ${this.logFiles}`)
128-
}
129-
130-
const logsMax = this.config.get('logs-max')
131-
if (logsMax <= 0) {
132-
// user specified no log file
133-
log.error('', `Log files were not written due to the config logs-max=${logsMax}`)
134-
} else {
135-
// could be an error writing to the directory
136-
log.error('',
137-
`Log files were not written due to an error writing to the directory: ${this.#logsDir}` +
138-
'\nYou can rerun the command with `--loglevel=verbose` to see the logs in your terminal'
139-
)
140-
}
77+
let err
78+
try {
79+
return await time.start('npm:load', () => this.#load())
80+
} catch (e) {
81+
err = e
14182
}
142-
}
143-
144-
get title () {
145-
return this.#title
83+
return this.#handleError(err)
14684
}
14785

14886
async #load () {
@@ -260,8 +198,210 @@ class Npm {
260198
return { exec: true, command: commandArg, args: this.argv }
261199
}
262200

263-
get isShellout () {
264-
return this.#command?.constructor?.isShellout
201+
async exec (cmd, args = this.argv) {
202+
if (!this.#command) {
203+
let err
204+
try {
205+
await this.#exec(cmd, args)
206+
} catch (e) {
207+
err = e
208+
}
209+
return this.#handleError(err)
210+
} else {
211+
return this.#exec(cmd, args)
212+
}
213+
}
214+
215+
// Call an npm command
216+
async #exec (cmd, args) {
217+
const Command = this.constructor.cmd(cmd)
218+
const command = new Command(this)
219+
220+
// since 'test', 'start', 'stop', etc. commands re-enter this function
221+
// to call the run-script command, we need to only set it one time.
222+
if (!this.#command) {
223+
this.#command = command
224+
process.env.npm_command = this.command
225+
}
226+
227+
if (this.config.get('usage')) {
228+
return output.standard(command.usage)
229+
}
230+
231+
let execWorkspaces = false
232+
const hasWsConfig = this.config.get('workspaces') || this.config.get('workspace').length
233+
// if cwd is a workspace, the default is set to [that workspace]
234+
const implicitWs = this.config.get('workspace', 'default').length
235+
// (-ws || -w foo) && (cwd is not a workspace || command is not ignoring implicit workspaces)
236+
if (hasWsConfig && (!implicitWs || !Command.ignoreImplicitWorkspace)) {
237+
if (this.global) {
238+
throw new Error('Workspaces not supported for global packages')
239+
}
240+
if (!Command.workspaces) {
241+
throw Object.assign(new Error('This command does not support workspaces.'), {
242+
code: 'ENOWORKSPACES',
243+
})
244+
}
245+
execWorkspaces = true
246+
}
247+
248+
return time.start(`command:${cmd}`, () =>
249+
execWorkspaces ? command.execWorkspaces(args) : command.exec(args))
250+
}
251+
252+
// This gets called at the end of the exit handler and
253+
// during any tests to cleanup all of our listeners
254+
// Everything in here should be synchronous
255+
unload () {
256+
this.#timers.off()
257+
this.#display.off()
258+
this.#logFile.off()
259+
}
260+
261+
finish () {
262+
// Finish all our timer work, this will write the file if requested, end timers, etc
263+
this.#timers.finish({
264+
id: this.#runId,
265+
command: this.#argvClean,
266+
logfiles: this.logFiles,
267+
version: this.version,
268+
})
269+
}
270+
271+
exitErrorMessage () {
272+
if (this.logFiles.length) {
273+
return `A complete log of this run can be found in: ${this.logFiles}`
274+
}
275+
276+
const logsMax = this.config.get('logs-max')
277+
if (logsMax <= 0) {
278+
// user specified no log file
279+
return `Log files were not written due to the config logs-max=${logsMax}`
280+
}
281+
282+
// could be an error writing to the directory
283+
return `Log files were not written due to an error writing to the directory: ${this.#logsDir}` +
284+
'\nYou can rerun the command with `--loglevel=verbose` to see the logs in your terminal'
285+
}
286+
287+
async #handleError (err) {
288+
if (err) {
289+
Object.assign(err, await this.#getError(err))
290+
}
291+
292+
// TODO: make this not need to be public
293+
this.finish()
294+
295+
output.flush({
296+
[META]: true,
297+
jsonError: err && this.loaded && this.config.get('json') ? {
298+
code: err.code,
299+
summary: (err.summary || []).map(l => l.slice(1).join(' ')).join('\n'),
300+
detail: (err.detail || []).map(l => l.slice(1).join(' ')).join('\n'),
301+
} : null,
302+
})
303+
304+
if (err) {
305+
throw err
306+
}
307+
}
308+
309+
async #getError (err) {
310+
const { errorMessage, getExitCodeFromError } = require('./utils/error-message.js')
311+
312+
// if we got a command that just shells out to something else, then it
313+
// will presumably print its own errors and exit with a proper status
314+
// code if there's a problem. If we got an error with a code=0, then...
315+
// something else went wrong along the way, so maybe an npm problem?
316+
if (this.#command?.constructor?.isShellout && typeof err.code === 'number' && err.code) {
317+
return {
318+
exitCode: err.code,
319+
suppressError: true,
320+
}
321+
}
322+
323+
// XXX: we should stop throwing strings
324+
if (typeof err === 'string') {
325+
log.error('', err)
326+
return {
327+
exitCode: 1,
328+
suppressError: true,
329+
}
330+
}
331+
332+
// XXX: we should stop throwing other non-errors
333+
if (!(err instanceof Error)) {
334+
log.error('weird error', err)
335+
return {
336+
exitCode: 1,
337+
suppressError: true,
338+
}
339+
}
340+
341+
if (err.code === 'EUNKNOWNCOMMAND') {
342+
const didYouMean = require('./utils/did-you-mean.js')
343+
const suggestions = await didYouMean(this.localPrefix, err.command)
344+
output.standard(`Unknown command: "${err.command}"${suggestions}\n`)
345+
output.standard('To see a list of supported npm commands, run:\n npm help')
346+
return {
347+
exitCode: 1,
348+
suppressError: true,
349+
}
350+
}
351+
352+
err.code ??= err.message.match(/^(?:Error: )?(E[A-Z]+)/)?.[1]
353+
354+
for (const k of ['type', 'stack', 'statusCode', 'pkgid']) {
355+
const v = err[k]
356+
if (v) {
357+
log.verbose(k, replaceInfo(v))
358+
}
359+
}
360+
361+
const exitCode = getExitCodeFromError(err) || 1
362+
const { summary, detail, files } = errorMessage(err, this)
363+
364+
const { writeFileSync } = require('node:fs')
365+
for (const [file, content] of files) {
366+
const filePath = `${this.logPath}${file}`
367+
const fileContent = `'Log files:\n${this.logFiles.join('\n')}\n\n${content.trim()}\n`
368+
try {
369+
writeFileSync(filePath, fileContent)
370+
detail.push(['', `\n\nFor a full report see:\n${filePath}`])
371+
} catch (fileErr) {
372+
log.warn('', `Could not write error message to ${file} due to ${fileErr}`)
373+
}
374+
}
375+
376+
for (const k of ['code', 'syscall', 'file', 'path', 'dest', 'errno']) {
377+
const v = err[k]
378+
if (v) {
379+
log.error(k, v)
380+
}
381+
}
382+
383+
for (const errline of [...summary, ...detail]) {
384+
log.error(...errline)
385+
}
386+
387+
return {
388+
exitCode,
389+
summary,
390+
detail,
391+
suppressError: false,
392+
}
393+
}
394+
395+
get title () {
396+
return this.#title
397+
}
398+
399+
get loaded () {
400+
return this.config.loaded
401+
}
402+
403+
get version () {
404+
return this.constructor.version
265405
}
266406

267407
get command () {

‎lib/utils/error-message.js

+18-24
Original file line numberDiff line numberDiff line change
@@ -3,18 +3,6 @@ const { resolve } = require('node:path')
33
const { redactLog: replaceInfo } = require('@npmcli/redact')
44
const { log } = require('proc-log')
55

6-
const messageText = msg => msg.map(line => line.slice(1).join(' ')).join('\n')
7-
8-
const jsonError = (er, npm, { summary, detail }) => {
9-
if (npm?.config.loaded && npm.config.get('json')) {
10-
return {
11-
code: er.code,
12-
summary: messageText(summary),
13-
detail: messageText(detail),
14-
}
15-
}
16-
}
17-
186
const errorMessage = (er, npm) => {
197
const short = []
208
const detail = []
@@ -67,13 +55,9 @@ const errorMessage = (er, npm) => {
6755
case 'EACCES':
6856
case 'EPERM': {
6957
const isCachePath =
70-
typeof er.path === 'string' &&
71-
npm.config.loaded &&
72-
er.path.startsWith(npm.config.get('cache'))
58+
typeof er.path === 'string' && npm.loaded && er.path.startsWith(npm.config.get('cache'))
7359
const isCacheDest =
74-
typeof er.dest === 'string' &&
75-
npm.config.loaded &&
76-
er.dest.startsWith(npm.config.get('cache'))
60+
typeof er.dest === 'string' && npm.loaded && er.dest.startsWith(npm.config.get('cache'))
7761

7862
if (process.platform !== 'win32' && (isCachePath || isCacheDest)) {
7963
// user probably doesn't need this, but still add it to the debug log
@@ -407,25 +391,35 @@ const errorMessage = (er, npm) => {
407391

408392
default:
409393
short.push(['', er.message || er])
394+
if (er.cause) {
395+
detail.push(['cause', er.cause.message])
396+
}
410397
if (er.signal) {
411398
detail.push(['signal', er.signal])
412399
}
413-
414400
if (er.cmd && Array.isArray(er.args)) {
415401
detail.push(['command', ...[er.cmd, ...er.args.map(replaceInfo)]])
416402
}
417-
418403
if (er.stdout) {
419404
detail.push(['', er.stdout.trim()])
420405
}
421-
422406
if (er.stderr) {
423407
detail.push(['', er.stderr.trim()])
424408
}
425-
426409
break
427410
}
428-
return { summary: short, detail, files, json: jsonError(er, npm, { summary: short, detail }) }
411+
return { summary: short, detail, files }
412+
}
413+
414+
const getExitCodeFromError = (err) => {
415+
if (typeof err?.errno === 'number') {
416+
return err.errno
417+
} else if (typeof err?.code === 'number') {
418+
return err.code
419+
}
429420
}
430421

431-
module.exports = errorMessage
422+
module.exports = {
423+
getExitCodeFromError,
424+
errorMessage,
425+
}

‎smoke-tests/tap-snapshots/test/index.js.test.cjs

-1
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,6 @@ npm error
6666
npm error aliases: clean-install, ic, install-clean, isntall-clean
6767
npm error
6868
npm error Run "npm help ci" for more info
69-
7069
npm error A complete log of this run can be found in: {NPM}/{TESTDIR}/cache/_logs/{LOG}
7170
`
7271

‎smoke-tests/test/npm-replace-global.js

+5-1
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,11 @@ t.test('fail when updating with lazy require', async t => {
191191
// so an uncached lazy require within the exit handler will always throw
192192
await fs.writeFile(
193193
join(paths.globalNodeModules, 'npm/lib/cli/exit-handler.js'),
194-
`module.exports = () => require('./LAZY_REQUIRE_CANARY');module.exports.setNpm = () => {}`,
194+
`module.exports = class {
195+
setNpm(){}
196+
registerUncaughtHandlers(){}
197+
exit() { require('./LAZY_REQUIRE_CANARY') }
198+
}`,
195199
'utf-8'
196200
)
197201

‎tap-snapshots/test/lib/cli/exit-handler.js.test.cjs

+12-12
Original file line numberDiff line numberDiff line change
@@ -20,18 +20,18 @@ XX timing npm:load:setTitle Completed in {TIME}ms
2020
XX verbose logfile logs-max:10 dir:{CWD}/cache/_logs/{DATE}-
2121
XX verbose logfile {CWD}/cache/_logs/{DATE}-debug-0.log
2222
XX timing npm:load Completed in {TIME}ms
23+
XX timing command:root Completed in {TIME}ms
2324
XX verbose stack Error: Unknown error
25+
XX error code ECODE
26+
XX error Unknown error
27+
XX timing npm Completed in {TIME}ms
28+
XX info timing Timing info written to: {CWD}/cache/_logs/{DATE}-timing.json
2429
XX verbose cwd {CWD}/prefix
25-
XX verbose {OS}
30+
XX verbose os {OS}
2631
XX verbose {NODE-VERSION}
2732
XX verbose npm {NPM-VERSION}
28-
XX error code ECODE
29-
XX error ERR SUMMARY Unknown error
30-
XX error ERR DETAIL Unknown error
3133
XX verbose exit 1
3234
XX verbose code 1
33-
XX timing npm Completed in {TIME}ms
34-
XX info timing Timing info written to: {CWD}/cache/_logs/{DATE}-timing.json
3535
XX error A complete log of this run can be found in: {CWD}/cache/_logs/{DATE}-debug-0.log
3636
`
3737

@@ -50,17 +50,17 @@ timing npm:load:setTitle Completed in {TIME}ms
5050
verbose logfile logs-max:10 dir:{CWD}/cache/_logs/{DATE}-
5151
verbose logfile {CWD}/cache/_logs/{DATE}-debug-0.log
5252
timing npm:load Completed in {TIME}ms
53+
timing command:root Completed in {TIME}ms
5354
verbose stack Error: Unknown error
55+
error code ECODE
56+
error Unknown error
57+
timing npm Completed in {TIME}ms
58+
info timing Timing info written to: {CWD}/cache/_logs/{DATE}-timing.json
5459
verbose cwd {CWD}/prefix
55-
verbose {OS}
60+
verbose os {OS}
5661
verbose {NODE-VERSION}
5762
verbose npm {NPM-VERSION}
58-
error code ECODE
59-
error ERR SUMMARY Unknown error
60-
error ERR DETAIL Unknown error
6163
verbose exit 1
6264
verbose code 1
63-
timing npm Completed in {TIME}ms
64-
info timing Timing info written to: {CWD}/cache/_logs/{DATE}-timing.json
6565
error A complete log of this run can be found in: {CWD}/cache/_logs/{DATE}-debug-0.log
6666
`

‎tap-snapshots/test/lib/commands/doctor.js.test.cjs

+43-14
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,9 @@ Object {
121121

122122
exports[`test/lib/commands/doctor.js TAP bad proxy > logs 1`] = `
123123
Object {
124-
"error": Array [],
124+
"error": Array [
125+
"Some problems found. See above for recommendations.",
126+
],
125127
"info": Array [
126128
"doctor Running checkup",
127129
"doctor Pinging registry",
@@ -400,7 +402,9 @@ exports[`test/lib/commands/doctor.js TAP discrete checks git > output 1`] = `
400402

401403
exports[`test/lib/commands/doctor.js TAP discrete checks invalid environment > logs 1`] = `
402404
Object {
403-
"error": Array [],
405+
"error": Array [
406+
"Some problems found. See above for recommendations.",
407+
],
404408
"info": Array [
405409
"doctor Running checkup",
406410
"doctor Finding git in your PATH",
@@ -514,7 +518,9 @@ current: v1.0.0, recommended: v1.0.0
514518

515519
exports[`test/lib/commands/doctor.js TAP error reading directory > logs 1`] = `
516520
Object {
517-
"error": Array [],
521+
"error": Array [
522+
"Some problems found. See above for recommendations.",
523+
],
518524
"info": Array [
519525
"doctor Running checkup",
520526
"doctor Pinging registry",
@@ -616,7 +622,9 @@ verified 0 tarballs
616622

617623
exports[`test/lib/commands/doctor.js TAP incorrect owner > logs 1`] = `
618624
Object {
619-
"error": Array [],
625+
"error": Array [
626+
"Some problems found. See above for recommendations.",
627+
],
620628
"info": Array [
621629
"doctor Running checkup",
622630
"doctor Pinging registry",
@@ -686,6 +694,7 @@ Object {
686694
"doctor checkFilesPermission Missing permissions on {CWD}/global/node_modules (expect: readable)",
687695
"doctor checkFilesPermission Missing permissions on {CWD}/prefix/node_modules/.bin (expect: readable, writable, executable)",
688696
"doctor checkFilesPermission Missing permissions on {CWD}/global/bin (expect: executable)",
697+
"Some problems found. See above for recommendations.",
689698
],
690699
"info": Array [
691700
"doctor Running checkup",
@@ -710,7 +719,9 @@ Object {
710719

711720
exports[`test/lib/commands/doctor.js TAP missing git > logs 1`] = `
712721
Object {
713-
"error": Array [],
722+
"error": Array [
723+
"Some problems found. See above for recommendations.",
724+
],
714725
"info": Array [
715726
"doctor Running checkup",
716727
"doctor Pinging registry",
@@ -776,7 +787,9 @@ verified 0 tarballs
776787

777788
exports[`test/lib/commands/doctor.js TAP missing global directories > logs 1`] = `
778789
Object {
779-
"error": Array [],
790+
"error": Array [
791+
"Some problems found. See above for recommendations.",
792+
],
780793
"info": Array [
781794
"doctor Running checkup",
782795
"doctor Pinging registry",
@@ -895,7 +908,9 @@ verified 0 tarballs
895908

896909
exports[`test/lib/commands/doctor.js TAP node out of date - current > logs 1`] = `
897910
Object {
898-
"error": Array [],
911+
"error": Array [
912+
"Some problems found. See above for recommendations.",
913+
],
899914
"info": Array [
900915
"doctor Running checkup",
901916
"doctor Pinging registry",
@@ -952,7 +967,9 @@ verified 0 tarballs
952967

953968
exports[`test/lib/commands/doctor.js TAP node out of date - lts > logs 1`] = `
954969
Object {
955-
"error": Array [],
970+
"error": Array [
971+
"Some problems found. See above for recommendations.",
972+
],
956973
"info": Array [
957974
"doctor Running checkup",
958975
"doctor Pinging registry",
@@ -1009,7 +1026,9 @@ verified 0 tarballs
10091026

10101027
exports[`test/lib/commands/doctor.js TAP non-default registry > logs 1`] = `
10111028
Object {
1012-
"error": Array [],
1029+
"error": Array [
1030+
"Some problems found. See above for recommendations.",
1031+
],
10131032
"info": Array [
10141033
"doctor Running checkup",
10151034
"doctor Pinging registry",
@@ -1066,7 +1085,9 @@ verified 0 tarballs
10661085

10671086
exports[`test/lib/commands/doctor.js TAP npm out of date > logs 1`] = `
10681087
Object {
1069-
"error": Array [],
1088+
"error": Array [
1089+
"Some problems found. See above for recommendations.",
1090+
],
10701091
"info": Array [
10711092
"doctor Running checkup",
10721093
"doctor Pinging registry",
@@ -1123,7 +1144,9 @@ verified 0 tarballs
11231144

11241145
exports[`test/lib/commands/doctor.js TAP ping 404 > logs 1`] = `
11251146
Object {
1126-
"error": Array [],
1147+
"error": Array [
1148+
"Some problems found. See above for recommendations.",
1149+
],
11271150
"info": Array [
11281151
"doctor Running checkup",
11291152
"doctor Pinging registry",
@@ -1181,7 +1204,9 @@ verified 0 tarballs
11811204

11821205
exports[`test/lib/commands/doctor.js TAP ping 404 in color > logs 1`] = `
11831206
Object {
1184-
"error": Array [],
1207+
"error": Array [
1208+
"Some problems found. See above for recommendations.",
1209+
],
11851210
"info": Array [
11861211
"/u001b[94mdoctor/u001b[39m Running checkup",
11871212
"/u001b[94mdoctor/u001b[39m Pinging registry",
@@ -1239,7 +1264,9 @@ verified 0 tarballs
12391264

12401265
exports[`test/lib/commands/doctor.js TAP ping exception with code > logs 1`] = `
12411266
Object {
1242-
"error": Array [],
1267+
"error": Array [
1268+
"Some problems found. See above for recommendations.",
1269+
],
12431270
"info": Array [
12441271
"doctor Running checkup",
12451272
"doctor Pinging registry",
@@ -1297,7 +1324,9 @@ verified 0 tarballs
12971324

12981325
exports[`test/lib/commands/doctor.js TAP ping exception without code > logs 1`] = `
12991326
Object {
1300-
"error": Array [],
1327+
"error": Array [
1328+
"Some problems found. See above for recommendations.",
1329+
],
13011330
"info": Array [
13021331
"doctor Running checkup",
13031332
"doctor Pinging registry",

‎test/fixtures/mock-npm.js

-17
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,6 @@ const os = require('os')
22
const fs = require('fs').promises
33
const path = require('path')
44
const tap = require('tap')
5-
const { output, META } = require('proc-log')
6-
const errorMessage = require('../../lib/utils/error-message')
75
const mockLogs = require('./mock-logs.js')
86
const mockGlobals = require('@npmcli/mock-globals')
97
const tmock = require('./tmock')
@@ -83,21 +81,6 @@ const getMockNpm = async (t, { mocks, init, load, npm: npmOpts }) => {
8381
await Promise.all(this.unrefPromises)
8482
return res
8583
}
86-
87-
async exec (...args) {
88-
const [res, err] = await super.exec(...args).then((r) => [r]).catch(e => [null, e])
89-
// This mimics how the exit handler flushes output for commands that have
90-
// buffered output. It also uses the same json error processing from the
91-
// error message fn. This is necessary for commands with buffered output
92-
// to read the output after exec is called. This is not *exactly* how it
93-
// works in practice, but it is close enough for now.
94-
const jsonError = err && errorMessage(err, this).json
95-
output.flush({ [META]: true, jsonError })
96-
if (err) {
97-
throw err
98-
}
99-
return res
100-
}
10184
}
10285

10386
const npm = init ? new MockNpm() : null

‎test/lib/cli/entry.js

+12-21
Original file line numberDiff line numberDiff line change
@@ -8,16 +8,22 @@ const validateEngines = require('../../../lib/cli/validate-engines.js')
88
const cliMock = async (t, opts) => {
99
let exitHandlerArgs = null
1010
let npm = null
11-
const exitHandlerMock = (...args) => {
12-
exitHandlerArgs = args
13-
npm.unload()
14-
}
15-
exitHandlerMock.setNpm = _npm => npm = _npm
1611

1712
const { Npm, ...mock } = await loadMockNpm(t, { ...opts, init: false })
1813
const cli = tmock(t, '{LIB}/cli/entry.js', {
1914
'{LIB}/npm.js': Npm,
20-
'{LIB}/cli/exit-handler.js': exitHandlerMock,
15+
'{LIB}/cli/exit-handler.js': class MockExitHandler {
16+
exit (...args) {
17+
exitHandlerArgs = args
18+
npm.unload()
19+
}
20+
21+
registerUncaughtHandlers () {}
22+
23+
setNpm (_npm) {
24+
npm = _npm
25+
}
26+
},
2127
})
2228

2329
return {
@@ -113,21 +119,6 @@ t.test('print usage if no params provided', async t => {
113119
t.match(process.exitCode, 1)
114120
})
115121

116-
t.test('print usage if non-command param provided', async t => {
117-
const { cli, outputs, exitHandlerCalled, exitHandlerNpm } = await cliMock(t, {
118-
globals: {
119-
'process.argv': ['node', 'npm', 'tset'],
120-
},
121-
})
122-
await cli(process)
123-
124-
t.match(outputs[0], 'Unknown command: "tset"')
125-
t.match(outputs[0], 'Did you mean this?')
126-
t.match(exitHandlerCalled(), [], 'should call exitHandler with no args')
127-
t.ok(exitHandlerNpm(), 'exitHandler npm is set')
128-
t.match(process.exitCode, 1)
129-
})
130-
131122
t.test('load error calls error handler', async t => {
132123
const err = new Error('test load error')
133124
const { cli, exitHandlerCalled } = await cliMock(t, {

‎test/lib/cli/exit-handler.js

+199-121
Large diffs are not rendered by default.

‎test/lib/npm.js

+8
Original file line numberDiff line numberDiff line change
@@ -559,3 +559,11 @@ t.test('usage', async t => {
559559
}
560560
})
561561
})
562+
563+
t.test('print usage if non-command param provided', async t => {
564+
const { npm, outputs } = await loadMockNpm(t)
565+
566+
await t.rejects(npm.exec('tset'), { command: 'tset', exitCode: 1 })
567+
t.match(outputs[0], 'Unknown command: "tset"')
568+
t.match(outputs[0], 'Did you mean this?')
569+
})

‎test/lib/utils/error-message.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ const loadMockNpm = async (t, { errorMocks, ...opts } = {}) => {
4040
})
4141
return {
4242
...res,
43-
errorMessage: (er) => mockError(er, res.npm),
43+
errorMessage: (er) => mockError.errorMessage(er, res.npm),
4444
}
4545
}
4646

0 commit comments

Comments
 (0)
Please sign in to comment.