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

React to wrong library usage #1917

Closed
wants to merge 9 commits into from
2 changes: 2 additions & 0 deletions Readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,8 @@ subcommand is specified ([example](./examples/defaultCommand.js)).

You can add alternative names for a command with `.alias()`. ([example](./examples/alias.js))

Commands added with `.command()` automatically inherit settings for which inheritance is meaningful from the parent command, but only upon the subcommand creation. The setting changes made after calling `.command()` are not inherited.
aweebit marked this conversation as resolved.
Show resolved Hide resolved

For safety, `.addCommand()` does not automatically copy the inherited settings from the parent command. There is a helper routine `.copyInheritedSettings()` for copying the settings when they are wanted.

### Command-arguments
Expand Down
68 changes: 58 additions & 10 deletions lib/command.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ const { suggestSimilar } = require('./suggestSimilar');

// @ts-check

const PRODUCTION = process.env.NODE_ENV === 'production';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm. I don't think PRODUCTION will often be relevant for a CLI? I get the desire to avoid spamming an end-user who can't fix the problem directly. But I don't think Commander has a strong enough production/non-production case to make the distinction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't used NODE_ENV in CLI applications myself, but thought that having this distinction wouldn't harm, so why not add it. It gives the developer the freedom to ignore the warnings in production, which might be meaningful after #1915 is merged so that the developer can insist on manually handling thenable argument and option values by using parse().


class Command extends EventEmitter {
/**
* Initialize a new `Command`.
Expand Down Expand Up @@ -77,6 +79,9 @@ class Command extends EventEmitter {
this._helpCommandnameAndArgs = 'help [command]';
this._helpCommandDescription = 'display help for command';
this._helpConfiguration = {};

/** @type {boolean | undefined} */
this._asyncParsing = undefined;
}

/**
Expand Down Expand Up @@ -265,6 +270,10 @@ class Command extends EventEmitter {
throw new Error(`Command passed to .addCommand() must have a name
- specify the name in Command constructor or using .name()`);
}
if (cmd.parent) {
throw new Error(`'${cmd._name}' cannot be added using .addCommand()
aweebit marked this conversation as resolved.
Show resolved Hide resolved
- it already has a parent command`);
}

opts = opts || {};
if (opts.isDefault) this._defaultCommandName = cmd._name;
Expand Down Expand Up @@ -887,6 +896,28 @@ Expecting one of '${allowedValues.join("', '")}'`);
return userArgs;
}

/**
* @param {boolean} async
* @param {Function} userArgsCallback
* @param {string[]} [argv]
* @param {Object} [parseOptions]
* @param {string} [parseOptions.from]
* @return {Command|Promise}
* @api private
*/

_parseSubroutine(async, userArgsCallback, argv, parseOptions) {
this._asyncParsing = async;
const methodName = async ? 'parseAsync' : 'parse';
if (!PRODUCTION && this.parent) {
console.warn(`Called .${methodName}() on subcommand '${this._name}'.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think I care enough about this error to add. For interest, did you hit this in real world usage?

If I did add it, I think I would rather have some small code repetition than an extra level of code.

Copy link
Contributor Author

@aweebit aweebit Jul 29, 2023

Choose a reason for hiding this comment

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

I don't think I care enough about this error to add.

It seems a little weird to add the error in addCommand() but not this one. But everything could be fixed by implementing the suggestion from the first comment. Why do you dislike it? I think it is very easy to implement. If I'm not mistaken, it would require even fewer lines of code than the combination of this warning and the error in addCommand() (cannot check it right now unfortunately since I'm on a small trip and haven't taken my laptop with me). Only the error from passThroughOptions() would need to be turned into a warning in _parseSubroutibe(), and something done about the parent use in outputHelp() (maybe should just leave the parent assignment in command() / addCommand()).

For interest, did you hit this in real world usage?

No, just trying on the library developer mindset and thinking of different ways it could be used 🙂

If I did add it, I think I would rather have some small code repetition than an extra level of code.

More shared code can be added to _parseSubroutibe() later, like after the suggestion from the first comment is implemented or #1919 is merged.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The high level author error that does sometimes happen is chucking parse() on the end of the subcommand definition. This would catch that particular case. It does not come up often and I wonder if there are valid use cases for calling .parse() down tree in fancy custom programs.

const { program } = require('commander')
   .exitOverride()
   .option('-g, --global')
   .command('sub')
   .option('--sub-option')
   .parse(); // oops

To encourage a usage pattern that avoids several potential problems, I use this style as much as possible in the hopes people will just copy it:

  • define program
  • configure program
  • new statement per subcommand
  • call .parse()
import { program } from 'commander';

program
   .exitOverride()
   .option('-g, --global');

program.command('sub')
   .option('--sub-option');

program.parse();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about we propagate parse() / parseAsync() calls to the root command? Then chucking a call at the end of a subcommand definition would not be a problem, and we would not need the warning since subcommands would not be usable as stand-alones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The suggestion from the first comment would also become irrelevant.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm... No, I think not. Either the user is intentionally doing something interesting outside normal usage, or they are confused about how Commander works. Doing something different that might be what they intended is masking the situation.

Copy link
Contributor Author

@aweebit aweebit Jul 29, 2023

Choose a reason for hiding this comment

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

As of now, there are no meaningful semantics / intended usage patterns for parse() / parseAsync() calls on subcommands anyway, and any such call should be considered wrong. The only meaningful semantics for such a call I can think of are exactly that, propagating it to the root command. So why not assign a meaning to calls that currently have no meaning at all, giving the developer the freedom to chuck a parse call at the end of a subcommand chain? That is not doing something different that might be what they intended, that is doing the only meaningful thing when receiving such a call instead of doing complete nonsense (what the library currently does) or having to come up with ways to tell the user they are using the library in a wrong way (what this PR is currently all about).

Copy link
Contributor Author

@aweebit aweebit Jul 31, 2023

Choose a reason for hiding this comment

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

Two days later, I am having second thoughts about my comment on the suggestion from the first comment becoming irrelevant. I think I was wrong saying that, it will still be relevant because it deals with parse calls on a parent command that has lost ownership of a subcommand, rather than on a subcommand. So I would still like to hear why you disliked that suggestion. Implementing it would make the warning this thread is about unnecessary since parsing subcommands as stand-alones would become possible. Could still leave the warning for the case when a parse call is chucked a the end of a subcommand chain, though, for example by adding a _parseCallsUndesired field with the default value of false to the Command class and setting it to true for subcommands created in comnand().

Call on top-level command instead`);
}

const userArgs = this._prepareUserArgs(argv, parseOptions);
return userArgsCallback(userArgs);
}

/**
* Parse `argv`, setting options and invoking commands when defined.
*
Expand All @@ -905,10 +936,10 @@ Expecting one of '${allowedValues.join("', '")}'`);
*/

parse(argv, parseOptions) {
const userArgs = this._prepareUserArgs(argv, parseOptions);
this._parseCommand([], userArgs);

return this;
return this._parseSubroutine(false, (userArgs) => {
this._parseCommand([], userArgs);
return this;
}, argv, parseOptions);
}

/**
Expand All @@ -931,10 +962,10 @@ Expecting one of '${allowedValues.join("', '")}'`);
*/

async parseAsync(argv, parseOptions) {
const userArgs = this._prepareUserArgs(argv, parseOptions);
await this._parseCommand([], userArgs);

return this;
return this._parseSubroutine(true, async(userArgs) => {
await this._parseCommand([], userArgs);
return this;
}, argv, parseOptions);
}

/**
Expand Down Expand Up @@ -1071,6 +1102,7 @@ Expecting one of '${allowedValues.join("', '")}'`);
_dispatchSubcommand(commandName, operands, unknown) {
const subCommand = this._findCommand(commandName);
if (!subCommand) this.help({ error: true });
subCommand._asyncParsing = this._asyncParsing;

let hookResult;
hookResult = this._chainOrCallSubCommandHook(hookResult, subCommand, 'preSubcommand');
Expand Down Expand Up @@ -1189,12 +1221,18 @@ Expecting one of '${allowedValues.join("', '")}'`);

_chainOrCall(promise, fn) {
// thenable
if (promise && promise.then && typeof promise.then === 'function') {
if (isThenable(promise)) {
// already have a promise, chain callback
return promise.then(() => fn());
}

// callback might return a promise
return fn();
const result = fn();
if (!PRODUCTION && !this._asyncParsing && isThenable(result)) {
aweebit marked this conversation as resolved.
Show resolved Hide resolved
console.warn(`.parse() is incompatible with async hooks and actions.
Use .parseAsync() instead.`);
}
return result;
}

/**
Expand Down Expand Up @@ -2193,4 +2231,14 @@ function getCommandAndParents(startCommand) {
return result;
}

/**
* @param {*} value
* @returns {boolean}
* @api private
*/

function isThenable(value) {
shadowspawn marked this conversation as resolved.
Show resolved Hide resolved
return typeof value?.then === 'function';
}

exports.Command = Command;