Warn about async calls in .parse()

[aweebit]

Partially fixes #1916.

Cherry-picked from #1917 for better separation of concerns and because the change is the only change introduced there that can be combined meaningfully with #1938.

I would like this PR and #1938 to supersede #1917. See #1917 (comment) for details.

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

An alternative could be to console.error() and process.exit() with a non-zero exit code, effectively forbidding the 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

• 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
• 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

Requires changes when merged with…

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

[shadowspawn]

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

Error vs warning has been mentioned in a couple of PR. I'll explain my mental model:

• detected mistakes by the author throw an explanation for author and should not be seen by end-user
• end-user mistakes display a message and exit the program (by default)

[aweebit]

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

Error vs warning has been mentioned in a couple of PR. I'll explain my mental model:

• detected mistakes by the author throw an explanation for author and should not be seen by end-user
• end-user mistakes display a message and exit the program (by default)

Here is the modified model I am suggesting:

• suspicious library usage patterns (although not always entirely wrong – that applies to all the warnings currently suggested in my PRs except the warning about parse calls on subcommands in #1917, which is one of the reasons I want it superseded): assume author mistake and show a warning unless in production mode the author could switch to in order to prevent warnings from being shown to end-users
• end-user mistakes including arguments deemed invalid by argParsers: display an error message and exit the program (by default)
• other errors originating in callbacks (argParsers, hooks, actions): throw the error for the author to handle – should not be seen by end-user

[aweebit]

Actually, not sure about using an environment variable anymore because that is not something for the author to control. A probably better idea would be to introduce something like Command.suppressWarnings().

Update: Yeah, makes zero sense for a tool like webpack-cli that depends on Commander to show warnings that are meant for the tool authors, but only when NODE_ENV (that has an entirely different meaning in webpack-cli and is supposed to be controlled by the end-user) is set to 'production'.

[shadowspawn]

I have wondered about a warning or error for thenable in .parse() when adding .parseAsync() and ever since. I do think it is best practice, but still not sure it is justified for Commander to be opinionated and demand .parseAsync() when thenable, particularly since it is detected at runtime depending on end-user usage.

I went through all the issues mentioning .parseAsync(). Somewhat ironically, there were zero issues supporting a warning but one wondering whether .parseAysnc() was actually required! #1681 🤭

For historical interest, the issues predating async support got 15 upvotes: #806

Closing for now and keep watching issues for evidence this is worthwhile.

[aweebit]

I went through all the issues mentioning .parseAsync(). Somewhat ironically, there were zero issues supporting a warning but one wondering whether .parseAysnc() was actually required! #1681 🤭

The warning could prevent developers like the author of the issue you linked to from neglecting .parseAsync() because .parse() seems to work fine, which it actually does until an error is thrown from an async callback.

It could also help forgetful developers avoid potential problems in the future when they switch to async callbacks but forget to replace .parse() with .parseAsync().

But if a developer wants to insist on using .parse() for whatever reason (justified if they know for sure the promises will never be rejected), they could do so by calling .suppressWarnings() suggested in #1955.

still not sure it is justified for Commander to be opinionated and demand .parseAsync() when thenable, particularly since it is detected at runtime depending on end-user usage.

The last part is only true if a callback returns thenables in some cases and non-thenables in others, which is definitely not common.

[shadowspawn]

still not sure it is justified for Commander to be opinionated and demand .parseAsync() when thenable, particularly since it is detected at runtime depending on end-user usage.

The last part is only true if a callback returns thenables in some cases and non-thenables in others, which is definitely not common.

I was worried about some subset of commands being async. Perhaps that is unlikely, and hence less likely developer fails to notice before end-users encounter the change in behaviour.

Still don't want to add it right now, but closer...

[aweebit]

I was worried about some subset of commands being async. Perhaps that is unlikely, and hence less likely developer fails to notice before end-users encounter the change in behaviour.

Oh you are right, and I don't even think that's unlikely, could absolutely be the case in a program that is a collection of different utility commands, or there could be a command that does something complicated asynchronously and, at the same time, one that simply logs some static information (similar to a version command).

But I do think it's unlikely the developer fails to notice the warning because they should test each command separately before making the program available to the end-user.