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.

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

Use @npmcli/run-script for exec, explore; add interactive exec #2202

Merged
merged 2 commits into from Dec 4, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions docs/content/commands/npm-exec.md
Expand Up @@ -16,9 +16,11 @@ npx <pkg>[@<specifier>] [args...]
npx -p <pkg>[@<specifier>] <cmd> [args...]
npx -c '<cmd> [args...]'
npx -p <pkg>[@<specifier>] -c '<cmd> [args...]'
Run without --call or positional args to open interactive subshell

alias: npm x, npx

common options:
--package=<pkg> (may be specified multiple times)
-p is a shorthand for --package only when using npx executable
-c <cmd> --call=<cmd> (may not be mixed with positional arguments)
Expand All @@ -30,6 +32,11 @@ This command allows you to run an arbitrary command from an npm package
(either one installed locally, or fetched remotely), in a similar context
as running it via `npm run`.

Run without positional arguments or `--call`, this allows you to
interactively run commands in the same sort of shell environment that
`package.json` scripts are run. Interactive mode is not supported in CI
environments when standard input is a TTY, to prevent hangs.

Whatever packages are specified by the `--package` option will be
provided in the `PATH` of the executed command, along with any locally
installed package executables. The `--package` option may be
Expand Down
48 changes: 35 additions & 13 deletions lib/exec.js
@@ -1,7 +1,6 @@
const npm = require('./npm.js')

const output = require('./utils/output.js')
const usageUtil = require('./utils/usage.js')

const usage = usageUtil('exec',
'Run a command from a local or remote npm package.\n\n' +

Expand All @@ -13,7 +12,9 @@ const usage = usageUtil('exec',
'npx <pkg>[@<specifier>] [args...]\n' +
'npx -p <pkg>[@<specifier>] <cmd> [args...]\n' +
'npx -c \'<cmd> [args...]\'\n' +
'npx -p <pkg>[@<specifier>] -c \'<cmd> [args...]\'',
'npx -p <pkg>[@<specifier>] -c \'<cmd> [args...]\'' +
'\n' +
'Run without --call or positional args to open interactive subshell\n',

'\n--package=<pkg> (may be specified multiple times)\n' +
'-p is a shorthand for --package only when using npx executable\n' +
Expand Down Expand Up @@ -59,15 +60,14 @@ const ciDetect = require('@npmcli/ci-detect')
const crypto = require('crypto')
const pacote = require('pacote')
const npa = require('npm-package-arg')
const escapeArg = require('./utils/escape-arg.js')
const fileExists = require('./utils/file-exists.js')
const PATH = require('./utils/path.js')

const cmd = (args, cb) => exec(args).then(() => cb()).catch(cb)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the isTTY definition on line 197 & hoist it up.

Suggested change
const cmd = (args, cb) => exec(args).then(() => cb()).catch(cb)
const cmd = (args, cb) => exec(args).then(() => cb()).catch(cb)
const isTTY = process.stdin.isTTY && process.stdout.isTTY


const run = async ({ args, call, pathArr }) => {
const run = async ({ args, call, pathArr, shell }) => {
// turn list of args into command string
const script = call || args.map(escapeArg).join(' ').trim()
const script = call || args.join(' ').trim() || shell

// do the fakey runScript dance
// still should work if no package.json in cwd
Expand All @@ -83,7 +83,15 @@ const run = async ({ args, call, pathArr }) => {

npm.log.disableProgress()
try {
if (script === shell) {
if (process.stdin.isTTY) {
if (ciDetect())
return npm.log.warn('exec', 'Interactive mode disabled in CI environment')
output(`\nEntering npm script environment\nType 'exit' or ^D when finished\n`)
}
}
return await runScript({
...npm.flatOptions,
pkg,
banner: false,
// we always run in cwd, not --prefix
Expand All @@ -101,13 +109,23 @@ const run = async ({ args, call, pathArr }) => {
}

const exec = async args => {
const { package: packages, call } = npm.flatOptions
const { package: packages, call, shell } = npm.flatOptions

if (call && args.length)
throw usage

const pathArr = [...PATH]

// nothing to maybe install, skip the arborist dance
if (!call && !args.length && !packages.length) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!call && !args.length && !packages.length) {
if (!call && !args.length && !packages.length && isTTY && !ciDetect()) {

return await run({
args,
call,
shell,
pathArr,
})
}

const needPackageCommandSwap = args.length && !packages.length
// if there's an argument and no package has been explicitly asked for
// check the local and global bin paths for a binary named the same as
Expand All @@ -126,8 +144,9 @@ const exec = async args => {
if (binExists) {
return await run({
args,
call: [args[0], ...args.slice(1).map(escapeArg)].join(' ').trim(),
call: [args[0], ...args.slice(1)].join(' ').trim(),
pathArr,
shell,
})
}

Expand Down Expand Up @@ -181,15 +200,18 @@ 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'

if (!isTTY || ciDetect())
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 {
if (!process.stdin.isTTY || ciDetect()) {
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${
Expand All @@ -205,7 +227,7 @@ const exec = async args => {
pathArr.unshift(resolve(installDir, 'node_modules/.bin'))
}

return await run({ args, call, pathArr })
return await run({ args, call, pathArr, shell })
}

const manifestMissing = (tree, mani) => {
Expand Down
76 changes: 41 additions & 35 deletions lib/explore.js
Expand Up @@ -4,59 +4,65 @@
const usageUtil = require('./utils/usage.js')
const completion = require('./utils/completion/installed-shallow.js')
const usage = usageUtil('explore', 'npm explore <pkg> [ -- <command>]')
const rpj = require('read-package-json-fast')

const cmd = (args, cb) => explore(args).then(() => cb()).catch(cb)

const output = require('./utils/output.js')
const npm = require('./npm.js')
const isWindows = require('./utils/is-windows.js')
const escapeArg = require('./utils/escape-arg.js')
const escapeExecPath = require('./utils/escape-exec-path.js')
const log = require('npmlog')

const spawn = require('@npmcli/promise-spawn')

const { resolve } = require('path')
const { promisify } = require('util')
const stat = promisify(require('fs').stat)
const runScript = require('@npmcli/run-script')
const { join, resolve, relative } = require('path')

const explore = async args => {
if (args.length < 1 || !args[0])
throw usage

const pkg = args.shift()
const cwd = resolve(npm.dir, pkg)
const opts = { cwd, stdio: 'inherit', stdioString: true }

const shellArgs = []
if (args.length) {
if (isWindows) {
const execCmd = escapeExecPath(args.shift())
opts.windowsVerbatimArguments = true
shellArgs.push('/d', '/s', '/c', execCmd, ...args.map(escapeArg))
} else
shellArgs.push('-c', args.map(escapeArg).join(' ').trim())
}
const pkgname = args.shift()

await stat(cwd).catch(er => {
throw new Error(`It doesn't look like ${pkg} is installed.`)
})
// detect and prevent any .. shenanigans
const path = join(npm.dir, join('/', pkgname))
if (relative(path, npm.dir) === '')
throw usage

const sh = npm.flatOptions.shell
log.disableProgress()
// run as if running a script named '_explore', which we set to either
// the set of arguments, or the shell config, and let @npmcli/run-script
// handle all the escaping and PATH setup stuff.

if (!shellArgs.length)
output(`\nExploring ${cwd}\nType 'exit' or ^D when finished\n`)
const pkg = await rpj(resolve(path, 'package.json')).catch(er => {
npm.log.error('explore', `It doesn't look like ${pkgname} is installed.`)
throw er
})

log.silly('explore', { sh, shellArgs, opts })
const { shell } = npm.flatOptions
pkg.scripts = {
...(pkg.scripts || {}),
_explore: args.join(' ').trim() || shell,
}

// only noisily fail if non-interactive, but still keep exit code intact
const proc = spawn(sh, shellArgs, opts)
if (!args.length)
output(`\nExploring ${path}\nType 'exit' or ^D when finished\n`)
npm.log.disableProgress()
try {
const res = await (shellArgs.length ? proc : proc.catch(er => er))
process.exitCode = res.code
return await runScript({
...npm.flatOptions,
pkg,
banner: false,
path,
stdioString: true,
event: '_explore',
stdio: 'inherit',
}).catch(er => {
process.exitCode = typeof er.code === 'number' && er.code !== 0 ? er.code
: 1
// if it's not an exit error, or non-interactive, throw it
const isProcExit = er.message === 'command failed' &&
(typeof er.code === 'number' || /^SIG/.test(er.signal || ''))
if (args.length || !isProcExit)
throw er
})
} finally {
log.enableProgress()
npm.log.enableProgress()
}
}

Expand Down
18 changes: 0 additions & 18 deletions lib/utils/escape-arg.js

This file was deleted.

21 changes: 0 additions & 21 deletions lib/utils/escape-exec-path.js

This file was deleted.