Add async custom processing support. Add chainArgParserCalls() for configuration. Additionally await thenable implicit and default option values and thenable default argument values

[aweebit]

This is a rework of #1902 not relying on the hook infrastructure and not introducing postArguments hooks, i.e. all simplifications from this comment have been implemented.

Changes summary

ChangeLog

Added

• Option.chainArgParserCalls() and Argument.chainArgParserCalls() to control whether to chain parseArg calls
• warnings about thenable option and argument values in .parse() calls, including default and implied values and ones returned from async argParsers, with a suggestion to use .parseAsync() instead

Changed

• Breaking: chain parseArg calls by default when .parseAsync() is called
• Breaking: await thenable option and argument values when .parseAsync() is called

Configuration details

Option.chainArgParserCalls() / Argument.chainArgParserCalls()

• When set to true (the default), next call to the function provided via .argParser() will be chained to its return value if it is thenable.

New life cycle description

The processing starts with an array of args. Each command processes and removes the options it understands, and passes the remaining args to the next subcommand. The final command calls the action handler.

Starting with top-level command (program):

• process options for this command (includes calling or chaining provided argParsers¹)
  • recognised options from args
  • options based on environment variables
• set implied option values (for this command)
• remove recognised options from args
• await thenable option values if parsing with parseAsync()
• if the first arg is a subcommand
  • call or chain 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 (includes calling or chaining provided argParsers¹)
• await thenable argument values if parsing with parseAsync()
• if an action handler is provided
  • call or chain preAction hooks¹
  • call or chain action handler¹
  • call or chain postAction hooks¹

Some explanations

Why enable chaining and awaiting for parseAsync() by default?

A good question, especially because this introduces breaking changes. The decision is motivated by the fact it is most logical for the following code snippets to produce the same results:

program
  .option('--fail', 'desc', () => {
    throw 'exception';
  })
  .argument('[args...]', 'desc', (value, previous) => (
    previous === undefined ? value : previous + value
  ))
  .action(processed => {
    console.log(processed);
  });

program.parse(['1', '2'], { from: 'user' });

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

program
  .option('--fail', 'desc', async() => {
    throw 'exception';
  })
  .argument('[args...]', 'desc', async(value, previous) => (
    previous === undefined ? value : previous + value
  ))
  .action(processed => {
    console.log(processed);
  });

await program.parseAsync(['1', '2'], { from: 'user' });

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

Without chaining, a promise resolving to '[object Promise]2' is logged from inside the action instead of 12 when running the second snippet.

Without awaiting, an unhandled promise rejection is reported instead of logging caught exception when running the second snippet.

Why not enable chaining and awaiting for parse()?

• To make the changes as "non-breaking" as possible (although they will be breaking anyway).

  There might be production code like the one in the original example in Consider supporting async option/argument parsers #1900 somewhere. By not enabling awaiting for parse() by default, we make sure this code still works after the upgrade.

• Chaining and awaiting in parse() introduces the risk of unhandled promise rejections, so they should not be enabled unless the user explicitly requests it.

Why is chainArgParserCalls() necessary?

• The need for the default true mode is motivated here and in the Why enable chaining and awaiting for parseAsync() by default? section.
• The need for the false mode is motivated here.

Why store and await overwritten option values?

An end user might repeat an option even if it is not supposed to be repeated, i.e. if the parser ignores the old value supplied to it in the second parameter. If chainArgParserCalls(false) had been called on the option, this can result in unhandled promise rejections if overwritten option values are not awaited.

Why pass handleError to _parseArg() and not just the error message for invalid argument errors?

It seems to be more or less an arbitrary design decision to give invalid argument errors thrown from option-argument and command-argument parsers the same .code values ('commander.invalidArgument'). They could just as well be different, or there could theoretically be other specially handled errors specific only to option-argument parsers or only to command-argument parsers, so it just does not seem right for _parseArg() to take over the error handling.

Why use the word "await" and not "settle"?

To settle a promise means to fulfill or to reject it. That happens internally in the promise callback, an observer can only await the promise.

Peer PRs

…solving similar problems

• React to wrong library usage #1917 (introduces warnings for async hook and action calls in parse() in addition to the warnings for async argParser calls introduced here)

Warnings need to be consistent with…

• React to wrong library usage #1917
• Warn about obscured help option #1931
• Add support for sharing and stand-alone parsing of subcommands. Warn about parse calls on commands added with .command() #1938
• Warn about async calls in .parse() #1940

Requires changes when merged with…

• Add .suppressWarnings() for warnings in #1915 #1931 #1938 #1940 #1955

See also

• Feature/async parse args experiment #1913 (differences)

Footnotes

1. _output a warning suggesting use of parseAsync() when parsing with parse() and a thenable is returned ↩ ↩² ↩³ ↩⁴ ↩⁵ ↩⁶

[shadowspawn]

Just a comment.

This is a nice logical position for processing the options just after they are defined. It does unfortunately spread the chaining logic through the top of this routine. I went for positions close to the hooks since I knew there was already chaining support for hooks.

(And processing async parseArgs for arguments when is no action handler spreads the chaining logic down through routine.)

[aweebit]

It does unfortunately spread the chaining logic through the top of this routine.

But on the other hand, it unifies the option value awaiting for parent and leaf commands, and _dispatchSubcommand() can remain unchanged. Also, it is actually quite nice to have the chain declaration at the very top and return chain at the bottom, that kind of sets the logical frame for the function.

[shadowspawn]

Chaining and awaiting in parse() introduces the risk of unhandled promise rejections, so they should not be enabled unless the user explicitly requests it.

I am not sure what scenario you have in mind here. Is this when the user has written their own async handling which the built-in chaining would effectively bypass?

[shadowspawn]

Why use the word "await" and not "settle"?

I laughed when I saw this, I have used "settle" in a couple of dubious places. Still getting terminology straight.

[shadowspawn]

The need for the default undefined mode is motivated by the fact we want the mode to be inherited by subcommands by default.

The way Commander usually does this is copy "common" settings to subcommands as they are added. The routine that does the work is copyInheritedSettings. This is a bit clunky but in practice works quite well.

(This might simplify some of your tri-state behaviours down to two.)

[aweebit]

Chaining and awaiting in parse() introduces the risk of unhandled promise rejections, so they should not be enabled unless the user explicitly requests it.

I am not sure what scenario you have in mind here. Is this when the user has written their own async handling which the built-in chaining would effectively bypass?

I mean the user should not rely on the library for async handling and instead ideally always have their own when calling parse() since there is no way to catch errors thrown from async parsers. That's why not chaining and not awaiting in parse() calls is the reasonable default, but the user still has the freedom to rely on the library by calling await(true) which might be useful if they know the parsers will never throw.

[aweebit]

The need for the default undefined mode is motivated by the fact we want the mode to be inherited by subcommands by default.

The way Commander usually does this is copy "common" settings to subcommands as they are added. The routine that does the work is copyInheritedSettings. This is a bit clunky but in practice works quite well.

(This might simplify some of your tri-state behaviours down to two.)

If we only have the true and false modes for chainArgParserCalls() and await() (with true modes being the default ones), no chaining and awaiting should be done in parse() calls in the respective true modes for reasons explained in Why not enable chaining and awaiting for parse() by default? and in my previous comment.

There are following problems with only having those two modes:

• Chaining and awaiting cannot be enforced in parse() calls.

  That is not too bad, just weird that even when chainArgParserCalls(true) / await(true) is called explicitly, it is silently ignored when parse() is called.

• The await mode is not inherited automatically by explicitly constructed commands added with addCommand().

  Now that is pretty bad since disabling awaiting for all descendant commands is not as simple as running await(false) on the top-level command anymore.

  What is even worse is that even if we were to use copyInheritedSettings() in user code, it would only work at one level:

  Code snippet

  program.await(false);
  
  const sub = new Command('sub');
  const subsub = sub.command('subsub')
    .argument('arg', 'desc', async() => 123)
    .action(arg => console.log(arg));
  
  console.log(subsub._await); // true
  
  program.addCommand(sub);
  sub.copyInheritedSettings(program); // sub._await = program._await;
  
  console.log(subsub._await); // still true
  
  // 123 is logged instead of promise
  program.parseAsync(['sub', 'subsub', 'abc'], { from: 'user' });

  So the only correct way to make all descendant commands inherit the await mode would be this:

  Code snippet

  function deepInheritAwait(command) {
    command.commands.forEach(subcommand => {
      subcommand._await = command._await;
      deepInheritAwait(subcommand);
    });
  }
  
  deepInheritAwait(program);

  And then there would also be no way for a subcommand to overwrite the inherited mode. That is just awful.

[aweebit]

Update based on the answer to #1916

Yes. The intended pattern is set the global properties on the program before adding subcommands

Is adding subsubcommands only after the subcommand was added to its parent also the intended pattern? If so, that would ruin my reasoning in the last 2 comments, but then it definitely needs to be mentioned somewhere in the docs.

Update: only relevant for explicitly constructed subcommands added with addCommand() for which there is already a warning in the docs that they don't automatically inherit settings, so there is no additional warning needed.

[shadowspawn]

Is adding subsubcommands only after the subcommand was added to its parent also the intended pattern

Yes. In particular if you have changed any inherited default configuration on the root command, then need to do in this order to get settings propagated.

(With default settings, I expect can construct in any order.)

[aweebit]

TODO

• Move async argParser warning to _parseArg()
• Warn about thenable implicit and default option values and thenable default argument values in parse() calls.
• Place async option argParser / thenable option value warnings in setOptionValueWithSource()
• Place async argument argParser / thenable argument value warnings in _processArguments()

[aweebit]

Now has to be merged together with #1917 for consistent async warnings in parse() calls.

[shadowspawn]

It seems to be more or less an arbitrary design decision to give invalid argument errors thrown from option-argument and command-argument parsers the same .code values ('commander.invalidArgument').

For interest, the rationale for that design decision was to allow an author's custom parser to be used with both options and command-arguments. Like say myParseScalar().

#1508

[shadowspawn]

This was an epic journey from a few extra lines of code to a deep exploration of async implementation! Special mention of lots of explanation and lots of unit tests. I learnt more about async while reviewing and trying out these ideas, especially the error handling.

I do not want to proceed with this at this time. My reservations are:

• not much requested
• complicates _parseCommand which is already a long complicated routine
• subtle and complicated async error handling to maintain
• requires much more use of async chaining in the implementation (e.g. _chainOrCall)
• 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.
• 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).

I think simple author implemented async support for a limited scenario could make a nice example file. Like in #1900 (comment)