fix(call): stop parsing -c for commands + fix corner cases

Some small inefficiencies fixed up in the process.

BREAKING CHANGE: `npx -c "foo"` will no longer install `foo`. Use `-p` to specicify packages to install. npx will no longer assume any particular format or escape status for `-c` strings: they will be passed directly, unparsed, and unaltered, to child_process.spawn.

child.js

@@ -7,12 +7,16 @@ const path = require('path')
 const Y = require('./y.js')
 
 module.exports.runCommand = runCommand
-function runCommand (cmdPath, cmdOpts, opts) {
-  return spawn(cmdPath, cmdOpts, {
+function runCommand (command, opts) {
+  const cmd = opts.call || command || opts.command
+  const copts = (opts.call ? [] : opts.cmdOpts) || []
+  return spawn(cmd, copts, {
     shell: opts.shell || !!opts.call,
     stdio: opts.stdio || 'inherit'
   }).catch({code: 'ENOENT'}, () => {
-    const err = new Error(Y`npx: command not found: ${path.basename(cmdPath)}`)
+    const err = new Error(
+      Y`npx: command not found: ${path.basename(cmd)}`
+    )
     err.exitCode = 127
     throw err
   })

index.js

@@ -9,7 +9,6 @@ const getPrefix = require('./get-prefix.js')
 const parseArgs = require('./parse-args.js')
 const path = require('path')
 const pkg = require('./package.json')
-let rimraf
 const updateNotifier = require('update-notifier')
 const which = BB.promisify(require('which'))
 const Y = require('./y.js')
@@ -34,25 +33,57 @@ function main (argv) {
     }
   }
 
-  if (!argv.command || !argv.package) {
+  if (!argv.call && (!argv.command || !argv.package)) {
     console.error(Y`\nERROR: You must supply a command.\n`)
     parseArgs.showHelp()
     process.exitCode = 1
     return
   }
 
+  // First, we look to see if we're inside an npm project, and grab its
+  // bin path. This is exactly the same as running `$ npm bin`.
   return localBinPath(process.cwd()).then(local => {
-    process.env.PATH = `${local}${PATH_SEP}${process.env.PATH}`
+    if (local) {
+      // Local project paths take priority. Go ahead and prepend it.
+      process.env.PATH = `${local}${PATH_SEP}${process.env.PATH}`
+    }
     return BB.join(
-      getCmdPath(argv.command, argv.package, argv),
-      getEnv(argv),
-      (cmdPath, env) => {
-        const currPath = process.env.PATH
-        process.env = env
-        process.env.PATH = currPath
-        return child.runCommand(cmdPath, argv.cmdOpts, argv)
+      // Figuring out if a command exists, early on, lets us maybe
+      // short-circuit a few things later. This bit here primarily benefits
+      // calls like `$ npx foo`, where we might just be trying to invoke
+      // a single command and use whatever is already in the path.
+      argv.command && getExistingPath(argv.command, argv),
+      // The `-c` flag involves special behavior when used: in this case,
+      // we take a bit of extra time to pick up npm's full lifecycle script
+      // environment (so you can use `$npm_package_xxxxx` and company).
+      // Without that flag, we just use the current env.
+      argv.call && getEnv(argv),
+      (existing, newEnv) => {
+        if (newEnv) {
+          // NOTE - we don't need to manipulate PATH further here, because
+          //        npm has already done so. And even added the node-gyp path!
+          process.env = newEnv
+        }
+        if ((!existing && !argv.call) || argv.packageRequested) {
+          // Some npm packages need to be installed. Let's install them!
+          return ensurePackages(argv.package, argv).then(() => existing)
+        } else {
+          // We can skip any extra installation, 'cause everything exists.
+          return existing
+        }
       }
-    ).catch(err => {
+    ).then(existing => {
+      return child.runCommand(existing, argv).catch(err => {
+        if (err.isOperational && err.exitCode) {
+          // At this point, we want to treat errors from the child as if
+          // we were just running the command. That means no extra msg logging
+          process.exitCode = err.exitCode
+        } else {
+          // But if it's not just a regular child-level error, blow up normally
+          throw err
+        }
+      })
+    }).catch(err => {
       console.error(err.message)
       process.exitCode = err.exitCode || 1
     })
@@ -68,37 +99,26 @@ function localBinPath (cwd) {
 
 module.exports._getEnv = getEnv
 function getEnv (opts) {
-  if (opts.call) {
-    return child.exec(opts.npm, ['run', 'env']).then(env => {
-      return dotenv.parse(env)
-    })
-  } else {
-    return process.env
-  }
+  return child.exec(opts.npm, ['run', 'env']).then(dotenv.parse)
 }
 
-module.exports._getCmdPath = getCmdPath
-function getCmdPath (command, specs, npmOpts) {
-  return getExistingPath(command, npmOpts).then(cmdPath => {
-    if (cmdPath) {
-      return cmdPath
-    } else {
-      return (
-        npmOpts.cache ? BB.resolve(npmOpts.cache) : getNpmCache(npmOpts)
-      ).then(cache => {
-        const prefix = path.join(cache, '_npx')
-        const bins = process.platform === 'win32'
-        ? prefix
-        : path.join(prefix, 'bin')
-        if (!rimraf) { rimraf = BB.promisify(require('rimraf')) }
-        return rimraf(bins).then(() => {
-          return installPackages(specs, prefix, npmOpts).then(() => {
-            process.env.PATH = `${bins}${PATH_SEP}${process.env.PATH}`
-            return which(command)
-          })
-        })
-      })
-    }
+function ensurePackages (specs, opts) {
+  return (
+    opts.cache ? BB.resolve(opts.cache) : getNpmCache(opts)
+  ).then(cache => {
+    const prefix = path.join(cache, '_npx')
+    const bins = process.platform === 'win32'
+    ? prefix
+    : path.join(prefix, 'bin')
+    return BB.promisify(require('rimraf'))(bins).then(() => {
+      return installPackages(specs, prefix, opts)
+    }).then(info => {
+      // This will make temp bins _higher priority_ than even local bins.
+      // This is intentional, since npx assumes that if you went through
+      // the trouble of doing `-p`, you're rather have that one. Right? ;)
+      process.env.PATH = `${bins}${PATH_SEP}${process.env.PATH}`
+      return info
+    })
   })
 }
 

package.json

@@ -38,6 +38,7 @@
   "dependencies": {
     "bluebird": "^3.5.0",
     "dotenv": "^4.0.0",
+    "gauge": "^2.7.4",
     "npm": "^5.0.3",
     "npm-package-arg": "^5.1.2",
     "rimraf": "^2.6.1",

parse-args.js

@@ -111,19 +111,16 @@ function parseArgs (argv) {
     if (typeof parsed.package === 'string') {
       parsed.package = [parsed.package]
     }
-    if (parsed.call) {
-      const splitCmd = parsed.call.trim().split(/\s+/)
-      const parsedCmd = npa(splitCmd[0])
-      parsed.command = parsed.package
-      ? splitCmd[0]
-      : guessCmdName(parsedCmd)
-      parsed.cmdOpts = splitCmd.slice(1)
+    // -c *requires* -p, because the -c string should not be touched by npx
+    if (parsed.call && parsed.package) {
       parsed.packageRequested = !!parsed.package
-      parsed.cmdHadVersion = parsed.package
-      ? false
-      : parsedCmd.name !== parsedCmd.raw
-      const pkg = parsed.package || [splitCmd[0]]
+      parsed.cmdHadVersion = false
+      const pkg = parsed.package
       parsed.p = parsed.package = pkg.map(p => npa(p).toString())
+    } else if (parsed.call && !parsed.package) {
+      parsed.packageRequested = false
+      parsed.cmdHadVersion = false
+      parsed.p = parsed.package = []
     } else if (hasDashDash) {
       const splitCmd = parsed._.slice(2)
       const parsedCmd = npa(splitCmd[0])

test/parse-args.js

@@ -85,23 +85,23 @@ test('parses multiple package options', t => {
   t.done()
 })
 
-test('parses -c', t => {
+test('does not parse -c', t => {
   const parsed = parseArgs(['/node', '/npx', '-c', 'foo a b'])
-  t.equal(parsed.command, 'foo')
-  t.deepEqual(parsed.package, ['foo@latest'])
+  t.deepEqual(parsed.command, null, 'stays unparsed')
+  t.deepEqual(parsed.package, [])
   t.equal(parsed.packageRequested, false)
   t.equal(parsed.cmdHadVersion, false)
-  t.deepEqual(parsed.cmdOpts, ['a', 'b'])
+  t.deepEqual(parsed.cmdOpts, null)
   t.done()
 })
 
 test('uses -p even with -c', t => {
   const parsed = parseArgs(['/node', '/npx', '-c', 'foo a b', '-p', 'bar'])
-  t.equal(parsed.command, 'foo')
+  t.deepEqual(parsed.command, null)
   t.deepEqual(parsed.package, ['bar@latest'])
   t.equal(parsed.packageRequested, true)
   t.equal(parsed.cmdHadVersion, false)
-  t.deepEqual(parsed.cmdOpts, ['a', 'b'])
+  t.deepEqual(parsed.cmdOpts, null)
   t.done()
 })