Prefer sending args to _ when both halt-at-non-option and populate-- are enabled

[kherock]

Fixes (part of) #453

There is currently an undocumented interaction between halt-at-non-option and populate-- where the remaining args are sent to -- when populate-- is true, and _ when it's false. There doesn't seem to be any tests enforcing this behavior, and it causes subcommand handling in yargs to break down. Given that this was a latent bug when combined with unknown-options-as-args: true (which was fixed in v21.1.0 by #438), I think it will be the least disruptive to ensure that populate-- has no effect on the behavior of this parse option.

[kherock]

@Toberumono Let me know if you're able to verify that this fix works for you!

[Toberumono]

@kherock That won't work because it's still going to treat the first command name as an invalid "argument", thereby blocking parsing of anything else. If you want to make this work without a flag, yargs-parser is going to need to be able to test whether a command is valid.

[kherock]

I see, so global options that you're sending to your subcommands aren't getting parsed due to halt-on-non-option's behavior. My suggestion is that you should disable halt-at-non-option for your outer yargs instance, and only apply it the inner yargs instances parsing your subcommands. You should be able to restore pre-v21.1.0 behavior with this approach.

@bcoe correct me if I'm wrong, but this means that halt-at-non-option is fundamentally incompatible with most subcommand patterns, right?

[Toberumono]

Again, your fix breaks parsing of commands at every level, not just subcommands, and not just for arguments. What you are suggesting would also mean copying the same parser configuration object into every subcommand, which is both inconvenient and incredibly error-prone, especially when factoring in subcommands that don't necessarily have arguments and trees of subcommands with uneven depths. (See git-remote for an example of both)
For a minimal implementation demonstrating the problem, please see the example that I provided here.

[kherock]

I can't stress this enough, what you are experiencing from #438's merge is a latent bug. You should encounter similar parsing problems on older versions if you use "unknown-options-as-args": false.

This PR should fix some things up for you, but what does not account for in your example are the inputs for

• node script.js valid --thing => Do something here true
• node script.js valid command --thing => Do something else here true

In this case, it's basically in the spec of halt-at-non-option that --thing shouldn't be parsed by the top level yargs. You have to turn it off, or otherwise repeat it as a known option on every subcommand.

[Toberumono]

First, while it may appear on the surface that this PR fixes some things, it's just kicking the can down the road - forcibly shunting stuff into '_' instead of '--' isn't a proper solution to the problem of, "yargs-parser doesn't understand that commands can exist".
Second, your point that "unknown-options-as-args": false produces the same erroneous behavior is just an argument that a similar thing was broken before you made your change, but that doesn't mean that your change is fixing it - it's just making it so that there isn't any combination of flags that produces the result that yargs needs in order to work cleanly (duplicating around 50 lines of code per subcommand in order to deal with argument parsing needing to be reconfigured on a per-subcommand-basis is not my idea of a clean solution).
Third, there is a solution to the problem that doesn't involve attempting to force everything into place, which is what I mentioned in my initial two ideas: add support for passing in a tree of commands to yargs-parser. Considering that it's looking like it'll be necessary, in the meantime, providing a flag that enables your desired functionality until the actual solution to the problem can be put in place seems to me like it would be a far better plan overall because it won't actually break existing functionality at any point.

[kherock]

@Toberumono I understand the frustration here, I'm sorry that I broke your stuff.

This PR also isn't supposed to be a hacked-together-knee-jerk reaction to your problem. I depend on this library to be robust as well!

So for me, I need #438 because I need precisely what halt-at-non-option offers, and the unit tests hopefully prove that I did things right. I didn't run into the bug that this PR addresses at the time because I'm not using subcommands (yet). I would like to understand your argument for a new option addressing #437 if it's just going to behave exactly the same as halt-at-non-option. Could you explain what you're using it for?

[Toberumono]

@kherock I realize that you weren't intending to break anything. And thank you for the apology. It wasn't your fault that it broke - the patch was a logical change in terms of following the words that the flags say. The company I work for has been shifting to using node for larger-scale process automation stuff, and the control scripts use node because the language was already available on those machines. One of the automatic updates nearly pulled in that change after I had (supposedly) pinned the version to 21.0.1, hence me wanting a flag (turns out that yarn doesn't actually obey explicit version-pinning unless you specify them in a bonus area). I'm sorry for being a bit short - I was juggling a couple of things and praying that I didn't get a call at the time.

With regards to the flag I was proposing, it's supposed just toggle the && /^-/.test(arg) that you added, which would (ideally) be more specific than the "halt-at-non-option" flag because I do want it to halt at non-options. It's a stopgap measure at heart - the ideal solution is to add support for commands to yargs-parser, but that is going to take a not-insignificant amount of work.
If you still feel that that flag clashes too much with the other flags, would waiting until the first '-'-prefixed option has appeared in the arguments list before applying your test work? While it's not technically the same functionality as before, [executable] [subcommands] [arguments] is sufficient for keeping my stuff working based on how I've seen the scripts being used.
I can provide a PR for it - it's not exactly a large amount of code, and flags are definitionally prefixed with a '-', so it would be largely immune to parser ambiguities. If that would break your stuff, I can make it opt-in. So long as the functionality is available, I'm not averse to having to turn it on.

[kherock]

Appreciate the context, and no hard feelings.

Unless I'm missing something, I think you can just turn off halt-at-non-option to restore your previous behavior. Before v21.1 It should have been a no-op with your config! maybe not, I forgot to consider that yargs will re-evaluate the rest of the args for subcommands :/

In order to actually leverage it with subcommands, it does seem like you'll need an unfortunate amount of refactoring to make it do useful work, given the existing capabilities of this library. I think the command tree solution makes a lot of sense.

[Toberumono]

Okay. So. After going through my system with several fine-toothed combs and a complete deletion of node-, npm-, and yarn-related cache I could find, it looks like some of the consistency problems that were occurring were actually due to yarn/npm installing two completely separate versions of the library simultaneously.
Setting halt-at-non-option to false does now consistently produce the original behavior (it wasn't earlier, which is just delightful). I think that this pull request can be closed; however, I'll still submit my proposed PR in order to allow halt-at-non-options to be used with subcommands in the manner that I had mentioned expecting.

[kherock]

Good to hear. Before I consider closing this let me verify that yargs itself doesn't already have some special handling for populate-- in this scenario since I still think that this PR's change is still valid. You should be out of the blast zone of this change regardless (since you have the option turned off) but I would be super grateful if you could find time to verify this PR on your updated setup.

[Toberumono]

I think that populate-- should be left alone for now. If it turns out that the refactoring route isn't available, then this might be worth it, but the functionality of populate-- winds up being confusing either way - in my case it was, "nothing is in '_'", but I can see somebody else with the problem, "nothing is in '--'" depending on the expectation of how the flag works. Also, I do have populate-- on in a few other locations, so I'll need to verify that they work without it.

[bcoe]

I agree that populate-- should be left alone for now. To me, it seems more intuitive that if you've specified populate--, anything that would have been populated in _ will end up in -- instead.

I'm leaving more comments in the original thread.

[bcoe]

@Toberumono