Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix npx for non-interactive shells #1936

Closed
wants to merge 1 commit into from
Closed

fix npx for non-interactive shells #1936

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions lib/exec.js
View file
Open in desktop
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
Open in desktop
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