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

This removes all other arg/shell escaping mechanisms, as they are no longer needed, and will be un-done by puka in @npmcli/run-script anyway. Adds an interactive shell mode when `npm exec` is run without any arguments, allowing users to interactively test out commands in an npm script environment. Previously, this would do nothing, and just exit. Prevent weird behavior from `npm explore ../blah`. `explore` now can _only_ be used to explore packages that are actually found in the relevant `node_modules` folder.
npm · Dec 2, 2020 · 15d7333 · 15d7333
1 parent e9a440b
commit 15d7333
Show file tree

Hide file tree

Showing 9 changed files with 293 additions and 220 deletions.
diff --git a/docs/content/commands/npm-exec.md b/docs/content/commands/npm-exec.md
@@ -16,9 +16,12 @@ 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)
@@ -30,6 +33,10 @@ 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.
+
 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

diff --git a/lib/exec.js b/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' +
 
@@ -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' +
@@ -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)
 
-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
@@ -84,6 +84,7 @@ const run = async ({ args, call, pathArr }) => {
   npm.log.disableProgress()
   try {
     return await runScript({
+      ...npm.flatOptions,
       pkg,
       banner: false,
       // we always run in cwd, not --prefix
@@ -101,13 +102,24 @@ 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) {
+    output(`\nEntering npm script environment\nType 'exit' or ^D when finished\n`)
+    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
@@ -126,8 +138,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,
       })
     }
 
@@ -187,9 +200,13 @@ const exec = async args => {
         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 (!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${
@@ -205,7 +222,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) => {

diff --git a/lib/explore.js b/lib/explore.js
@@ -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()
   }
 }
 

diff --git a/lib/utils/escape-arg.js b/lib/utils/escape-arg.js
diff --git a/lib/utils/escape-exec-path.js b/lib/utils/escape-exec-path.js
diff --git a/test/lib/exec.js b/test/lib/exec.js
@@ -1,6 +1,8 @@
 const t = require('tap')
 const requireInject = require('require-inject')
 const { resolve, delimiter } = require('path')
+const OUTPUT = []
+const output = (...msg) => OUTPUT.push(msg)
 
 const ARB_CTOR = []
 const ARB_ACTUAL_TREE = {}
@@ -29,6 +31,7 @@ const npm = {
     call: '',
     package: [],
     legacyPeerDeps: false,
+    shell: 'shell-cmd',
   },
   localPrefix: 'local-prefix',
   localBin: 'local-bin',
@@ -91,6 +94,7 @@ const mocks = {
   pacote,
   read,
   'mkdirp-infer-owner': mkdirp,
+  '../../lib/utils/output.js': output,
 }
 const exec = requireInject('../../lib/exec.js', mocks)
 
@@ -123,7 +127,7 @@ t.test('npx foo, bin already exists locally', async t => {
   await exec(['foo'], er => {
     t.ifError(er, 'npm exec')
   })
-  t.strictSame(RUN_SCRIPTS, [{
+  t.match(RUN_SCRIPTS, [{
     pkg: { scripts: { npx: 'foo' }},
     banner: false,
     path: process.cwd(),
@@ -147,7 +151,7 @@ t.test('npx foo, bin already exists globally', async t => {
   await exec(['foo'], er => {
     t.ifError(er, 'npm exec')
   })
-  t.strictSame(RUN_SCRIPTS, [{
+  t.match(RUN_SCRIPTS, [{
     pkg: { scripts: { npx: 'foo' }},
     banner: false,
     path: process.cwd(),
@@ -193,6 +197,33 @@ t.test('npm exec foo, already present locally', async t => {
   }])
 })
 
+t.test('npm exec <noargs>, run interactive shell', async t => {
+  ARB_CTOR.length = 0
+  MKDIRPS.length = 0
+  ARB_REIFY.length = 0
+  OUTPUT.length = 0
+  await exec([], er => {
+    if (er)
+      throw er
+  })
+  t.strictSame(OUTPUT, [
+    ['\nEntering npm script environment\nType \'exit\' or ^D when finished\n'],
+  ], 'printed message about interactive shell')
+  t.strictSame(MKDIRPS, [], 'no need to make any dirs')
+  t.strictSame(ARB_CTOR, [], 'no need to instantiate arborist')
+  t.strictSame(ARB_REIFY, [], 'no need to reify anything')
+  t.equal(PROGRESS_ENABLED, true, 'progress re-enabled')
+  t.match(RUN_SCRIPTS, [{
+    pkg: { scripts: { npx: 'shell-cmd' } },
+    banner: false,
+    path: process.cwd(),
+    stdioString: true,
+    event: 'npx',
+    env: { PATH: process.env.PATH },
+    stdio: 'inherit',
+  }])
+})
+
 t.test('npm exec foo, not present locally or in central loc', async t => {
   const path = t.testdir()
   const installDir = resolve('cache-dir/_npx/f7fbba6e0636f890')