fix(cli): improve handling of forwarding args/flags

**What**: This migrates us from `commander` and `omelette` to `yargs` and in the process changes how we handle forwarding of arguments and flags. We no longer just arbitrarily forward arguments to every script. Also instead of comma separated scripts, they're simply separated by a space. If you want to pass args/flags to a script, then you put it in quotes. **Why**: Makes this more predicatble. Also `yargs` is more powerful in general anyway. **How**: Magic... Will comment inline...
sezna · Feb 14, 2017 · 67dbed7 · 67dbed7
1 parent 8c04785
commit 67dbed7
Show file tree

Hide file tree

Showing 29 changed files with 752 additions and 476 deletions.
diff --git a/.all-contributorsrc b/.all-contributorsrc
@@ -228,6 +228,15 @@
       "contributions": [
         "bug"
       ]
+    },
+    {
+      "login": "addityasingh",
+      "name": "Aditya Pratap Singh",
+      "avatar_url": "https://avatars.githubusercontent.com/u/5351262?v=3",
+      "profile": "http://blog.adityapsingh.com",
+      "contributions": [
+        "review"
+      ]
     }
   ]
 }
diff --git a/README.md b/README.md
@@ -11,7 +11,7 @@ All the benefits of npm scripts without the cost of a bloated package.json and l
 [![downloads][downloads-badge]][npm-stat]
 [![MIT License][license-badge]][LICENSE]
 
-[![All Contributors](https://img.shields.io/badge/all_contributors-22-orange.svg?style=flat-square)](#contributors)
+[![All Contributors](https://img.shields.io/badge/all_contributors-23-orange.svg?style=flat-square)](#contributors)
 [![PRs Welcome][prs-badge]][prs]
 [![Donate][donate-badge]][donate]
 [![Code of Conduct][coc-badge]][coc]
@@ -58,6 +58,7 @@ module.exports = {
     },
     // learn more about concurrently here: https://npm.im/concurrently
     validate: 'concurrently "nps lint" "nps test" "nps build"',
+    // concurrently script too verbose for your liking? Check out other/EXAMPLES.md!
   },
 }
 ```
@@ -81,27 +82,41 @@ scripts:
 ```
 
 To use `p-s`, it's recommended that you either install it globally (`npm i -g p-s`) or add `./node_modules/bin` to your
-`$PATH` (be careful that you know what you're doing when doing this).
+`$PATH` (be careful that you know what you're doing when doing this, find out how [here](https://youtu.be/2WZ5iS_3Jgs)).
 
 Then you can run:
 
 ```console
-nps --help
+nps help
 ```
 
 Which will output:
 
 ```console
-  Usage: nps [options]
-
-  Options:
-
-    -h, --help                                  output usage information
-    -V, --version                               output the version number
-    -s, --silent                                Silent nps output
-    -c, --config <filepath>                     Config file to use (defaults to nearest package-scripts.yml or package-scripts.js)
-    -l, --log-level <level>                     The log level to use (error, warn, info [default])
-    -r, --require <module>                      Module to preload
+Usage: nps [options] <script>...
+
+Commands:
+  init        automatically migrate from npm scripts to p-s
+  completion  generate bash completion script
+
+Options:
+  --config, -c     Config file to use (defaults to nearest package-scripts.yml
+                   or package-scripts.js)
+                     [default: "<path-to-your-project>/package-scripts.js"]
+  --silent, -s     Silent nps output                  [boolean] [default: false]
+  --log-level, -l  The log level to use
+                   [choices: "error", "warn", "info", "debug"] [default: "info"]
+  --require, -r    Module to preload
+  -h, --help       Show help                                           [boolean]
+  -v, --version    Show version number                                 [boolean]
+
+Examples:
+  nps.js test build                         Runs the `test` script then the
+                                            `build` script
+  nps.js "test --cover" "build --prod"      Runs the `test` script and forwards
+                                            the "--cover" flag then the `build`
+                                            script and forwards the "--prod"
+                                            flag
 
 Available scripts (camel or kebab case accepted)
 
@@ -179,14 +194,7 @@ npm install --global p-s
 
 From here you can use `p-s` on the command line via one of the installed aliases: `nps` or `p-s`.
 
-If you do this, you may also be interested in installing the shell autocompletion script. Do so by running:
-
-```
-nps completion <optionally-your-bash-profile-file>
-```
-
-The bash profile file defaults to `~/.bash_profile` for bash and `~/.zshrc` for zsh. Special thanks to the
-[`omelette`][omelette] package for making this so easy.
+If you do this, you may also be interested in installing the shell autocompletion script. See more about this below.
 
 ## Getting started
 
@@ -222,13 +230,12 @@ As indicated above, this will migrate your npm scripts to package-scripts.
 
 ##### completion
 
-Installs autocompletion functionality into your default bash or zsh configuration. You can override the default by
-providing a specific file:
-
 ```console
-nps completion ~/.bashrc
+nps completion >> <your-bash-profile-file>
 ```
 
+Normally `<your-bash-profile-file>` will be `~/.bash_profile`, `~/.bashrc`, or `~/.zshrc`.
+
 Note: you should probably only do this if you have the package installed globally. In that case you should probably also
 normally use the `nps` alias rather than `p-s` because it's easier to type.
 
@@ -266,32 +273,33 @@ Specify the log level to use
 You can specify a module which will be loaded before the config file is loaded. This allows you to preload for example
 babel-register so you can use all babel presets you like.
 
-##### args
+##### scripts
 
-You can pass additional arguments to the script(s) that are being spawned:
+To run a script, you simply provide the name of the script like so:
 
 ```console
-nps lint --fix # --fix will be passed on to the lint script
+nps cover
 ```
 
-##### scripts
-
-To run a script, you simply provide the name of the script like so:
+And you can run multiple scripts in series by simply adding more space-separated arguments.
 
 ```console
-nps cover
+nps cover check-coverage
 ```
 
-And you can run multiple scripts in series by providing a comma-separated list:
+And you can pass arguments to scripts by putting the scripts in quotes:
 
 ```console
-nps cover,check-coverage
+nps "test --cover" check-coverage
 ```
 
 That's all for the CLI.
 
 ### package-scripts.js
 
+> Remember, this file is JavaScript, so you can write functions to make things more simple!
+> See other/EXAMPLES.md for examples of cool things you can do with this.
+
 `nps` expects to your `package-scripts.js` file to `module.exports` an object with the following properties:
 
 #### scripts
@@ -410,7 +418,7 @@ Thanks goes to these people ([emoji key][emojis]):
 | :---: | :---: | :---: | :---: | :---: | :---: | :---: |
 | [<img src="https://avatars.githubusercontent.com/u/1972567?v=3" width="100px;"/><br /><sub>Tommy</sub>](http://www.tommyleunen.com)<br />[🐛](https://github.com/kentcdodds/p-s/issues?q=author%3Atleunen) [💻](https://github.com/kentcdodds/p-s/commits?author=tleunen) [⚠️](https://github.com/kentcdodds/p-s/commits?author=tleunen) 👀 | [<img src="https://avatars.githubusercontent.com/u/509946?v=3" width="100px;"/><br /><sub>Jayson Harshbarger</sub>](http://www.hypercubed.com)<br />💡 👀 | [<img src="https://avatars.githubusercontent.com/u/1355481?v=3" width="100px;"/><br /><sub>JD Isaacks</sub>](http://www.jisaacks.com)<br />[💻](https://github.com/kentcdodds/p-s/commits?author=jisaacks) [⚠️](https://github.com/kentcdodds/p-s/commits?author=jisaacks) | [<img src="https://avatars.githubusercontent.com/u/924465?v=3" width="100px;"/><br /><sub>Christopher Hiller</sub>](https://boneskull.com)<br />👀 | [<img src="https://avatars.githubusercontent.com/u/1834413?v=3" width="100px;"/><br /><sub>Robin Malfait</sub>](https://robinmalfait.com)<br />💡 | [<img src="https://avatars.githubusercontent.com/u/622118?v=3" width="100px;"/><br /><sub>Eric McCormick</sub>](https://ericmccormick.io)<br />👀 [📖](https://github.com/kentcdodds/p-s/commits?author=edm00se) | [<img src="https://avatars.githubusercontent.com/u/1913805?v=3" width="100px;"/><br /><sub>Sam Verschueren</sub>](https://twitter.com/SamVerschueren)<br />👀 |
 | [<img src="https://avatars.githubusercontent.com/u/1155589?v=3" width="100px;"/><br /><sub>Sorin Muntean</sub>](https://github.com/sxn)<br />[💻](https://github.com/kentcdodds/p-s/commits?author=sxn) [⚠️](https://github.com/kentcdodds/p-s/commits?author=sxn) [📖](https://github.com/kentcdodds/p-s/commits?author=sxn) | [<img src="https://avatars.githubusercontent.com/u/1970063?v=3" width="100px;"/><br /><sub>Keith Gunn</sub>](https://github.com/gunnx)<br />[🐛](https://github.com/kentcdodds/p-s/issues?q=author%3Agunnx) [💻](https://github.com/kentcdodds/p-s/commits?author=gunnx) [⚠️](https://github.com/kentcdodds/p-s/commits?author=gunnx) | [<img src="https://avatars.githubusercontent.com/u/1019478?v=3" width="100px;"/><br /><sub>Joe Martella</sub>](http://martellaj.github.io)<br />[🐛](https://github.com/kentcdodds/p-s/issues?q=author%3Amartellaj) [💻](https://github.com/kentcdodds/p-s/commits?author=martellaj) [⚠️](https://github.com/kentcdodds/p-s/commits?author=martellaj) | [<img src="https://avatars.githubusercontent.com/u/1887854?v=3" width="100px;"/><br /><sub>Martin Segado</sub>](https://github.com/msegado)<br />[📖](https://github.com/kentcdodds/p-s/commits?author=msegado) | [<img src="https://avatars.githubusercontent.com/u/36491?v=3" width="100px;"/><br /><sub>Bram Borggreve</sub>](http://colmena.io/)<br />[🐛](https://github.com/kentcdodds/p-s/issues?q=author%3Abeeman) [💻](https://github.com/kentcdodds/p-s/commits?author=beeman) | [<img src="https://avatars.githubusercontent.com/u/86454?v=3" width="100px;"/><br /><sub>Elijah Manor</sub>](http://elijahmanor.com)<br />📹 | [<img src="https://avatars.githubusercontent.com/u/10691183?v=3" width="100px;"/><br /><sub>Ragu Ramaswamy</sub>](https://github.com/rrag)<br />[💻](https://github.com/kentcdodds/p-s/commits?author=rrag) [⚠️](https://github.com/kentcdodds/p-s/commits?author=rrag) |
-| [<img src="https://avatars.githubusercontent.com/u/2915616?v=3" width="100px;"/><br /><sub>Erik Fox</sub>](http://www.erikfox.co/)<br />[🐛](https://github.com/kentcdodds/p-s/issues?q=author%3Aerikfox) |
+| [<img src="https://avatars.githubusercontent.com/u/2915616?v=3" width="100px;"/><br /><sub>Erik Fox</sub>](http://www.erikfox.co/)<br />[🐛](https://github.com/kentcdodds/p-s/issues?q=author%3Aerikfox) | [<img src="https://avatars.githubusercontent.com/u/5351262?v=3" width="100px;"/><br /><sub>Aditya Pratap Singh</sub>](http://blog.adityapsingh.com)<br />👀 |
 <!-- ALL-CONTRIBUTORS-LIST:END -->
 
 This project follows the [all-contributors][all-contributors] specification.
@@ -455,4 +463,3 @@ MIT
 [scripty]: https://npmjs.com/package/scripty
 [npm scripts]: https://docs.npmjs.com/misc/scripts
 [video]: http://kcd.im/p-s-video
-[omelette]: https://npmjs.com/package/omelette
diff --git a/cli-test/__snapshots__/cli.test.js.snap b/cli-test/__snapshots__/cli.test.js.snap
@@ -1,7 +1,18 @@
+exports[`test with --get-yargs-completions 1`] = `
+Object {
+  "stderr": "",
+  "stdout": "like
+lint
+lint.sub.thing
+lint.sub.hiddenThing
+",
+}
+`;
+
 exports[`test with --require 1`] = `
 Object {
   "stderr": "",
-  "stdout": "[90mnps executing: [39m[32mecho \"log\"[39m
+  "stdout": "nps executing: echo \"log\"
 log
 ",
 }
@@ -17,7 +28,7 @@ Object {
 
 exports[`test with a missing config 1`] = `
 Object {
-  "stderr": "[31mUnable to find JS config at \"./something-that-does-not-exist.js\". Attempted to require as \"<projectRootDir>/cli-test/fixtures/something-that-does-not-exist.js\"[39m https://github.com/kentcdodds/p-s/blob/v0.0.0-semantically-released/other/ERRORS_AND_WARNINGS.md#unable-to-find-config
+  "stderr": "Unable to find JS config at \"./something-that-does-not-exist.js\". Attempted to require as \"<projectRootDir>/cli-test/fixtures/something-that-does-not-exist.js\" https://github.com/kentcdodds/p-s/blob/v0.0.0-semantically-released/other/ERRORS_AND_WARNINGS.md#unable-to-find-config
 ",
   "stdout": "",
 }
@@ -26,32 +37,8 @@ Object {
 exports[`test with config with default script 1`] = `
 Object {
   "stderr": "",
-  "stdout": "[90mnps executing: [39m[32mecho \"default script\"[39m
+  "stdout": "nps executing: echo \"default script\"
 default script
 ",
 }
 `;
-
-exports[`test without arguments 1`] = `
-Object {
-  "stderr": "",
-  "stdout": "
-  Usage: nps [options]
-
-  Options:
-
-    -h, --help               output usage information
-    -V, --version            output the version number
-    -s, --silent             Silent nps output
-    -c, --config <filepath>  Config file to use (defaults to nearest package-scripts.yml or package-scripts.js)
-    -l, --log-level <level>  The log level to use (error, warn, info [default])
-    -r, --require <module>   Module to preload
-
-Available scripts (camel or kebab case accepted)
-
-[32mtest[39m - [90mecho \"test script\"[39m
-[32mlint[39m - [90mecho \"lint.default\"[39m
-[32mlint.sub.thing[39m - [37mthis is a description[39m - [90mecho \"deeply nested thing\"[39m
-",
-}
-`;
diff --git a/cli-test/cli.test.js b/cli-test/cli.test.js
@@ -3,11 +3,6 @@ import runNPS from './run-nps'
 
 const fixturesPath = resolve(__dirname, './fixtures')
 
-test(
-  'without arguments',
-  () => snapshot(),
-)
-
 test(
   'with config with default script',
   () => snapshot('-c ./package-scripts-with-default.js'),
@@ -28,6 +23,11 @@ test(
   () => snapshot('--config ./es6-package-scripts.js --require babel-register log'),
 )
 
+test(
+  'with --get-yargs-completions',
+  () => snapshot('--config ./package-scripts.js --get-yargs-completions li'),
+)
+
 function snapshot(args) {
   return runNPS(fixturesPath, args).then(results => {
     const snapshottableResults = relativizePaths(results)

diff --git a/cli-test/fixtures/package-scripts.js b/cli-test/fixtures/package-scripts.js
@@ -1,6 +1,8 @@
 module.exports = {
   scripts: {
     test: 'echo "test script"',
+    like: 'echo "I like you"',
+    let: 'things',
     lint: {
       default: 'echo "lint.default"',
       sub: {

diff --git a/cli-test/run-nps.js b/cli-test/run-nps.js
@@ -21,7 +21,9 @@ function runNPS(cwd, args = '') {
     let stdout = ''
     let stderr = ''
     const command = `node ${NPS_PATH} ${args}`
-    const child = spawn(command, {cwd})
+    const env = Object.assign({}, process.env)
+    delete env.FORCE_COLOR
+    const child = spawn(command, {cwd, env})
 
     child.on('error', error => {
       reject(error)

diff --git a/other/ERRORS_AND_WARNINGS.md b/other/ERRORS_AND_WARNINGS.md
@@ -30,7 +30,7 @@ This happens when you use the `--require` flag and the module you specify cannot
 
 ## Failed with exit code
 
-This means that one of the scripts p-s tried to run resulted in a non-zero exit code (a failing exit code)
+This means that one of the scripts `p-s` tried to run resulted in a non-zero exit code (a failing exit code)
 
 ### To Fix:
 
@@ -53,3 +53,11 @@ Your `package-scripts.js`, `package-scripts.yml`, or whatever you specified as t
 ### To Fix:
 
 Make sure that your config is an object or a function that returns an object.
+
+## Invalid flags
+
+This happens if you pass flags to `p-s` that are not valid (like `p-s --invalid-flag-name`). This most often happens when you're trying to forward arguments to a script like: `p-s build --fast`
+
+### To Fix:
+
+Make sure you put your scripts and the relevant arguments in quotes: `p-s "build --fast"`
diff --git a/other/EXAMPLES.md b/other/EXAMPLES.md
@@ -69,7 +69,6 @@ module.exports = {
 }
 
 function concurrent(scripts) {
-  process.env.FORCE_COLOR = true // this is necessary until https://github.com/kimmobrunfeldt/concurrently/issues/86
   const names = scripts.join(',')
   const quotedScripts = `"nps ${scripts.join('" "nps ')}"`
   return `concurrently --prefix "[{name}]" --names "${names}" ${quotedScripts}`

diff --git a/package-scripts.js b/package-scripts.js
@@ -1,13 +1,20 @@
 const transpile = 'babel --copy-files --out-dir dist --ignore *.test.js,fixtures src'
 const cleanDist = 'rimraf dist'
-const validate = ['build.andValidate', 'test', 'lint']
+// const npsBin = 'babel-node src/bin/nps.js'
+const npsBin = 'nps'
+const validate = [
+  'build.andValidate',
+  'test',
+  'lint',
+]
 
 module.exports = {
   scripts: {
     commit: {
       description: 'This uses commitizen to help us generate beautifully formatted commit messages',
       script: 'git-cz',
     },
+    temp: 'sleep 8; exit 0',
     test: {
       default: 'jest --config=test/jest.src.config.json --coverage',
       watch: 'jest --config=test/jest.src.config.json --watch',
@@ -26,7 +33,7 @@ module.exports = {
       },
       andValidate: {
         description: 'Runs the normal build first, then validates the CLI',
-        script: 'nps build,test.cli',
+        script: 'nps build && nps test.cli',
       },
     },
     lint: {
@@ -60,8 +67,12 @@ module.exports = {
 }
 
 function concurrent(scripts) {
-  process.env.FORCE_COLOR = true // this is required until https://github.com/kimmobrunfeldt/concurrently/issues/86
   const names = scripts.join(',')
-  const quotedScripts = `"nps ${scripts.join('" "nps ')}"`
-  return `concurrently --prefix "[{name}]" --names "${names}" ${quotedScripts}`
+  const quotedScripts = `"${npsBin} ${scripts.join(`" "${npsBin} `)}"`
+  const colors = [
+    ['bgBlue.bold'],
+    ['bgMagenta.bold'],
+    ['bgGreen.bold'],
+  ]
+  return `concurrently --prefix-colors "${colors.join(',')}" --prefix "[{name}]" --names "${names}" ${quotedScripts}`
 }
diff --git a/package.json b/package.json
@@ -23,17 +23,15 @@
   "dependencies": {
     "arrify": "^1.0.1",
     "chalk": "^1.1.3",
-    "commander": "^2.9.0",
     "find-up": "^2.1.0",
     "js-yaml": "^3.7.0",
     "lodash": "^4.17.4",
     "manage-path": "^2.0.0",
-    "omelette": "^0.3.1",
     "prefix-matches": "^0.0.9",
     "readline-sync": "^1.4.6",
-    "shell-escape": "^0.2.0",
     "spawn-command-with-kill": "^1.0.0",
-    "type-detect": "^4.0.0"
+    "type-detect": "^4.0.0",
+    "yargs": "^6.6.0"
   },
   "devDependencies": {
     "all-contributors-cli": "^3.1.0",
@@ -47,7 +45,7 @@
     "cli-tester": "^2.0.0",
     "codecov": "^1.0.1",
     "commitizen": "^2.9.5",
-    "concurrently": "^3.1.0",
+    "concurrently": "^3.3.0",
     "condition-node-version": "^1.3.0",
     "cross-env": "^3.1.4",
     "cz-conventional-changelog": "^1.2.0",

diff --git a/src/__mocks__/console.js b/src/__mocks__/console.js
@@ -0,0 +1,7 @@
+const console = require.requireActual('console')
+
+module.exports = Object.assign({}, console, {
+  error: jest.fn(),
+  warn: jest.fn(),
+  info: jest.fn(),
+})