React to wrong library usage

[aweebit]

Update: Ideally, this PR should be superseded by #1938 and #1940. See comment for details.

Partially fixes #1916. I think it is a good idea to react to wrong library uses even if they have not been featured in many issues.

Why console.warn() and not throw from within parse() / parseAsync()?

Errors thrown from parse() / parseAsync() are expected to have originated in user-supplied code (argParsers, hooks, actions).

An alternative could be to console.error() and process.exit() with a non-zero exit code, effectively forbidding all wrong usage, but what I don't like is the discrepancy between this approach and how all other library errors are simply thrown and can be handled by the user.

ChangeLog

Added

• Breaking: throw an error when trying to add a subcommand that already has a parent command
• warnings about parse calls on subcommands
• warnings about async hook and action calls made in .parse()

Peer PRs

Warnings need to be consistent with…

• Add async custom processing support. Add chainArgParserCalls() for configuration. Additionally await thenable implicit and default option values and thenable default argument values #1915
• 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

Parse call subroutine (_parseSubroutine()) needs to be consistent with…

• Clean up state from previous parse call when calling parse() / parseAsync() #1919

Incompatible with…

• Add .suppressWarnings() for warnings in #1915 #1931 #1938 #1940 #1955: parse methods shall never be called on subcommands, so if warnings about such calls are added, the JSDoc and the ChangeLog entry for .suppressWarnings() introduced there become incorrect. The fact there is currently no well-defined meaning for such calls is one of the reasons why #1938 and #1940 should be preferred over this PR (#1917)!

[aweebit]

Idea: allow adding a subcommand that already has a parent to a different parent and console.warn() about incorrect usage if parse() / parseAsync() is called on the old parent. That might be better because

• no breaking change is introduced
• there is nothing bad about reusing a subcommand as long as the old parent is no longer used.

But since we would be doing parent checks in parse() / parseAsync() anyway, we could just as well set the parent property for all subcommands in this.commands not upon adding them with command() / addCommand(), but in parse() / parseAsync() instead. That would change the expected behavior mentioned in #1916 (comment) and enable sharing and stand-alone use of subcommands. Update: This suggestion conflicts with #1924 (bd12c90), see comment (Update: there is no conflict if all parent commands are stored, see #1938).

[shadowspawn]

A comment. There are about four different independent things here. I struggle to avoid adding extra things myself while on a roll making code changes. But as a reviewer, I dislike it!

[shadowspawn]

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?

[aweebit]

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().

[shadowspawn]

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.

[aweebit]

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.

[shadowspawn]

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();

[aweebit]

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.

[aweebit]

The suggestion from the first comment would also become irrelevant.

[shadowspawn]

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.

[aweebit]

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).

[aweebit]

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().

[aweebit]

A comment. There are about four different independent things here. I struggle to avoid adding extra things myself while on a roll making code changes. But as a reviewer, I dislike it!

Okay, I will take that into account next time.

[aweebit]

(Force push is due to a hard reset needed because I committed on a wrong branch.)

[aweebit]

Now, I would like this PR to be superseded by #1940 (the warning part taken from this PR) and #1938 (implementing a better version of the changes suggested in comment and comment).

Due to the support for sharing and stand-alone parsing of subcommands introduced in #1938, fewer use cases would be considered wrong and there would be need for only one new warning, instead of two new warnings and one new error like in this PR.

[aweebit]

Since #1940 is closed now, I could also remove the warning about async calls from this PR, but I don't really want to change anything here because I strongly advocate for this PR being superseded by #1938.

[shadowspawn]

Related: https://stackoverflow.com/questions/76896860/test-suite-fails-when-commander-error-is-thrown-in-jest-even-with-catch

[aweebit]

Related: https://stackoverflow.com/questions/76896860/test-suite-fails-when-commander-error-is-thrown-in-jest-even-with-catch

Related to the now-closed #1940 component of this PR, to be more more specific. I couldn't ask for a better demonstration of why closing it was a mistake.

Also, the comment you left there is related to #1922: accepting the PR would make it easier to copy settings to an entire command hierarchy when it can only be done after subcommands were added.

[shadowspawn]

There are multiple alternative preferred PRs by same author, and this PR was somewhat of a learning experience. In particular #1940 has been rejected, for now.

I think it is a good idea to react to wrong library uses even if they have not been featured in many issues.

I think this is an easy approach in a new library, as users can be directed to the intended use patterns. I move more slowly and conservatively in a mature library, where every change may break an upgrade for some existing users who may or may not appreciate being forced to change their "working" program.