Skip to content

Commit

Permalink
fix npx for non-interactive shells
Browse files Browse the repository at this point in the history 
PR-URL: #1936
Credit: @nlf
Close: #1936
Reviewed-by: @ruyadorno
  • Loading branch information
@nlf @ruyadorno
nlf authored and ruyadorno committed Oct 9, 2020
1 parent 09b456f commit e859fba
Show file tree
Hide file tree
Showing 2 changed files with 178 additions and 9 deletions.
22 changes: 14 additions & 8 deletions lib/exec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,19 +126,25 @@ const exec = async args => {

// no need to install if already present
if (add.length) {
const isTTY = process.stdin.isTTY && process.stdout.isTTY
if (!npm.flatOptions.yes) {
// set -n to always say no
if (npm.flatOptions.yes === false) {
throw 'canceled'
}
const addList = add.map(a => ` ${a.replace(/@$/, '')}`)
.join('\n') + '\n'
const prompt = `Need to install the following packages:\n${
addList
}Ok to proceed? `
const confirm = await read({ prompt, default: 'y' })
if (confirm.trim().toLowerCase().charAt(0) !== 'y') {
throw 'canceled'

if (!isTTY) {
npm.log.warn('exec', `The following package${add.length === 1 ? ' was' : 's were'} not found and will be installed: ${add.map((pkg) => pkg.replace(/@$/, '')).join(', ')}`)
} else {
const addList = add.map(a => ` ${a.replace(/@$/, '')}`)
.join('\n') + '\n'
const prompt = `Need to install the following packages:\n${
addList
}Ok to proceed? `
const confirm = await read({ prompt, default: 'y' })
if (confirm.trim().toLowerCase().charAt(0) !== 'y') {
throw 'canceled'
}
}
}
await arb.reify({ ...npm.flatOptions, add })
Expand Down
165 changes: 164 additions & 1 deletion test/lib/exec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ class Arborist {
}

let PROGRESS_ENABLED = true
const LOG_WARN = []
const npm = {
flatOptions: {
yes: true,
Expand All @@ -41,6 +42,9 @@ const npm = {
},
enableProgress: () => {
PROGRESS_ENABLED = true
},
warn: (...args) => {
LOG_WARN.push(args)
}
}
}
Expand Down Expand Up @@ -88,6 +92,7 @@ t.afterEach(cb => {
READ.length = 0
READ_RESULT = ''
READ_ERROR = null
LOG_WARN.length = 0
npm.flatOptions.legacyPeerDeps = false
npm.flatOptions.package = []
npm.flatOptions.call = ''
Expand Down Expand Up @@ -464,7 +469,16 @@ t.test('positional args and --call together is an error', t => {
return exec(['foo'], er => t.equal(er, exec.usage))
})

t.test('prompt when installs are needed if not already present', async t => {
t.test('prompt when installs are needed if not already present and shell is a TTY', async t => {
const stdoutTTY = process.stdout.isTTY
const stdinTTY = process.stdin.isTTY
t.teardown(() => {
process.stdout.isTTY = stdoutTTY
process.stdin.isTTY = stdinTTY
})
process.stdout.isTTY = true
process.stdin.isTTY = true

const packages = ['foo', 'bar']
READ_RESULT = 'yolo'

Expand Down Expand Up @@ -522,7 +536,138 @@ t.test('prompt when installs are needed if not already present', async t => {
}])
})

t.test('skip prompt when installs are needed if not already present and shell is not a tty (multiple packages)', async t => {
const stdoutTTY = process.stdout.isTTY
const stdinTTY = process.stdin.isTTY
t.teardown(() => {
process.stdout.isTTY = stdoutTTY
process.stdin.isTTY = stdinTTY
})
process.stdout.isTTY = false
process.stdin.isTTY = false

const packages = ['foo', 'bar']
READ_RESULT = 'yolo'

npm.flatOptions.package = packages
npm.flatOptions.yes = undefined

const add = packages.map(p => `${p}@`).sort((a, b) => a.localeCompare(b))
const path = t.testdir()
const installDir = resolve('cache-dir/_npx/07de77790e5f40f2')
npm.localPrefix = path
ARB_ACTUAL_TREE[path] = {
children: new Map()
}
ARB_ACTUAL_TREE[installDir] = {
children: new Map()
}
MANIFESTS.foo = {
name: 'foo',
version: '1.2.3',
bin: {
foo: 'foo'
},
_from: 'foo@'
}
MANIFESTS.bar = {
name: 'bar',
version: '1.2.3',
bin: {
bar: 'bar'
},
_from: 'bar@'
}
await exec(['foobar'], er => {
if (er) {
throw er
}
})
t.strictSame(MKDIRPS, [installDir], 'need to make install dir')
t.match(ARB_CTOR, [ { package: packages, path } ])
t.match(ARB_REIFY, [{add, legacyPeerDeps: false}], 'need to install both packages')
t.equal(PROGRESS_ENABLED, true, 'progress re-enabled')
const PATH = `${resolve(installDir, 'node_modules', '.bin')}${delimiter}${process.env.PATH}`
t.match(RUN_SCRIPTS, [{
pkg: { scripts: { npx: 'foobar' } },
banner: false,
path: process.cwd(),
stdioString: true,
event: 'npx',
env: { PATH },
stdio: 'inherit'
}])
t.strictSame(READ, [], 'should not have prompted')
t.strictSame(LOG_WARN, [['exec', 'The following packages were not found and will be installed: bar, foo']], 'should have printed a warning')
})

t.test('skip prompt when installs are needed if not already present and shell is not a tty (single package)', async t => {
const stdoutTTY = process.stdout.isTTY
const stdinTTY = process.stdin.isTTY
t.teardown(() => {
process.stdout.isTTY = stdoutTTY
process.stdin.isTTY = stdinTTY
})
process.stdout.isTTY = false
process.stdin.isTTY = false

const packages = ['foo']
READ_RESULT = 'yolo'

npm.flatOptions.package = packages
npm.flatOptions.yes = undefined

const add = packages.map(p => `${p}@`).sort((a, b) => a.localeCompare(b))
const path = t.testdir()
const installDir = resolve('cache-dir/_npx/f7fbba6e0636f890')
npm.localPrefix = path
ARB_ACTUAL_TREE[path] = {
children: new Map()
}
ARB_ACTUAL_TREE[installDir] = {
children: new Map()
}
MANIFESTS.foo = {
name: 'foo',
version: '1.2.3',
bin: {
foo: 'foo'
},
_from: 'foo@'
}
await exec(['foobar'], er => {
if (er) {
throw er
}
})
t.strictSame(MKDIRPS, [installDir], 'need to make install dir')
t.match(ARB_CTOR, [ { package: packages, path } ])
t.match(ARB_REIFY, [{add, legacyPeerDeps: false}], 'need to install the package')
t.equal(PROGRESS_ENABLED, true, 'progress re-enabled')
const PATH = `${resolve(installDir, 'node_modules', '.bin')}${delimiter}${process.env.PATH}`
t.match(RUN_SCRIPTS, [{
pkg: { scripts: { npx: 'foobar' } },
banner: false,
path: process.cwd(),
stdioString: true,
event: 'npx',
env: { PATH },
stdio: 'inherit'
}])
t.strictSame(READ, [], 'should not have prompted')
t.strictSame(LOG_WARN, [['exec', 'The following package was not found and will be installed: foo']], 'should have printed a warning')
})

t.test('abort if prompt rejected', async t => {
const stdoutTTY = process.stdout.isTTY
const stdinTTY = process.stdin.isTTY
t.teardown(() => {
process.stdout.isTTY = stdoutTTY
process.stdin.isTTY = stdinTTY
})
process.stdout.isTTY = true
process.stdin.isTTY = true

const packages = ['foo', 'bar']
READ_RESULT = 'no, why would I want such a thing??'

Expand Down Expand Up @@ -570,6 +715,15 @@ t.test('abort if prompt rejected', async t => {
})

t.test('abort if prompt false', async t => {
const stdoutTTY = process.stdout.isTTY
const stdinTTY = process.stdin.isTTY
t.teardown(() => {
process.stdout.isTTY = stdoutTTY
process.stdin.isTTY = stdinTTY
})
process.stdout.isTTY = true
process.stdin.isTTY = true

const packages = ['foo', 'bar']
READ_ERROR = 'canceled'

Expand Down Expand Up @@ -617,6 +771,15 @@ t.test('abort if prompt false', async t => {
})

t.test('abort if -n provided', async t => {
const stdoutTTY = process.stdout.isTTY
const stdinTTY = process.stdin.isTTY
t.teardown(() => {
process.stdout.isTTY = stdoutTTY
process.stdin.isTTY = stdinTTY
})
process.stdout.isTTY = true
process.stdin.isTTY = true

const packages = ['foo', 'bar']

npm.flatOptions.package = packages
Expand Down

0 comments on commit e859fba

Please sign in to comment.