Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Check the app name before proceeding. #628

Merged
merged 6 commits into from
Sep 11, 2016
Merged

Check the app name before proceeding. #628

merged 6 commits into from
Sep 11, 2016

Conversation

mareksuscak
Copy link
Contributor

Fixes #616.

@ghost ghost added the CLA Signed label Sep 11, 2016
@fson fson added this to the 0.4.2 milestone Sep 11, 2016
@ghost ghost added the CLA Signed label Sep 11, 2016
@fson
Copy link
Contributor

fson commented Sep 11, 2016

Thanks, the check seems to work 👍

screen shot 2016-09-11 at 18 52 49

I think the message with big screaming red capital letters sounds a bit too aggressive? I would go with something friendlier like `Can't use "react" as the name of the app, because a dependency with the same name exists. Please use a different name.` (`"react"` here would be the conflicting name the user tried to use.)

fson
fson reviewed Sep 11, 2016
View reviewed changes
global-cli/index.js
@@ -69,14 +69,18 @@ createApp(commands[0], argv.verbose, argv['scripts-version']);

function createApp(name, verbose, version) {
var root = path.resolve(name);
var appName = path.basename(root);

// NPM is whining about installing packages with names equal to appName.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment can be removed.

@mareksuscak
Copy link
Contributor Author

mareksuscak commented Sep 11, 2016 •
edited
Loading

I'm open to suggestions. I just felt like listing all disallowed package names might be a good idea to keep users sane. Uppercased MUST NOT is the standard taken from RFC keywords standard. I think people have gotten used to these keywords in their uppercase form.

I like that it emphasizes the following list of names MUST NOT be used instead of CAN be used.

EDIT: Would changing the color to yellow make any difference? Also listed names could be white.

EDIT2: Or just removing the bold face?

@mareksuscak
Copy link
Contributor Author

We could rephrase it to:

Can't use "react" as the app name because a dependency with the same name exists.

Following names MUST NOT be used:

  react
  react-dom
  react-scripts

@fson
Copy link
Contributor

fson commented Sep 11, 2016

I think the rephrased message looks alright, except for the all caps MUST NOT part. I know that's how it's written in IETFs RFCs, but this is a user interface message, not a standard document. Would just making it bold and lowercase emphasize it enough? I know some tools YELL AT THEIR USERS, but we can do better :)

@mareksuscak
Copy link
Contributor Author

I think it looks good now. What do you think?

screen shot 2016-09-12 at 00 19 16

fson
fson reviewed Sep 11, 2016
View reviewed changes
global-cli/index.js
// TODO: there should be a single place that holds the dependencies
var dependencies = ['react', 'react-dom']
var devDependencies = ['react-scripts']
var allDependencies = dependencies.concat(devDependencies).sort()
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a few semicolons here.

@fson
Copy link
Contributor

fson commented Sep 11, 2016

Looks good to me too!

@fson
Copy link
Contributor

fson commented Sep 11, 2016

Thank you @mareksuscak!

@fson fson merged commit d8b6529 into facebook:master Sep 11, 2016
@mareksuscak mareksuscak deleted the restrict-app-names branch September 11, 2016 22:54
@vjeux
Copy link
Contributor

vjeux commented Sep 12, 2016 •
edited
Loading

Thanks for adding this error message!

If we are to nitpick on must not, it actually doesn't read right to me.

In a standard context, must not is written because the developer has a choice to do the thing that's prohibited and s/he must not do it otherwise it's not going to be spec compliant.

In this case, the user literally can not transgress the rule otherwise 1) we won't let you and 2) it's not going to work.

This is not a big deal so feel free to ignore this message, I just wanted to share my opinion :)

@gaearon
Copy link
Contributor

gaearon commented Sep 12, 2016

Agree. How about we change it to "Due to the way npm works, you can't use the following names:"? Also note the change from passive to active verb, it reads friendlier.

@mareksuscak
Copy link
Contributor Author

@gaearon just to confirm before I jump on it you meant to change the message to:

Can't use "react" as the app name because a dependency with the same name exists.

Due to the way npm works, you can't use the following names:

  react
  react-dom
  react-scripts

gaearon
gaearon reviewed Sep 18, 2016
View reviewed changes
global-cli/index.js
if (allDependencies.indexOf(appName) >= 0) {
console.error(
chalk.red(
`Can't use "${appName}" as the app name because a dependency with the same name exists.\n\n` +
Copy link
Contributor

Choose a reason for hiding this comment

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

This file can’t use template strings. It’s meant to be runnable in older nodes, at least until we warn about unsupported version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I'm very sorry. I've got confused by this line but now I realized it's for create-react-app project itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also requirement for Node >=4 is stated in the README. It is misleading. Shall we change that?
screen shot 2016-09-18 at 14 03 55

Copy link
Contributor

Choose a reason for hiding this comment

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

@mareksuscak The requirement for Node >= 4 is correct, however we want this entry point file to still run in older versions, so that instead of just crashing, we can show a warning, if someone tries to use an unsupported version.

This is what it looks like:
screen shot 2016-09-18 at 15 16 00

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, good then. Didn't know. Thanks for explaining.

@gaearon
Copy link
Contributor

gaearon commented Sep 18, 2016 •
edited
Loading

Tweaked in 061aa2c.

@gaearon gaearon mentioned this pull request Sep 18, 2016
Merged
feiqitian pushed a commit to feiqitian/create-react-app that referenced this pull request Oct 25, 2016
@mareksuscak @feiqitian
Check the app name before proceeding. (facebook#628)
8808a97 
* Check the app name before proceeding.

* Refactor.

* Use arrow function and template string.

* Remove comment.

* Rephrase the error.

* Add missing semicolons.
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gaearon gaearon gaearon left review comments

@fson fson fson left review comments

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
0.4.2
Development

Successfully merging this pull request may close these issues.
4 participants
@mareksuscak @fson @vjeux @gaearon