[Feature Request] postParse hooks

[aweebit]

I am currently working on recommander, a library extending Commander's functionality by features that I consider reasonable additions despite there having been little interest shown for them here. It already has async argParser support (#1900), and I would like to add Option.requires() to solve #1802 and something like Option.unrequires() to solve #1855.

Both the checks of the option requirement constraints and the awaiting of options and arguments have to be done after the parsing is finished, so it would be great to have a hook event fired at this point. postParse would be a good name for it.

Currently, the same code has to be run to in three places to simulate such a hook:

• in a preSubcommand hook
• for the leaf command in a preAction hook
• for the leaf command at the end of .parse() / .parseAsync() calls in case it does not have an action handler

To enable the root command on which .parse() / .parseAsync() was called to find the leaf command, the dispatched subcommand needs to be stored in a variable at each level of the command hierarchy. Here are the code lines responsible for this in recommander/lib/command.js:

// in the constructor
const preSubcommandHook = (thisCommand, subcommand) => {
  this.__recommander_dispatchedSubcommand =
    /** @type {Command} */ (subcommand);
  this.__recommander_dispatchedSubcommand
    .__recommander_newParseState(this.__recommander_asyncParsing);
  return this.__recommander_await();
};
this.hook('preSubcommand', preSubcommandHook);

// in a subroutine used by .parse() and .parseAsync()
let leafCommand;
for (
  leafCommand = this;
  leafCommand.__recommander_dispatchedSubcommand;
  leafCommand = leafCommand.__recommander_dispatchedSubcommand
);
leafCommand.__recommander_await(); // in case it has no action handler

Very annoying and unnecessary! This shows postParse hooks are a must-have feature for subclasses.

[aweebit]

On second thought, postParse might not be a good name because it could make it seem like the event is always fired at the end of a .parse() call.

[aweebit]

The idea was actually already implemented in #1902 (a451ac5). The event name was postArguments, which might be a better alternative.

The most precise, non-confusing name I can think of is postParseOptionsAndArguments, but that is way too long.

[shadowspawn]

The scenario that proved messy to cover in #1902 #1913 is the no-action-hander fall-through case.

I think a hook to do post-parse-processing is a potentially useful new location. With a good name. 😆

I wonder whether this would be easy to do by adding another layer and calling the hook after:

commander.js/lib/command.js

Lines 1259 to 1265 in 4ef19fa

	_parseCommand(operands, unknown) {
	const parsed = this.parseOptions(unknown);
	this._parseOptionsEnv(); // after cli, so parseArg not called on both cli and env
	this._parseOptionsImplied();
	operands = operands.concat(parsed.operands);
	unknown = parsed.unknown;
	this.args = operands.concat(unknown);

[aweebit]

The scenario that proved messy to cover in #1902 #1913 is the no-action-hander fall-through case.

Exactly.

As I said, #1902 had already introduced this new event. I closed the PR myself because relying on the hook infrastructure seemed too messy, but it actually doesn't look so bad now when I think about it. What I did not realize back then is that

• using just the postArguments hook would be enough if the event would also always be fired before preSubcommand (which it should be although I forgot to make it clear in the issue description)
• caring about values overwritten by option repetition would be excessive if chainArgParserCalls were on by default (users of the library would just need to be warned that turning it off for non-variadic options that are not expected to be repeated is a bad idea like in this JSDoc in recommander, but there would be no reason for them to do it anyway)

Also, I was clinging on the idea of letting users of the library decide at which hook processing stage to add the await hook that originally came up in #1900 (comment) too much. It wasn't very useful even when users had to call .awaitHook() to enable awaiting like in #1902, but then in #1915, the freedom for users to control whether awaiting is on was removed altogether, and I now think the same should have been done in #1902 because it would greatly simplify everything. The hook would just need to be added in the Command constructor. (A good idea would be to add it without calling .hook() because otherwise, the fact it is used internally for awaiting could be tracked by overriding the method, which we don't want.)

Taking all that into consideration, the code changes in #1902 could be simplified to only include

• the .asyncParsing variable used to decide whether to await and chain
• the addition of a postArguments awaiting hook in the constructor
• the chainArgParserCalls setting for options and arguments with true being the default value (but only being respected if .asyncParsing is true)
• the ._parseArg() method taken from #1915 (but without handledResultCallback because we wouldn't care about overwritten option values anymore)
• an ._await() method like the one in recommander

#1915 was closed because it was too complex. Here is how the reasons in the closing comment could be countered if this new suggestion from above is implemented.

---

• not much requested

True, but unlike most other enhancement I've suggested over the past month, this one is motivated by a real use case in one of my projects.

• complicates _parseCommand which is already a long complicated routine

The only complication due to the new awaiting functionality is that .asyncParsing needs to be set and unset in ._parseCommand(). However, that variable could also be useful in other places like #1919 has shown.

• subtle and complicated async error handling to maintain

Not really, since overwritten option values are not handled specially anymore. There is only one line responsible for rejection handling, namely this one from ._parseArgSubroutine():

handledResult = result.then(x => x, handleError);

If the rejection is not due to an invalid argument, it is supposed to be handled by the library user.

• requires much more use of async chaining in the implementation (e.g. _chainOrCall)

No additional places where ._chainOrCall() gets called in code due to the awaiting functionality anymore.

• does not provide a clean implementation of async: delayed resolution after other code, out of order resolution, promises temporarily stored instead of values. This will lead to some occasional subtle problems for authors. To be clear, this is pretty much impossible to avoid without a drastic refractor! But means there are downsides.

The awaiting of both the option and argument values is now done in the same place and only once. Resolution is out-of-order only for options and arguments of one particular command in the subcommand chain, but that is expected when using async features.

I cannot think of any problems the enhancement would introduce for users of the library because they wouldn't get to see the non-awaited values anywhere when using .parseAsync() (guaranteed by the fact the awaiting hook is registered in the constructor and is therefore always the first one).

• simple use cases can be handled by author with "a few lines of extra code", but really hard to handle in Commander for general use. In particular, in existing hooks (such as in early implementations).

The new hook event and the fact overwritten values are not handled specially anymore make the implementation of the awaiting functionality quite simple even in the general case.

---

I might open a PR implementing this suggestion later if it's okay with you.

[aweebit]

I think a hook to do post-parse-processing is a potentially useful new location. With a good name. 😆

I am thinking about giving up the pre/post convention and naming the event something like

• argumentsParsed
  • Might make it seem like that doesn't include options because the word "argument" is used to only mean a command-argument in the method names of .argument, .arguments and .addArgument.
• argumentsAndOptionsParsed / optionsAndArgumentsParsed
  • No argument / command-argument confusion anymore.
  • Too long and hard to remember what the right word order is.
• commandInputParsed
  • Again, no argument / command-argument confusion.
  • The addition of "command" signifies that the event is fired for each command in the subcommand chain.
  • I think this one is the best so far.

Another option I've been thinking of is postPreprocessing, but it probably isn't clear enough.

I wonder whether this would be easy to do by adding another layer and calling the hook after:

commander.js/lib/command.js

Lines 1259 to 1265 in 4ef19fa

	_parseCommand(operands, unknown) {
	const parsed = this.parseOptions(unknown);
	this._parseOptionsEnv(); // after cli, so parseArg not called on both cli and env
	this._parseOptionsImplied();
	operands = operands.concat(parsed.operands);
	unknown = parsed.unknown;
	this.args = operands.concat(unknown);

Okay for "inner" commands, but not the leaf command because argParsers for command-arguments haven't been run yet. The two places where I think the event should be fired are

• right before preSubcommand is fired in ._dispatchSubcommand()
• and at the end of ._processArguments() like in #1902.

[shadowspawn]

Okay for "inner" commands, but not the leaf command because argParsers for command-arguments haven't been run yet.

Right. I thought it seemed too easy (else we would have found it before!).

[shadowspawn]

This issue has not had any activity in over six months. It isn't likely to get acted on due to this report.

The new hook would be useful for some cases, but is messy, so will wait for more interest. See comment for more background: #1976 (comment)

Feel free to open a new issue if it comes up again, with new information and renewed interest.

Thank you for your contributions.