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

Warn about async calls in .parse() #1940

Closed
wants to merge 2 commits into from

Conversation

aweebit
Copy link
Contributor

@aweebit aweebit commented Aug 5, 2023

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…

Requires changes when merged with…

@shadowspawn
Copy link
Collaborator

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
Copy link
Contributor Author

aweebit commented Aug 11, 2023

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
Copy link
Contributor Author

aweebit commented Aug 11, 2023

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

aweebit added a commit to aweebit/commander.js that referenced this pull request Aug 11, 2023
aweebit added a commit to aweebit/commander.js that referenced this pull request Aug 11, 2023
@shadowspawn
Copy link
Collaborator

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
Copy link
Contributor Author

aweebit commented Aug 12, 2023

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
Copy link
Collaborator

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
Copy link
Contributor Author

aweebit commented Aug 13, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants