feat: add preSubcommand hook

[hungtcs]

Pull Request

Problem

Read --env options and process it before subcommand parsed.

Closes #1762

Solution

Add preSubcommand hook to handle some operations that take effect for all subcommands.

ChangeLog

[shadowspawn]

I think this fits in with the idea of life cycle hooks.

The action hooks get called with the command the hook is attached to, and the command with the action handler. I wonder if the preSubcommand hook should get passed the command the hook is attached to, and the subcommand which is about to be called.

[shadowspawn]

An example program for thinking about expected callbacks. This has deeply nested commands. Should the program callback get called just once before the direct subcommand, or multiple times from each level of subcommand?

program
  .name('program')
  .hook('preSubCommand', (thisCommand, subcommand) => {
    console.log(`pre ${thisCommand.name()}/${subcommand.name()}}`)
  });

program
  .command('first')
  .command('second')
  .command('third');

program.parse(['first', 'second', 'third'], { from: 'user' });

Current behaviour at time of writing:

 % node index.js
pre program/first}
pre program/second}
pre program/third}

[hungtcs]

An example program for thinking about expected callbacks. This has deeply nested commands. Should the program callback get called just once before the direct subcommand, or multiple times from each level of subcommand?

program
  .name('program')
  .hook('preSubCommand', (thisCommand, subcommand) => {
    console.log(`pre ${thisCommand.name()}/${subcommand.name()}`)
  });

program
  .command('first')
  .command('second')
  .command('third');

program.parse(['first', 'second', 'third'], { from: 'user' });

Current behaviour at time of writing:

 % node index.js
pre program/first
pre program/second
pre program/third

Yes, that doesn't look right, it should only run once normally, repeated operation will cause trouble and meaningless.
I will try to repair and add corresponding test cases.

[hungtcs]

commander.js/lib/command.js

Lines 1203 to 1205 in 3ae30a2

	if (operands && this._findCommand(operands[0])) {
	return this._dispatchSubcommand(operands[0], operands.slice(1), unknown);
	}

operands are consumed when parsing each level of menu, when operands is empty, this indicates that it was the final subcommand to be executed, I take this as the basis for judgment.

[shadowspawn]

operands are consumed when parsing each level of menu, when operands is empty, this indicates that it was the final subcommand to be executed, I take this as the basis for judgment.

That test won't work. The operands includes command-arguments too. Like: program first second third test.txt

[shadowspawn]

I was wondering about only calling the hook for the immediate subcommands like:

  _chainOrCallSubcommandHook(promise, subCommand, event) {
    let result = promise;
    if (this._lifeCycleHooks[event] !== undefined) {
      this._lifeCycleHooks[event].forEach((hook) => {
        result = this._chainOrCall(result, () => {
          return hook(this, subCommand);
        });
      });
    }
    return result;
  }

    hookResult = this._chainOrCallSubcommandHook(actionResult, subCommand, 'preSubCommand');

For the call:

program.parse(['first', 'second', 'third'], { from: 'user' });

The output would be:

 % node index.js
pre program/first

This feels quite natural for subcommand processing, but is quite different than the way the action hooks work!

[hungtcs]

@shadowspawn I feel like I'm in the wrong process, what you said makes sense.

[hungtcs]

That test won't work. The operands includes command-arguments too. Like: program first second third test.txt

I totally agree with #1763 (comment)

But before that, I additionally realized that I can use subCommand.commands.length to judge whether it is the actual command executed.

[hungtcs]

_chainOrCallHooks looks up all ancestor command hooks, but in the case of subcommands hook, pre its parent command also means pre itself. I think I figured it out.

[shadowspawn]

The hook event name should be preSubcommand rather than preSubCommand, as the README refers to "subcommands" rather than "sub-commands".

[shadowspawn]

These two previous issues ask about passing program options into executable subcommands: #563 #1137

This PR is not a direct solution, but the preSubcommand hooks does allow modifying the environment variables before calling subcommand.

(I don't think the parsing is currently set up to allow directly modifying the remaining arguments to be parsed, which was also mentioned as a possible approach. Future middleware concept!)

[shadowspawn]

How about a table?

---

The supported events are:

event name	when hook called	callback parameters
preAction, postAction	before/after action handler for this command and its nested subcommands	(thisCommand, actionCommand)
preSubcommand	before parsing direct subcommand	(thisCommand, subcommand)

---

Raw markdown:

The supported events are:

| event name | when hook called | callback parameters |
| :-- | :-- | :-- |
| `preAction`, `postAction` |  before/after action handler for this command and its nested subcommands |   `(thisCommand, actionCommand)` |
| `preSubcommand` | before parsing direct subcommand  | `(thisCommand, subcommand)` |

[hungtcs]

Yes, it looks much clearer

[shadowspawn]

Here is a big rework of the existing hook example file to include all the events, with just one subcommand, and one example call each. (Using port idea from your example.)

#!/usr/bin/env node

// const { Command, Option } = require('commander'); // (normal include)
const { Command, Option } = require('../'); // include commander in git clone of commander repo
const program = new Command();

// This example shows using some hooks for life cycle events.

const timeLabel = 'command duration';
program
  .option('--profile', 'show how long command takes')
  .hook('preAction', (thisCommand) => {
    if (thisCommand.opts().profile) {
      console.time(timeLabel);
    }
  })
  .hook('postAction', (thisCommand) => {
    if (thisCommand.opts().profile) {
      console.timeEnd(timeLabel);
    }
  });

program
  .option('--trace', 'display trace statements for commands')
  .hook('preAction', (thisCommand, actionCommand) => {
    if (thisCommand.opts().trace) {
      console.log('>>>>');
      console.log(`About to call action handler for subcommand: ${actionCommand.name()}`);
      console.log('arguments: %O', actionCommand.args);
      console.log('options: %o', actionCommand.opts());
      console.log('<<<<');
    }
  });

program
  .option('--env <filename>', 'specify environment file')
  .hook('preSubcommand', (thisCommand, subcommand) => {
    if (thisCommand.opts().env) {
      // One use case for this hook is modifying environment variables before
      // parsing the subcommand, say by reading .env file.
      console.log(`Reading ${thisCommand.opts().env}...`);
      process.env.PORT = 80;
      console.log(`About to call subcommand: ${subcommand.name()}`);
    }
  });

program.command('start')
  .argument('[script]', 'script name', 'server.js')
  .option('-d, --delay <seconds>', 'how long to delay before starting')
  .addOption(new Option('-p, --port <number>', 'port number').default(8080).env('PORT'))
  .action(async(script, options) => {
    if (options.delay) {
      await new Promise(resolve => setTimeout(resolve, parseInt(options.delay) * 1000));
    }
    console.log(`Starting ${script} on port ${options.port}`);
  });

// Some of the hooks or actions are async, so call parseAsync rather than parse.
program.parseAsync().then(() => {});

// Try the following:
//    node hook.js start
//    node hook.js --trace start --port 9000 test.js
//    node hook.js --profile start --delay 5
//    node hook.js --env=production.env start

[shadowspawn]

Just a couple of minor changes to README references to example file to do.

LGTM

[hungtcs]

Thanks for your help 😃 @shadowspawn

[abetomo]

Awsome!

[shadowspawn]

Released in Commander v9.4.0

Thank you for your contributions.