Skip to content

Commit

Permalink
fix: move shellout logic into commands
Browse files Browse the repository at this point in the history
Each command now signals whether or not it is a shellout
  • Loading branch information
wraithgar authored and lukekarrys committed Mar 28, 2022
1 parent 99d8845 commit 45dd8b8
Show file tree
Hide file tree
Showing 10 changed files with 10 additions and 31 deletions.
1 change: 1 addition & 0 deletions lib/commands/birthday.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const BaseCommand = require('../base-command.js')
class Birthday extends BaseCommand {
static name = 'birthday'
static ignoreImplicitWorkspace = true
static isShellout = true

async exec () {
this.npm.config.set('yes', true)
Expand Down
1 change: 1 addition & 0 deletions lib/commands/exec.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ class Exec extends BaseCommand {
]

static ignoreImplicitWorkspace = false
static isShellout = true

async exec (_args, { locationMsg, path, runPath } = {}) {
if (!path) {
Expand Down
1 change: 1 addition & 0 deletions lib/commands/run-script.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ class RunScript extends BaseCommand {
static name = 'run-script'
static usage = ['<command> [-- <args>]']
static ignoreImplicitWorkspace = false
static isShellout = true

async completion (opts) {
const argv = opts.conf.argv.remain
Expand Down
1 change: 1 addition & 0 deletions lib/lifecycle-cmd.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
const BaseCommand = require('./base-command.js')
class LifecycleCmd extends BaseCommand {
static usage = ['[-- <args>]']
static isShellout = true

async exec (args, cb) {
return this.npm.exec('run-script', [this.constructor.name, ...args])
Expand Down
6 changes: 1 addition & 5 deletions lib/npm.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ const Config = require('@npmcli/config')
require('graceful-fs').gracefulify(require('fs'))

const { definitions, flatten, shorthands } = require('./utils/config/index.js')
const { shellouts } = require('./utils/cmd-list.js')
const usage = require('./utils/npm-usage.js')

const which = require('which')
Expand Down Expand Up @@ -62,10 +61,6 @@ class Npm extends EventEmitter {
return this.constructor.version
}

get shelloutCommands () {
return shellouts
}

// Get an instantiated npm command
// npm.command is already taken as the currently running command, a refactor
// would be needed to change this
Expand All @@ -92,6 +87,7 @@ class Npm extends EventEmitter {
if (!this.command) {
process.env.npm_command = command.name
this.command = command.name
this.commandInstance = command
}

// this is async but we dont await it, since its ok if it doesnt
Expand Down
14 changes: 0 additions & 14 deletions lib/utils/cmd-list.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,24 +139,10 @@ const cmdList = [

const plumbing = ['birthday', 'help-search']

// these commands just shell out to something else or handle the
// error themselves, so it's confusing and weird to write out
// our full error log banner when they exit non-zero
const shellouts = [
'exec',
'run-script',
'test',
'start',
'stop',
'restart',
'birthday',
]

module.exports = {
aliases: Object.assign({}, shorthands, affordances),
shorthands,
affordances,
cmdList,
plumbing,
shellouts,
}
2 changes: 1 addition & 1 deletion lib/utils/exit-handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ const exitHandler = err => {
// will presumably print its own errors and exit with a proper status
// code if there's a problem. If we got an error with a code=0, then...
// something else went wrong along the way, so maybe an npm problem?
const isShellout = npm.shelloutCommands.includes(npm.command)
const isShellout = npm.commandInstance && npm.commandInstance.constructor.isShellout
const quietShellout = isShellout && typeof err.code === 'number' && err.code
if (quietShellout) {
exitCode = err.code
Expand Down
9 changes: 0 additions & 9 deletions tap-snapshots/test/lib/utils/cmd-list.js.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -176,15 +176,6 @@ Object {
"birthday",
"help-search",
],
"shellouts": Array [
"exec",
"run-script",
"test",
"start",
"stop",
"restart",
"birthday",
],
"shorthands": Object {
"c": "config",
"cit": "install-ci-test",
Expand Down
1 change: 0 additions & 1 deletion test/lib/npm.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ t.test('not yet loaded', async t => {
set: Function,
},
version: String,
shelloutCommands: Array,
})
t.throws(() => npm.config.set('foo', 'bar'))
t.throws(() => npm.config.get('foo'))
Expand Down
5 changes: 4 additions & 1 deletion test/lib/utils/exit-handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ t.cleanSnapshot = (path) => cleanDate(cleanCwd(path))
// nerf itself, thinking global.process is broken or gone.
mockGlobals(t, {
process: Object.assign(new EventEmitter(), {
...pick(process, 'execPath', 'stdout', 'stderr', 'cwd', 'env'),
...pick(process, 'execPath', 'stdout', 'stderr', 'cwd', 'env', 'umask'),
argv: ['/node', ...process.argv.slice(1)],
version: 'v1.0.0',
kill: () => {},
Expand Down Expand Up @@ -450,7 +450,10 @@ t.test('exits uncleanly when only emitting exit event', async (t) => {
t.test('do no fancy handling for shellouts', async t => {
const { exitHandler, npm, logs } = await mockExitHandler(t)

const exec = await npm.cmd('exec')

npm.command = 'exec'
npm.commandInstance = exec

const loudNoises = () =>
logs.filter(([level]) => ['warn', 'error'].includes(level))
Expand Down

0 comments on commit 45dd8b8

Please # to comment.