Use "commander" for cli argv handling #1195

Merged
merged 6 commits into from Dec 11, 2016

Projects

None yet

3 participants

@EnoahNetzach
Contributor

commander is more easily configurable than the previous tool, and gives some output out-of-the-box.

Also I've bumped the comment on the minimal node version as per #575.

@gaearon
Contributor
gaearon commented Dec 7, 2016

Could you show the output difference before and after?

@EnoahNetzach
Contributor
EnoahNetzach commented Dec 7, 2016 edited

Before:

  • create-react-app -h
Usage: create-react-app <project-directory> [--verbose]
  • create-react-app --version
create-react-app version: 1.0.0

After:

  • create-react-app -h
  Usage: index [options] <name>

  Options:

    -h, --help                                   output usage information
    -V, --version                                output the version number
    -v, --verbose                                to print logs while init
    -s, --scripts-version <alternative package>  to select a react script variant [react-scripts]

Example of valid script version values:
  - a specific npm version: "0.22.0-rc1"
  - a .tgz archive from any npm repo: "https://registry.npmjs.org/react-scripts/-/react-scripts-0.20.0.tgz"
  - a package prepared with `tasks/clean_pack.sh`: "/Users/home/vjeux/create-react-app/react-scripts-0.22.0.tgz"
  • create-react-app --version
1.0.0
@gaearon
Contributor
gaearon commented Dec 10, 2016

Does the CLI still run on old Node? (It should print an explanation before exiting the process.)

@EnoahNetzach
Contributor

I tried it on a docker running node 0.10.84 and npm 2.15.1.

The output was the same.
After trying to install every and each module.
Wouldn't be better if we just console.error the explanation asap?

@gaearon
Contributor
gaearon commented Dec 10, 2016

Wouldn't be better if we just console.error the explanation asap?

Yep. The thinking is that with time, minimal version of react-scripts may increase so we still need to keep check afterwards. It doesn't hurt to check sooner though!

@EnoahNetzach
Contributor

Should we also add a quick check in travis?

@gaearon
Contributor
gaearon commented Dec 10, 2016

It would be nice.

tasks/e2e.sh
+if [ `node --version | sed -e 's/^v//' -e 's/\..\+//g'` -lt 4 ]
+then
+ cd $temp_app_path
+ node "$root_path"/packages/create-react-app/index.js test-node-version && exit 1 || exit 0
@gaearon
gaearon Dec 10, 2016 Contributor

We should verify that it prints a nice message instead of failing with e.g. a parse error.

@gaearon
Contributor
gaearon commented Dec 11, 2016

I'm a bit concerned the project is abandoned: tj/commander.js#568.

@gaearon
Contributor
gaearon commented Dec 11, 2016

Yarn uses it so should be fine. Thanks!

@gaearon gaearon merged commit 7f9fb29 into facebookincubator:master Dec 11, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gaearon gaearon added this to the 0.8.4 milestone Dec 11, 2016
@stephenjwatkins stephenjwatkins added a commit to stephenjwatkins/create-react-app that referenced this pull request Jan 4, 2017
@EnoahNetzach @stephenjwatkins EnoahNetzach + stephenjwatkins Use "commander" for cli argv handling (#1195)
* Use "commander" for  cli argv handling

* Handle different scripts version forms and exits without a name given

* Revert comment about min supported node version

* Check sooner for the minimal node version

* Add travis test for node <4

* Parse stderr in node versions <4
11accd2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment