Add Command.awaitHook(), Argument.chainArgParserCalls() and Option.chainArgParserCalls()

[aweebit]

Pull Request

Problem

#1900

At the moment, a few lines of extra code are required to enable intuitive use of asynchronous custom processing of arguments and option-arguments by means of async argument parsers.

Solution

The extra code is wrapped in the awaitHook() method of the Command class. It additionally awaits values that the custom processing is not applied to, namely default argument values as well as default and implied option values.

A chainArgParserCalls() method causing thenables returned from parseArg() to be chained is added to both Argument and Option classes. This is relevant when parseArg() is called with second argument supplied, which is the case for variadic arguments and options and repeated non-variadic options.

[shadowspawn]

Interesting that this implicitly supports defaults which are promises.

I am not certain this is robust enough. Some code has run and tested the promises rather than the values, like the processing of implied values. But the code I looked through only cared about the value being defined, and Commander can't make assumptions about what a custom parser returns, so may actually be ok.

[shadowspawn]

processedArgs may include an element which is an array for a variadic argument. I am guessing this is not supported by Promise.all so might need more complex processing.

[shadowspawn]

1. The option value may be an array of values for a variadic option.

2. I think each call to await will delay execution until the next tick. I suggest only call for thenable values.

3. I would slightly prefer not to call setOptionValueWithSource on non-promises, which also fits in with testing for thenable in above item.

[shadowspawn]

There is an existing test for thenable here:

commander.js/lib/command.js

Line 1192 in 4ef19fa

if (promise && promise.then && typeof promise.then === 'function') {

[aweebit]

1. Due to the "previous value" paradigm of custom variadic option processing, the only possible sources for such an array are default and implied values which are assumed to be preprocessed and should ideally be left untouched. If the array really needs to be awaited elementwise, one could simply use Promise.all(arr) as the default / implied value. The same applies to arguments.
2. Solved by 4738e3c.
3. Solved by 4738e3c.

[shadowspawn]

I am intrigued whether this will be good enough to build in rather than be opt-in! See how it goes...

[shadowspawn]

At the moment, a few lines of extra code are required

The few lines will cover many programs, but it is harder building the code into Commander where we have to consider all the ways arguments and options may be declared and used.

[shadowspawn]

A paranoid thought. Are repeated (non-variadic) options a problem because they will create dangling promises?

Suppose lookup does an async parseArg. The promise for bar will overwrite the promise for foo in the option values.

my-utility --lookup foo --lookup bar

[aweebit]

4a6e07c introduces a breaking change which is necessary to enable asynchronous custom processing of variadic arguments and options.

[aweebit]

A paranoid thought. Are repeated (non-variadic) options a problem because they will create dangling promises?

Suppose lookup does an async parseArg. The promise for bar will overwrite the promise for foo in the option values.

my-utility --lookup foo --lookup bar

Dangling pointers don't really pose any threat as long as their exceptions are caught, which I took care for in 4738e3c by running parsedValue.catch(handleError) on line 1181.

[aweebit]

4a6e07c introduces a breaking change which is necessary to enable asynchronous custom processing of variadic arguments and options.

On second thought, there might be use cases where custom processing is expected to produce promises without chaining them, for example

program.argument('<file...>', 'files', (filename, promise) => {
  const newPromise = new Promise((resolve, reject) => {
    // Do something with filename...
  });
  return promise ? Promise.all([promise, newPromise]) : newPromise;
});

So maybe this change does not have to be breaking and should be made available via a method, e.g. program.chainParsedVariadicPromises() / program.chainParsedVariadicThenables().

[aweebit]

Or via an argument method for that matter: new Argument('<file...>', 'files', parseFiles).chainThenables().

[shadowspawn]

Good point that promises and variadic only happen with a custom parseArgs, and author is in control of what is stored. (This was a design decision when adding variadic as parseArgs can be more than just a coercion.)

[aweebit]

Dangling pointers don't really pose any threat as long as their exceptions are caught, which I took care for in 4738e3c by running parsedValue.catch(handleError) on line 1181.

Line 1181 turned out to be not enough since handleError also throws, and we need a way to let users catch those errors. I managed to solve this in c0b7e74.

After spending a lot of time figuring out these unhandled rejections, I now understand your concern about repeated non-variadic options better. The code passes the test I wrote for this case without errors, but I honestly have no idea why the parser only gets called once.

[aweebit]

Oh, now I see it is because we treat non-variadic options the same way as variadic when they are repeated and a parser is provided, so as long as we chain calls to parseArg(), there will be no problem. It is something to consider though because I am planning to make the chaining optional.

[aweebit]

Problem solved by creating an object with backups of overwritten values. Should it just be restricted to thenable values?

[aweebit]

Ok, now asynchronous custom processing (awaitHook()) and parseArg call chaining (chainArgParserCalls()) are two completely independent things, and neither affects parseAsync() when not used, so there are no more breaking changes. I think the code is ready for review.

[shadowspawn]

What use case is chainArgParserCalls for?

I was wondering about dropped promises, but perhaps that is relatively easy for the author to handle.

One of the custom parse examples is:

function collect(value, previous) {
  return previous.concat([value]);
}
program.option('-c, --collect <value>', 'repeatable value', collect, []);

If we make that async, could it just be:

async function collect(value, previous) {
   const previousArray = await previous;
   const newValue = await someAsyncCall(value);
   return previousArray.concat(newValue);
}

If we were were doing a simple one-value coerce/parse, but similarly wanting to avoid dangling promises:

async function coerce(value, previous) {
   await previous; // allow previous promise to settle
   return someAsyncCall(value);
}

[aweebit]

I guess it is just a shortcut that makes it possible to write async parsers that work with two values just like normal ones, rather than with one value and one promise.

Quite an artificial example, but imagine there is a deeply nested structure somewhere on a server, and we are writing a program that accepts keys needed to access a certain property nested in the structure as arguments. We want to join the keys into one key string the server understands, but also we want all substructurs accessed on the way to the property to satisfy certain criteria for the input to be considered valid, and the only way to check it is by sending requests to the server for all partial key strings constructed along the way. The parser could then look like this:

async function parseKeyPart(value, previous) {
  const key = previous ? `${previous}.${value}` : value;
  if (!(await satisfiesCriteria(key))) {
    throw new InvalidArgumentError(
      `Partial key ${key} does not satisfy the criteria`
    );
  }
  return key;
}

Update: Too much work for an argument parser, I agree, but it does not have to be a server, could just as well be a local database only ever accessed once but asynchronously, so the condition would have to be something like

!(satisfiesCriteriaInDb(await dbPromise, key))

That would imply no more work for the parser to do than in my original example in #1900. So there are actually some cases where chainArgParserCalls() could be useful. Of course, a parser like this could be wrapped in a three line long value+promise parser like the one in your collect example, but having a shortcut is always nice.

[shadowspawn]

Some musing, I am not sure of how best to approach these, just at the thinking stage.

I don't think the existing hooks are a good match for settling the option promises at the higher levels of a deeply nested command, which I think are not currently covered. It might make sense to do that in ._dispatchSubcommand() so the options are settled at each level before calling into the subcommand.

That leaves the options on the leaf command itself and the processed arguments, which are currently covered by the preAction implementation.

A complication is that the various places that _processArguments() is called, only the action handler branch of code has support for chaining, so won't get consistent coverage without more work.

[shadowspawn]

For a user implementation of this, walking up the command chain from preAction to settle promises might be fine. The author knows whether they are relying on any option promises to be resolved before their action handler is called, or indeed whether there are any global options at all.

[aweebit]

I don't think the existing hooks are a good match for settling the option promises at the higher levels of a deeply nested command, which I think are not currently covered. It might make sense to do that in ._dispatchSubcommand() so the options are settled at each level before calling into the subcommand.

Well, then preSubcommand seems to be a good match, we just need to make sure the hook is added at each level. I managed to achieve this in my last commit, so please check it out. Also, the new test I added fails when simply walking up the command chain from preAction to settle promises.

[aweebit]

A complication is that the various places that _processArguments() is called, only the action handler branch of code has support for chaining, so won't get consistent coverage without more work.

Could you please elaborate on this? I am not sure I understand the problem. Is it about the risk of unhandled promise rejections again?

[aweebit]

You probably mean the fact the exception is not caught in the following code, although it is when an action is added:

program
  .argument('arg', 'desc', async() => {
    throw 123;
  })
  .awaitHook();

try {
  await program.parseAsync(['abc'], { from: 'user' });
} catch {
  console.log('caught');
}

Well, in that case, adding a new postArguments event for hooks that is always triggered right after _processArguments() is called might be a good idea.

But also, it is kind of weird that the following snippets produce different results.

program
  .argument('arg', 'desc', () => {
    throw 123;
  });

try {
  program.parse(['abc'], { from: 'user' });
} catch {
  console.log('caught');
}

program
  .argument('arg', 'desc', async() => {
    throw 123;
  });

try {
  await program.parseAsync(['abc'], { from: 'user' });
} catch {
  console.log('caught');
}

I am thinking whether it would be reasonable to make the behavior of awaitHook() the default to get rid of this discrepancy, at the expense of introducing a breaking change. What are your thoughts on this?

[aweebit]

Not sure I will have time for this in the coming weeks, so I just went on and implemented the changes I meant.

awaitHook() is now the default with parseAsync(), but it can be disabled by calling awaitHook(false), or applied forcefully to the command and its descendants by calling awaitHook(true). This is a breaking change.

Callbacks are added for both preSubcommand and postArguments events, the latter being a new feature. They are always pushed to the end of the corresponding _lifeCycleHooks arrays. If awaitHook() is called multiple times, the old callback is removed before pushing the new one. All of this ensures the flexibility I mentioned in the discussion of the original issue.

The code is much more complex now and will require thorough testing.

[shadowspawn]

The code is much more complex now and will require thorough testing.

Yes. This is getting too complex for my liking.

High level, your idea of using the stored promises from calling parseArgs to settle later is an appealing way of adding async support, if there are good places to handle the pause. The hooks support async so pausing "near" hooks is likely to be easier.

I wonder how simple this could be leaving out extras like chainArgParserCalls. I'm interested in investigating this myself too, so no worries if this doesn't fit into your ideas or you are don't have time for a while.

Adding a few lines to the "Parsing life cycle and hooks" description, there looks like good places to place the extra calls.

Starting with top-level command (program):

• parse options: parse recognised options (for this command) and remove from args
• parse env: look for environment variables (for this command)
• process implied: set any implied option values (for this command)
• if the first arg is a subcommand
  • settle promises from async Option.parseArgs
  • call preSubcommand hooks
  • pass remaining arguments to subcommand, and process same way

Once reach final (leaf) command:

• check for missing mandatory options
• check for conflicting options
• check for unknown options
• process remaining args as command-arguments
• settle promises from async Option.parseArgs
• settle promises from async Argument.parseArgs
• call preAction hooks
• call action handler
• call postAction hooks

[aweebit]

I wonder how simple this could be leaving out extras like chainArgParserCalls.

chainArgParserCalls() does not have anything to do with awaitHook() and can be left out even now if you do not think it is a useful feature, but I think it is because of this remark, so in my opinion we should keep it and maybe make the default when calling parseAsync(), since we now have breaking changes anyway.

awaitHook() now has three modes, undefined being the default one. If it was called with true rather than undefined as the parameter value, the promises will be awaited even if parse() is called. Do you think we should keep it that way, or only ever await when parseAsync() is called? If we keep it like this, there is a threat of unhandled promise rejections when calling parse() (but this threat is also present when using async hooks with parse(), so maybe we should keep it like this for consistency and assume the user knows what they are doing when using both awaitHook(true) and parse()).

Either way, the same behavior should be implemented for chainArgParserCalls() if we make it the default.

• Either we only chain when parseAsync() is called if we are in the default undefined mode, but allow to force it even on parse() calls by calling chainArgParserCalls(true);
• or we only have the true and false modes and only chain when parseAsync() is called if we are in the default true mode.

awaitHook() needs three modes in either case because the undefined mode also means the decision about whether to await the promises is inherited from the nearest ancestor command that is in either the true or false mode. This makes it possible to only call awaitHook(false) on the top-level command to disable the hook for all descendant commands, despite it now being enabled in all of them by default.

So as you can see, some of the new complexity is unavoidable if we want to have the hook enabled by default: _shouldAwait() has to be kept, for example. The ways to reduce complexity I see are the following:

• Eliminating _awaitHookIndices and therefore the freedom to decide how soon the underlying hooks are called (before, after or in between the other user-supplied preSubcommand and postArguments hooks). The underlying hooks can be added in the constructor instead if we are okay with them always being the first ones, or in parseAsync() / _parseCommand() if we want them to be the last
• Or not using the existing hook infrastructure altogether and instead awaiting in _dispatchSubcommand() and _processArguments() (what you seem to suggest in your "Parsing life cycle and hooks" description)
• Additionally getting rid of the new postArguments event (but to me, it looks like something that has been missing in the library as the need for it to implement awaitHook() using the existing hook infrastructure has shown)

This is what the "Parsing life cycle and hooks" description should look like if we do not use the existing hook infrastructure but keep the postArguments event, and always await before calling the user-supplied hooks:

Parsing life cycle and hooks

Starting with top-level command (program):

• parse options: parse recognised options (for this command) and remove from args
• parse env: look for environment variables (for this command)
• process implied: set any implied option values (for this command)
• if the first arg is a subcommand
  • if the command's (possibly inherited) awaitHook mode is true, or if it is undefined and parseAsync() was called
    • settle promises from async Option.parseArgs
  • call preSubcommand hooks
  • pass remaining arguments to subcommand, and process same way

Once reach final (leaf) command:

• check for missing mandatory options
• check for conflicting options
• check for unknown options
• process remaining args as command-arguments
• if the command's (possibly inherited) awaitHook mode is true, or if it is undefined and parseAsync() was called
  • settle promises from async Option.parseArgs
  • settle promises from async Argument.parseArgs
• call postArguments hooks
• if an action handler is provided
  • call preAction hooks
  • call action handler
  • call postAction hooks

[shadowspawn]

• Or not using the existing hook infrastructure altogether ... (what you seem to suggest in your "Parsing life cycle and hooks" description)

Yes, that was what I had in mind. I opened a PR with an experiment in #1913

[aweebit]

The ways to reduce complexity I see are the following:

• Eliminating _awaitHookIndices and therefore the freedom to decide how soon the underlying hooks are called (before, after or in between the other user-supplied preSubcommand and postArguments hooks). The underlying hooks can be added in the constructor instead if we are okay with them always being the first ones, or in parseAsync() / _parseCommand() if we want them to be the last
• Or not using the existing hook infrastructure altogether and instead awaiting in _dispatchSubcommand() and _processArguments() (what you seem to suggest in your "Parsing life cycle and hooks" description)
• Additionally getting rid of the new postArguments event (but to me, it looks like something that has been missing in the library as the need for it to implement awaitHook() using the existing hook infrastructure has shown)

I have implemented the simplifications in #1915.

[aweebit]

Using the existing hook infrastructure turned out to be a bad idea giving nothing but unnecessary complexity. Closing in favor of #1915.