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.

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

Proper handling and forwarding of --flags #100

Closed
kentcdodds opened this issue Feb 9, 2017 · 7 comments
Closed

Proper handling and forwarding of --flags #100

kentcdodds opened this issue Feb 9, 2017 · 7 comments

Comments

@kentcdodds
Copy link
Collaborator

Right now you can do:

nps foo --bar

And p-s will run the script called foo with the flag called --bar

However if you run:

nps foo --silent --bar

then p-s will run the script called foo with the flag called --bar but without the flag called --silent. This is because of this.

Normally it's not a huge deal. I think that's confusing.

Here are a few possible solutions (all of them are breaking):

  1. Only accept p-s flags before the scripts (so you'd have to do nps --silent foo --bar)
  2. Require -- before the scripts and flags (nps --silent -- foo --bar)
  3. Forward all flags whether they're p-s related or not (nps foo --silent --bar and forwards --silent and --bar both)

I can't think of a way to change this behavior without breaking things.

My favorite solution is the first. Mostly because it's least noisy and seems to make the most sense.

I'm working on #70 right now and will make this a part of that PR. I want feedback on this really quick. If anyone has any ideas let me know. If you'd like to vote for one or the other use github reactions to vote: 👍 for 1, 😄 for 2, 🎉 for 3, and 😕 for "I don't know/care"

@gunnx
Copy link
Collaborator

gunnx commented Feb 9, 2017 via email

@kentcdodds
Copy link
Collaborator Author

Actually, yargs has a pretty sweet API for this. I think we'll do things in a way that's mostly backward compatible. If you want to forward flags into commands, you'll have to put those commands in quotes. This will make the most sense I think!

@addityasingh
Copy link

@kentcdodds But do you think it would look a clean syntax?

nps 'build --foo'

My 2 cents: I would still prefer to not have any special syntax for my cli commands and below would be better than above:

nps build --foo

@kentcdodds
Copy link
Collaborator Author

Yeah, I can see how the syntax looks much cleaner. As I described though, the problem is that there's no way to know whether the user intends for the command to be passed to p-s or build. Things get even more complicated when you have multiple scripts you're running too. Before we just forwarded all non-p-s flags on, but that got confusing if you wanted to pass on a flag to a script that p-s wouldn't forward (like --silent).

I think that this is a situation where explicit is better. Most of the time I expect that people don't pass flags via the command line directly and the flags happen inside the package-scripts.js file. So saving time typing is ok.

@kentcdodds
Copy link
Collaborator Author

Actually, I've been wrestling with this a lot because I actually realized that I pass flags/arguments to nps on the command line all the time. For example: nps t file-to-test.js --no-cache is something that I'll do fairly regularly. And I would get annoyed if I had to wrap that in quotes everytime.

So here's a proposal:

  • Remove the ability to do multiple scripts in series with a single nps invocation (so no more nps lint test build, you'd have to do nps lint && nps test && nps build)
  • force all nps argument to come before the script name: (so you can't do nps lint --require babel-register --cache you'd have to do nps --require babel-register lint --cache, otherwise --require babel-register would be forwarded onto the lint script in addition to --cache)

I'm not a huge fan of this either. I'm really unsure of what to do here because I don't want to make things confusing, but I want them to be ergonomic. Right now I think we favor economics to clear. Maybe we should just leave things as they are... 🤔

@kentcdodds
Copy link
Collaborator Author

I think we're just going to require the quotes. But I'm going to add option validation so if you try nps build --foo you'll get an error indicating that you may have forgotten to add quotes.

@addityasingh
Copy link

Even though I would have preferred no-quotes, but I see your point now. But adding the error for no quotes should definitely be more consistent 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants