.showHelp() and .getHelp() now return same output for commands as --help

[OsmanAltun]

Closes issues:

• closes Show help for no command and handle unknown command #883

[bcoe]

@OsmanAltun 👏 thank you, I've been traveling recently, but will give you review soon.

[OsmanAltun]

@bcoe To fix issue #1791, would setting shortCircuit:true not have fixed it? That's what I had done previously, before merge.

[bcoe]

would setting shortCircuit:true not have fixed it?

@OsmanAltun this would have perhaps been an alternate approach? But I believe the problem was addressed as I started refactoring how help messages were cached, which was necessary to implement .getHelp().

[OsmanAltun]

which was necessary to implement .getHelp().

Ohkay, I get it now.

[bcoe]

@OsmanAltun I was looking at this implementation, and I'm a little concerned by the additional complexity that registering commands with innerArgv introduces.

I was wondering if instead we could update the helper usageFromParentCommandsCommandHandler so that it populates usage information properly for the default command?

function usageFromParentCommandsCommandHandler(
    parentCommands: string[],
    commandHandler: CommandHandler
  ) {
    const c = DEFAULT_MARKER.test(commandHandler.original)
      ? commandHandler.original.replace(DEFAULT_MARKER, '').trim()
      : commandHandler.original;
    const pc = parentCommands.filter(c => {
      return !DEFAULT_MARKER.test(c);
    });
    pc.push(c);
    return `$0 ${pc.join(' ')}`;
  }

☝️ couldn't we figure out how the display the proper usage behavior here, if a default command is registered?

[OsmanAltun]

The helper function is (as far as I know) only responsible for the first line in the usage output, which is the command call order.

If u add all commands here, it would display between the description and the command call order. Besides, to add the commands back, u would still have to loop over every command using yargsInstance.getUsageInstance().command() and/or .usage()

---

The best solution might be to make sure the reset function does also preserve all commands (just like the options). I am not too familiar with the reset function though, I don't know if preserving commands would break functionality.

EDIT:
Well, I tried it out and preserving commands causes stackoverflow. No clue why, so if u got any other ideas or suggestions, lemme know.

[bcoe]

@OsmanAltun I want to encourage external contributions to yargs, and I feel bad for keeping this issue open for so long.

To get patches landed quickly, it's best if:

1. each PR addresses one specific issue (this will help me understand the fix).
2. a patch without a test doesn't explain to me what's being fixed (I've found this PR confusing).
3. the PR title/body should also clearly describe a specific bug being addressed (otherwise I don't have a clue what's going on).

If you provide a test, and a clear description of what you're fixing, I will help guide you towards the part of the codebase where I think we can best fix things.

Very much appreciate you taking your time to make a contribution.

Edit: reading through issues yesterday, I think I got a clearer picture of what you're aiming to fix in this PR 👍. I think my comment below should point you in the right direction. Although, I believe #1820 to be a slightly different issue (we'll probably want to dig into, and fix in a separate PR).

[bcoe]

@OsmanAltun I did some digging this afternoon, and think I see a simpler patch:

diff --git a/lib/yargs-factory.ts b/lib/yargs-factory.ts
index 57d1295..69f6308 100644
--- a/lib/yargs-factory.ts
+++ b/lib/yargs-factory.ts
@@ -1550,10 +1550,11 @@ function Yargs(
 
       const handlerKeys = command.getCommands();
       const requestCompletions = completion!.completionKey in argv;
-      const skipRecommendation = argv[helpOpt!] || requestCompletions;
+      const skipRecommendation =
+        argv[helpOpt!] || requestCompletions || helpOnly;
       const skipDefaultCommand =
         skipRecommendation &&
-        (handlerKeys.length > 1 || handlerKeys[0] !== '$0');
+        (handlerKeys.length > 1 || !command.hasDefaultCommand());
 
       if (argv._.length) {
         if (handlerKeys.length) {

The underlying issue seems to be that we were calling the default command when we shouldn't have.

The default command should not be called when:

1. it has been requested that help be output, and there is no default command.
2. it has been requested that help be output, there is a default command, but there are other commands.

The default command should be called when:

• it has been requested that help be output, and the only command is the default command.

---

Do you want to run with this patch I've shared? The work that needs to be done is adding the following test cases:

it('has same output for --help and .showHelp() when default command is only command')
it('has same output for --help and .showHelp() when there are multiple commands, including default command')
it('has same output for --help and .showHelp() when called from within a handler')

[OsmanAltun]

Great job, everything works as expected for me now. One small question though.
Currently it looks like this inside command.ts -> runCommand():

if (isCommandBuilderCallback(builder)) {
 // logic
} else if (isCommandBuilderOptionDefinitions(builder)) {
 // freeze()
 // reset()
 // unfreeze()
 // similar logic to first block
}

Should freeze/unfreeze not also be applied in the first block?

[bcoe]

Should freeze/unfreeze not also be applied in the first block?

I'm betting we can pull the logic right to the top of the function, and always unfreeze the usage if it's a default command?

If we're running the default command:

1. we want the top level help.
2. we know we definitely won't be running a subcommand, as the precondition for running the default command is that no other commands matched?

[OsmanAltun]

I misread, but yeah, having everything on the root of the function seems better. Right now, I am skipping the isCommandBuilderOptionDefinitions() e.g.:

let innerYargs: YargsInstance;
if (isCommandBuilderCallback(builder)) {
  const builderOutput = builder(yargs.reset(parsed.aliases));
  innerYargs = isYargsInstance(builderOutput) ? builderOutput : yargs;
} else {
  innerYargs = yargs.reset(parsed.aliases);
  Object.keys(commandHandler.builder).forEach(key => {
    innerYargs.option(key, builder[key]);
  });
}

instead of

} else if (isCommandBuilderOptionDefinitions(builder)) {

I have no idea if this is a problem, all tests work, so I am guessing it's ok?

[bcoe]

Right now, I am skipping the isCommandBuilderOptionDefinitions() e.g.:

I'm confused as to why this code needs to be removed as part of the change.

[bcoe]

@OsmanAltun I pushed a couple updates to tests, and tweaked the implementation a bit.

Could you try it out for your use case and make sure I haven't broken it?

[OsmanAltun]

I just checked and the tests don't fail, however the sample code I use for testing fails. I realized this is caused by using yargs.argv instead of yargs.parse(). The tests don't take this into consideration. Adding freeze() fixes this. I assumed that you forgot to add freeze(), since the code snippet you sent didn't work at first.

The code snippet I use for testing:

const argv = yargs.usage('This is my awesome program')
  .commands([
    { command: '*', description: 'testing 123', handler: () => {}},
    { command: 'foo', description: 'testing 1932', handler: () => {}}
  ]).argv;

yargs.showHelp();
console.log(argv);

[bcoe]

I just checked and the tests don't fail, however the sample code I use for testing fails. I realized this is caused by using yargs.argv instead of yargs.parse().

Good catch. The problem is that .parse() called freeze() (as it should) to store the current state of usage, command, etc. Whereas, .argv was a different code path which did not call freeze().

To fix things I switched to making .argv call .parse() simplifying the logic. Mind checking that this fixes things for you?

[OsmanAltun]

Alright, I think everything works as expected now. Go ahead and merge it 👍🏼