Never call preSubcommand hooks on help command

[aweebit]

This PR builds upon the already approved #1930.

Diff only for the change introduced by this PR:
aweebit/commander.js@feature/fixes...feature/help-command-no-preSubcommand

Problem

Currently, there is an inconsistency between how ._dispatchHelpCommand() deals with regular options and those with an executable handler, which seems to have been missed in #1864.

// program.js
const { Command } = require('commander');

const program = new Command()
  .hook('preSubcommand', (_, sub) => {
    console.log(sub.name());
  });
program.command('regular');
program.command('executable', 'description');
program.parse();

// program-executable.js
const { Command } = require('commander');

const program = new Command();
program.parse();

node program help regular # only help
node program help executable # 'executable' logged before help

Solution

Call ._executeSubCommand() directly in ._dispatchHelpCommand().

This is technically a breaking change, but hey, are we really going to assume someone relied on preSubcommand hooks being called before help for just one particular kind of subcommands?

ChangeLog

Fixed

(or maybe Changed?)

• do not call preSubcommand hooks when displaying help for an executable subcommand

[aweebit]

Posted too early by accident, the description is now complete.

[shadowspawn]

I haven't put a lot of thought into this yet, but my first reaction is I am more interested in making it consistent by calling the hook in both cases. Do you want to have a go at that as well? I am imagining a few lines to add the hook before the direct call to the subcommand in _dispatchHelpCommand, not a refactor. Would be a separate competing PR.

I think some people might try using the preSubcommand hook to do some lazy setup, and that might affect the help too.

[aweebit]

I haven't put a lot of thought into this yet, but my first reaction is I am more interested in making it consistent by calling the hook in both cases.

I don't really like this idea for two reasons:

• There is no Command instance that would be reasonable to pass to the preSubcommand hooks in the second parameter since the help command only exists virtually.

• There would be a discrepancy between this and how the preAction hooks are not called when the help option is invoked (and that is a valid comparison because the help option would have to be implemented in the action handler just like the help command would have to be implemented in a subcommand if the convenience shortcuts were not offered by the library).

  try {
    const program = new Command()
      .exitOverride()
      .hook('preAction', () => console.log('preAction'))
      .action(() => {});
    program.parse(['--help'], { from: 'user' }); // 'preAction' not logged
  } catch {}
  
  try {
    const program = new Command()
      .exitOverride()
      .addHelpCommand('help')
      .hook('preSubcommand', () => console.log('preSubcommand'));
    program.parse(['help'], { from: 'user' }); // 'preSubcommand' would be logged
  } catch {}

[shadowspawn]

This has got 5 extra commits, some of which have landed.

Unless you are keen on cherry pick and rebase, I suggest perhaps redo the small relevant commit on the release 12.x branch (I have merged develop into release 12.x today).

77828ba

[shadowspawn]

I was thinking about this again today, and leaning towards triggering the hook again!

#1956 (comment)

[shadowspawn]

Interesting comparison with a "real" help command. I had not considered that model.

I think of the built-in prog help foo as equivalent to prog foo --help. I'm leaning towards calling preSubcommand, as it did before #1864.

(This is pretty obscure behaviour, no one may ever notice!)