Run default command if it's not just *

[zephraph]

Pull Request

Problem

I'm trying to build a command like the one below

cli [optional]
cli subcommand [optional]

The implementation loosely looks something like this...

program
	.argument('[optional]')
	.option('-d', 'do a thing')
	.action(() => { console.log('whoop') })

program
	.command('subcommand [optional]')
	.option('-b', 'blah')
	.action(() => { console.log('beh') }

program.parse(process.argv)

When trying to run cli -d the action for the main command won't run. Using program subcommand -b runs fine.

Solution

I traced it down to #844 which contains a fix for this issue: #843. In said issue, @Skywalker13 reported that it was expected that a program that only had one command registered that was the * command shouldn't trigger an action. The fix in #844 just changed it so that if there were any commands registered then the default command won't run.

I preserved the behavior from #843 while also ensuring that valid usecases (i.e. when commands are present besides *) will still cause the base action to trigger.

ChangeLog

Run actions when no optional arguments are provided and real commands are registered.

[shadowspawn]

This might already be fixed by #1047 and #614

[zephraph]

I don't run them as separate executables. The commands are all configured in the same file/process.

[shadowspawn]

Oh right, sorry. I was just working on v4 so fired off a quick reply!

[zephraph]

I do appreciate the quick reply! 😄

[shadowspawn]

I did a little reading and experimentation. Crikey, we have a lot of little quirks and special cases around whether there are subcommands, whether there are git-style executables, whether there are parameters...

At the moment I am not sure if this is a change making things more consistent, although I agree it does not break the existing tests. This change feels a bit too special case to me on first impression.

I'm not going to accept this PR yet as I do not have a clear idea (in my own mind) of sensible high-level story for what we intend for non-subcommands calls with:

• program with and without subcommands using action handlers
• program with and without subcommands using git-style executables
• program with and without action handler on program (which listens on 'command:*')
• program with and without command('*') (which listens on command:*')
• call with and without arguments
• call with and without options

I'm not planning to fix everything at once, but want to be more confident that heading in right direction.

[zephraph]

There's certainly a lot going on, so I understand that sentiment. I have a patch (using patch-package) to unblock me for now.

This change feels a bit too special case to me on first impression

Again, I completely understand this. This is a unique issue that occurs in a very specific circumstance. Here's a minimal reproduction of what I ran into:

// command.js
const commander = require("commander")
const program = new commander.Command();

// This should be called, but won't be because there's another command registered
program.arguments('[option]').action(() => {
  console.log('called')
})

program.command('something').action(() => {
  console.log('does\'t matter')
})

program.parse(process.argv)

node command.js

If the program takes only optional arguments and it has an action, that action will not be called if any commands are registered. The program's action here is treated basically the same as command(*). If there's a required argument or there are no commands registered, it's totally fine.

[shadowspawn]

I have been doing some research today into general command handling.

The original behaviour was that you could define .command('*') to get called as a default handler for the runtime command/argument. The ability to add action handler and pass arguments to program using .arguments() got added in #258. The new support also uses the event command:* as it is is somewhat a replacement or alternative.

I think when program has an action handler with defined arguments, it is reasonable and expected it get called when the calling signature matches, which would mean it should be called in @zephraph example program in opening comment. (I don't think using .arguments() is required, but it does make it more obvious that the user knows what they want!)

I am not sure whether we need to treat .command('*') as different, but I think can look to the default command handling for executable (sub)commands for inspiration. For example, that has been extended to include calling when just options and no other arguments. (See also #843 for using a defined command as the default handler.)

So I think there is a "bug" here, but I don't this is the right fix.

@zephraph would you like to explore a better fix here in this PR, or open a new issue for future consideration with your problem description?

[zephraph]

I’m open to explore other options here.

_{Sent with GitHawk}

[shadowspawn]

Listening for command:* is still a documented use case for detecting unknown commands. I do not think the event is expected to be emitted when there are no arguments. (So effectively the * matches any string but not a missing string.)

I think to some extent .command('*') is legacy now, but we could treat it as a fancy way of declaring a command:* listener and keep it backwards compatible, even if we remove it from the documentation.

With an action handler on the main program, I think there is a reasonable expectation that it is called if the signature matches the declared .arguments(), including with no parameters.

To allow separate behaviours for command:* and program action handler, we would either use a new event for the program action handler, or just make it possible to tell it was defined if they are not usefully both defined.

I haven't thought of a use case where I would want both an action handler on main, and a separate listener on .command:*?

[zephraph]

To allow separate behaviours for command:* and program action handler, we would either use a new event for the program action handler, or just make it possible to tell it was defined if they are not usefully both defined.

Yeah, I think this is the right direction. If they're both defined that's indeterministic functionality and should be a runtime error. I don't think having them both would ever make sense, because you can't differentiate between the arguments on main and an invalid command.

[shadowspawn]

Renamed PR while Work In Progress. 🚧 🛠
😄

[zephraph]

Okay @shadowspawn I think I got something decent. I'm sure there are details we can work through.

• There's a program-action _(name?) event now that's for the main program action handler. That's what gets registered if there's no parent.
• Having both program-action and command:* handlers registered at the same time is considered an error.
• If a command doesn't match, it'll emit command:* if there's a registered handler. Otherwise it'll emit program-action.

The only thing I'm not really confident in is what happens if args.length is 0.
Specifically this line:

commander.js/index.js

Line 821 in 6a87525

this.emit('program-action');

I removed that funky command:* call logic on the this._arg check, but I might need to add it back? Open to feedback.

[zephraph]

I went back through the old issue that added some of the previous logic and made sure we had a test that matched that logic exactly. It adds a little more confidence at least.

[shadowspawn]

FYI: I am completely rewriting the command handling in #1149 informed in part by the hard work we did here to understand the interactions of the "normal" command handling and the * handling and how they might be used.

[shadowspawn]

This is hopefully behaving as desired in published pre-release of Commander v5.0.0 (#1163)

[shadowspawn]

Commander v5.0.0 has been released.

https://github.com/tj/commander.js/releases/tag/v5.0.0