Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Use "commander" for cli argv handling #1195

Merged
merged 6 commits into from
Dec 11, 2016

Conversation

EnoahNetzach
Copy link
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
Copy link
Contributor

gaearon commented Dec 7, 2016

Could you show the output difference before and after?

@EnoahNetzach
Copy link
Contributor Author

EnoahNetzach commented Dec 7, 2016

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
Copy link
Contributor

gaearon commented Dec 10, 2016

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

@EnoahNetzach
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

Should we also add a quick check in travis?

@gaearon
Copy link
Contributor

gaearon commented Dec 10, 2016

It would be nice.

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@gaearon
Copy link
Contributor

gaearon commented Dec 11, 2016

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

@gaearon
Copy link
Contributor

gaearon commented Dec 11, 2016

Yarn uses it so should be fine. Thanks!

@gaearon gaearon merged commit 7f9fb29 into facebook:master Dec 11, 2016
@gaearon gaearon added this to the 0.8.4 milestone Dec 11, 2016
This was referenced Dec 11, 2016
alexdriaguine pushed a commit to alexdriaguine/create-react-app that referenced this pull request Jan 23, 2017
* 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
randycoulman pushed a commit to CodingZeal/create-react-app that referenced this pull request May 8, 2017
* 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
@lock lock bot locked and limited conversation to collaborators Jan 21, 2019
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants