Validation for multiple arguments if specified for create-react-app

[jamesgeorge007]

As per now create-react-app <project-name> arg1 works ignoring any arguments provided after the <project-name> without any sort of warnings.

I think it is good to warn the user appropriately if he/she does so 🤔

[henryarihoona]

Waooo

[jamesgeorge007]

@mrmckeb What do you think?

[mrmckeb]

Hi @jamesgeorge007, sorry for the delay.

A side note, we recommend to raise an issue before raising a PR, as that way we can have a discussion about the merits of the work before - we want to avoid people spending their time on a PR that won't be accepted. It also helps make that PR more visible :)

I think it is good to warn the user appropriately if he/she does so
What you've suggested here is actually an error more than a warning, as it doesn't allow the user to continue.

I think what you're proposing makes sense. Some thoughts:

• Have you considered adding a suggestion to use --help for a full list of options? That might be better than just telling the user that the input is wrong.
• Is it better to actually warn the user, and log out unused arguments? They can kill the process then if they wish. My thinking here is that we already ignore unknown options/arguments.

[jamesgeorge007]

@mrmckeb So, shall I update it so that help shows up if the user fires in multiple arguments.
Will that be fine?

[mrmckeb]

I would suggest that it's better to just say something like (as an example):
For a full list of available options, run {...command} --help

The other question is, do we want to exit? We don't exit for invalid flags, so what makes this different?

Thanks for your work here by the way, again, sorry it took so long to get to review.

[jamesgeorge007]

@mrmckeb Updated the warning message as you suggested.

[jamesgeorge007]

@mrmckeb What about a warning message:

Info: you provided more that one argument for the project-name. First one will be considered and the rest ignored

And the process continues without exiting. What are your thoughts?

[mrmckeb]

Let's use a message that matches the format here:
https://github.com/facebook/create-react-app/blob/2524e2e98be8f3e2b44a3349497cac8f8e0dd495/packages/create-react-app/createReactApp.js#L240

For the help text, I would leverage this:
https://github.com/facebook/create-react-app/blob/2524e2e98be8f3e2b44a3349497cac8f8e0dd495/packages/create-react-app/createReactApp.js#L161

So, yellow font, and tell the user:

You have provided more that one argument for <project-directory>. 
// Then help text on a new line, matching the format as above.

Something else to work on - and I'm sorry I didn't raise this earlier - is that there are other valid options that don't start with -. Such as the script-name when using a custom --scripts-version. I think you'll need to look at this a little more and ensure those cases are covered off too :)
https://github.com/facebook/create-react-app/blob/2524e2e98be8f3e2b44a3349497cac8f8e0dd495/packages/create-react-app/createReactApp.js#L90

We need to be very careful here, as breaking this can mean that people can't build new apps ;)

[mrmckeb]

Thanks @jamesgeorge007.

The reason I directed you here (https://github.com/facebook/create-react-app/pull/6080/files#diff-f9867c1e09ced1328f2ccdac4afac4a5R178) was that there is a program name in use there.

I think the check we're doing now is still a little risky. There's also the possibility that someone passes in --scripts-version without the actual package name, which would cause the test to fail.

Can I suggest:

• The test might be safest if it looks only at the the immediate option after where we expect <project-directory>, and if it is not an option (--) then we place this warning. I can't see any other ways to do this safely.
• The message as it is now is good. It would be best if included in this block, https://github.com/facebook/create-react-app/pull/6080/files#diff-f9867c1e09ced1328f2ccdac4afac4a5R175, then we can just share the help messaging:
  if (typeof projectName === 'undefined' || multipleProjectNameArgs) {

Sorry this PR is complicated, it's because you're working on the first thing people see when they start with CRA, so we need to keep this stable and consistent.

[jamesgeorge007]

@mrmckeb

There's also the possibility that someone passes in --scripts-version without the actual package name, which would cause the test to fail.

--scripts-version requires an argument to be provided with it. Right?
Also, not passing in the projectName results in a warning.

Kindly correct me if I'm wrong.
It would be great if you could provide a test-case which results in failure.

Are you trying to say that I should move those validations here

[mrmckeb]

Sorry, you're right, what you've done is safe (I've tested today) as CRA fails without an argument for scripts-version.

Are you trying to say that I should move those validations here
Yes, that's correct. I think the messaging there can be reused, only changing the first part.

I'd strongly suggest that just simplifying this by checking for args up until the first --[arg] (if any exist) would be safest, and aligns better with the goal of this PR. It would also simplify your test.

For example:

yarn create react-app my-app another-app --option #Fail
yarn create react-app my-app another-app #Fail
yarn create react-app my-app --option unimportant-arg #Pass

I hope that all makes sense, sorry I wasn't clearer earlier. If you can make that move, I think we're ready to merge!

[jamesgeorge007]

@mrmckeb Is it fine now?

[jamesgeorge007]

@mrmckeb I haven't heard from you for a while 🤔

[jamesgeorge007]

@Timer @iansu What do you think?

[mrmckeb]

I'm very sorry @jamesgeorge007, I was out of action for a week and a half.

I'll take another quick look tonight or tomorrow and merge this in :)

[mrmckeb]

@jamesgeorge007 Are you OK with the clean-up I made with this commit? There were a few styling issues, and I simplified the test a little.

If that's OK, let's get this in :)

[jamesgeorge007]

@mrmckeb It can be simplified further by using minimist to parse args instead of the manual way.
Anyways, both LGTM 👍

[mrmckeb]

That would be a great addition for future work, thanks for the suggestion @jamesgeorge007.

[mrmckeb]

Thanks again @jamesgeorge007, and congrats on your first contribution. Sorry it was a tricky one.

[jamesgeorge007]

@mrmckeb I guess this change doesn't take into consideration, all those arguments given after the --scripts-version option.

For instance, create-react-app <app-name> --scripts-version <arg> arg works just fine.

Using an argument parser like minimist avoids one from having to reinvent the wheel.
Shall I propose another PR with the minimist way of implementation?

[mrmckeb]

Correct, and that was intentional as this PR was meant to prevent multiple arguments for the name (as I understood it). Adding more validation is a good idea, but I'd prefer to use a tool (as doing it by hand means we'll likely make a mistake).

Your suggesting sounds good to me, I'd love to see how that looks.

[jamesgeorge007]

@mrmckeb Have a look at #6338