Skip to content

Commit

Permalink
fix: empty newline printed to stderr
Browse files Browse the repository at this point in the history
Starting in v7.7.0 running `npm` (no args) is printing an empty newline
to stderr.

This fixes that by correctly exiting via errorHandler and avoiding
hitting the cb() never called error and adds a test to make sure we
avoid that regression moving forward.

Fixes: nodejs/node#37678 (comment)

Co-authored-by: Gar <gar+gh@danger.computer>
  • Loading branch information
ruyadorno and wraithgar committed Mar 29, 2021
1 parent c55ea98 commit 6696bb5
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 7 deletions.
4 changes: 1 addition & 3 deletions lib/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,16 +61,14 @@ module.exports = (process) => {
impl(npm.argv, errorHandler)
else {
try {
// I don't know why this is needed but we get a cb() not called if we
// omit it
npm.log.level = 'silent'
if (cmd) {
const didYouMean = require('./utils/did-you-mean.js')
const suggestions = await didYouMean(npm, npm.localPrefix, cmd)
npm.output(`Unknown command: "${cmd}"${suggestions}\n\nTo see a list of supported npm commands, run:\n npm help`)
} else
npm.output(npm.usage)
process.exitCode = 1
return errorHandler()
} catch (err) {
errorHandler(err)
}
Expand Down
13 changes: 13 additions & 0 deletions smoke-tests/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,19 @@ t.test('npm init', async t => {
t.equal(pkg.version, '1.0.0', 'should have expected generated version')
})

t.test('npm (no args)', async t => {
const cmd = `"${process.execPath}" "${npmLocation}" --no-audit --no-update-notifier`
const cmdRes = await execAsync(cmd, { cwd: localPrefix, env })
.catch(err => {
t.equal(err.code, 1, 'should exit with error code')
return err
})

t.equal(cmdRes.stderr, '', 'should have no stderr output')
t.matchSnapshot(String(cmdRes.stdout),
'should have expected no args output')
})

t.test('npm install prodDep@version', async t => {
const cmd = `${npmBin} install abbrev@1.0.4`
const cmdRes = await exec(cmd)
Expand Down
37 changes: 37 additions & 0 deletions tap-snapshots/smoke-tests-index.js-TAP.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,43 @@
* Make sure to inspect the output below. Do not ignore changes!
*/
'use strict'
exports[`smoke-tests/index.js TAP npm (no args) > should have expected no args output 1`] = `
npm <command>
Usage:
npm install install all the dependencies in your project
npm install <foo> add the <foo> dependency to your project
npm test run this project's tests
npm run <foo> run the script named <foo>
npm <command> -h quick help on <command>
npm -l display usage info for all commands
npm help <term> search for help on <term>
npm help npm more involved overview
All commands:
access, adduser, audit, bin, bugs, cache, ci, completion,
config, dedupe, deprecate, diff, dist-tag, docs, doctor,
edit, exec, explain, explore, find-dupes, fund, get, help,
hook, init, install, install-ci-test, install-test, link,
ll, login, logout, ls, org, outdated, owner, pack, ping,
prefix, profile, prune, publish, rebuild, repo, restart,
root, run-script, search, set, set-script, shrinkwrap, star,
stars, start, stop, team, test, token, uninstall, unpublish,
unstar, update, version, view, whoami
Specify configs in the ini-formatted file:
{CWD}/smoke-tests/index/.npmrc
or on the command line via: npm <command> --key=value
More configuration info: npm help config
Configuration fields: npm help 7 config
npm@7.7.5 {CWD}
`

exports[`smoke-tests/index.js TAP npm diff > should have expected diff output 1`] = `
diff --git a/package.json b/package.json
index v1.0.4..v1.1.1 100644
Expand Down
29 changes: 25 additions & 4 deletions test/lib/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -172,17 +172,37 @@ t.test('gracefully handles error printing usage', t => {
t.teardown(() => {
npmock.output = output
errorHandlerCb = null
errorHandlerCalled = null
})
const proc = {
argv: ['node', 'npm', 'asdf'],
argv: ['node', 'npm'],
on: () => {},
}
npmock.argv = []
npmock.output = (msg) => {
throw new Error('test exception')
errorHandlerCb = () => {
t.match(errorHandlerCalled, [], 'should call errorHandler with no args')
t.end()
}
cli(proc)
})

t.test('handles output error', t => {
const { output } = npmock
t.teardown(() => {
npmock.output = output
errorHandlerCb = null
errorHandlerCalled = null
})
const proc = {
argv: ['node', 'npm'],
on: () => {},
}
npmock.argv = []
npmock.output = () => {
throw new Error('ERR')
}
errorHandlerCb = () => {
t.match(errorHandlerCalled, /test exception/)
t.match(errorHandlerCalled, /ERR/, 'should call errorHandler with error')
t.end()
}
cli(proc)
Expand All @@ -191,6 +211,7 @@ t.test('gracefully handles error printing usage', t => {
t.test('load error calls error handler', t => {
t.teardown(() => {
errorHandlerCb = null
errorHandlerCalled = null
LOAD_ERROR = null
})

Expand Down

0 comments on commit 6696bb5

Please sign in to comment.