[Feature Request] Pass Command instance to argParsers

[aweebit]

Since #1915 has been closed, I have been working on a third-party library that would offer similar functionality by means of subclassing.

The most promising approach seems to be to overwrite the .parseArg property of Option and Argument instances whenever it receives a new value, so something like this:

/**
 * @param {typeof Option | typeof Argument} Base
 */
function ParseArgMixin(Base) {
  return class extends Base {
    #originalArgParser = undefined; // argParser provided by library user

    #parseArg = (value, previous) => {
      // This function is the one
      // that will overwrite the argParser provided by library user,
      // but it will still call it internally.
    };

    #overwriteParseArg() {
      this.#originalArgParser = this.parseArg;
      this.parseArg = this.#parseArg;
    }

    argParser(fn) {
      super.argParser(fn);
      this.#overwriteParseArg();
      return this;
    }
  };
}

However, there is one problem when it comes to catching errors in promise chains.

I would like to handle such errors in the same manner errors in synchronous argParsers are currently handled, and that involves calling the .error() method on the Command instance if the error turns out to be an InvalidArgumentError. However, that is not possible because neither Option nor Argument instances have access to this method.

That is why I suggest to extend the argParser signature by a third parameter whose value will always be the Command instance in which the argParser was called.

  argParser<T>(fn: (value: string, previous: T, command: Command) => T): this;

Another reaason why that could come in handy in my scenario is because it would make it possible to only treat promises specially when .parseAsync() has been called, e.g. by reading the _asyncParsing property just like in #1915.

Regular users could also benefit from this change since it would enable them to write reusable argParsers with custom error messages (which they might want to do if they are not satisfied with the default ones for whatever reason). Or they could retrieve the version number via .version() and include it in the error message. Or they could even choose to call .help() instead of .error(). There is a lot that could be done.

[shadowspawn]

There was a previous request to add an option|argument as well: #1207

The additions of .choices() and throwing InvalidArgumentError covered many of the common use cases for custom parsers.

[aweebit]

There was a previous request to add an option|argument as well: #1207

The additions of .choices() and throwing InvalidArgumentError covered many of the common use cases for custom parsers.

Happy to see I'm not the only one who thought of this. The circumstances have changed, sure, and now the use cases for this are much narrower, but there certainly still are some, especially for developers of third party tools who cannot know what Command instance called the argParser even if they subclass Command, too.

[aweebit]

a previous request to add an option|argument

Shouldn't the Option / Argument instance be available in this unless the argParser is an anonymous function? If so, adding them as parameters seems redundant to me.

[shadowspawn]

Anonymous functions are so common for these sorts of functions that I treat them as a first class use case. (And tend to forget this is even possible. 🤭 )

Could you achieve access to Command currently by adding parent command to option in createOption and passing it as additional parameter in subclass implementation of parseArg?

[aweebit]

Anonymous functions are so common for these sorts of functions that I treat them as a first class use case. (And tend to forget this is even possible. 🤭 )

They are also common for action handlers, but in my CLI tool based on Commander, I always preferred regular function() {} and this over the alternative (opts, command) => {}. Could pass this to argParsers in the last, fourth parameter for consistency, though.

Could you achieve access to Command currently by adding parent command to option in createOption…

Unlike commands, Option and Argument instances are reusable even in the current library version, so no, I don't think adding a command property and setting it in .createOption() / .createArgument() is a good idea. It could of course be set in .parse() and .parseAsync() for all registered options and arguments, but man would that be annoying! I would also like to use private fields for pretty much everything except .chainArgParserCalls() taken from #1915, but the command property would have to be public becauseCommand instances would not be able to control it otherwise.

…and passing it as additional parameter in subclass implementation of parseArg?

Not sure what you mean here. If we store the Command instance in a property, we don't need to pass it to parseArg anymore. But if we don't store the instance and so do need to pass it, and if it is decided against implementing my suggestion, then the Command methods calling .parseArg() (.addOption() and ._processArguments()) would have to be overridden by methods with entire implementations copied from the library source just to change that one .parseArg() call, which is just awful.

[shadowspawn]

Could you achieve access to Command currently by adding parent command to option in createOption and passing it as additional parameter in subclass implementation of parseArg?

I wanted to try this out.

const { Command, Option } = require('commander');

class MyOption extends Option {
  #owner;

  setOwner(cmd) {
    this.#owner = cmd;
  }

  argParser(fn) {
    this.parseArg = (arg, previous) => {
      return fn(arg, previous, this.#owner);
    };
    return this;
  }
}

class MyCommand extends Command {
  createCommand(name) {
    return new MyCommand(name);
  }

  createOption(flags, description) {
    const option = new MyOption(flags, description);
    option.setOwner(this);
    return option;
  }
}

function myParseInspect(arg, previous, cmd) {
  console.log(`Parsing arg: ${arg}`);
  console.log(`Called parse for an option on ${cmd.name()}`);
}

const program = new MyCommand('myProgram')
   .option('-v, --value <VALUE>', 'enter description', myParseInspect);

program.parse();

% node index.js -v XXX 
Parsing arg: XXX
Called parse for an option on myProgram

[aweebit]

@shadowspawn this works, but only for implicitly constructed options added with .option() / .requiredOption() that are not shared by commands (very unlikely to happen, but theoretically possible, e.g. when all options from the original command are copied to a new one).

Also, I don't really want to expose the Command instance in the user-supplied argParsers unless Commander does that, or otherwise I might have to change the order of parameters if Commander decides to add it later but with a different order (or use the third parameter for something completely different).

It could of course be set in .parse() and .parseAsync() for all registered options and arguments, but man would that be annoying!

I ended up implementing this idea for now, take a look at aweebit/recommander if you are interested. The code is a bit involved, but you could just Ctrl+F __recommander_command.

Would still love this issue to be acted on, though.

[shadowspawn]

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

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

Thank you for your contributions.