Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add preSubcommand hook #1763

Merged
merged 4 commits into from
Jul 12, 2022
Merged

feat: add preSubcommand hook #1763

merged 4 commits into from
Jul 12, 2022

Conversation

hungtcs
Copy link
Contributor

@hungtcs hungtcs commented Jun 27, 2022

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

lib/command.js Outdated Show resolved Hide resolved
@shadowspawn shadowspawn changed the base branch from master to develop June 27, 2022 20:50
@shadowspawn
Copy link
Collaborator

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.

@hungtcs hungtcs marked this pull request as ready for review June 28, 2022 03:35
@shadowspawn
Copy link
Collaborator

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
Copy link
Contributor Author

hungtcs commented Jun 28, 2022

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
Copy link
Contributor Author

hungtcs commented Jun 28, 2022

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor Author

hungtcs commented Jun 28, 2022

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

@hungtcs
Copy link
Contributor Author

hungtcs commented Jun 28, 2022

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
Copy link
Contributor Author

hungtcs commented Jun 28, 2022

_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
Copy link
Collaborator

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

@hungtcs hungtcs changed the title feat: add preSubCommand hook feat: add preSubcommand hook Jul 9, 2022
@shadowspawn
Copy link
Collaborator

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!)

typings/index.test-d.ts Outdated Show resolved Hide resolved
@shadowspawn
Copy link
Collaborator

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
Copy link
Contributor Author

hungtcs commented Jul 9, 2022

Yes, it looks much clearer

@shadowspawn
Copy link
Collaborator

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

Readme.md Outdated Show resolved Hide resolved
Readme_zh-CN.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

LGTM

@hungtcs
Copy link
Contributor Author

hungtcs commented Jul 12, 2022

Thanks for your help 😃 @shadowspawn

Copy link
Collaborator

@abetomo abetomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awsome!

@shadowspawn shadowspawn merged commit 2c7f687 into tj:develop Jul 12, 2022
@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Jul 12, 2022
@shadowspawn
Copy link
Collaborator

Released in Commander v9.4.0

Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need a hook or event to set process.env before _parseOptionsEnv
3 participants