-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Changes from 7 commits
97fda7c
ac955dc
0553d32
55b9146
dfe2fc7
3572657
adde7ff
8ca1459
c958d2b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,8 @@ const { suggestSimilar } = require('./suggestSimilar'); | |
|
||
// @ts-check | ||
|
||
const PRODUCTION = process.env.NODE_ENV === 'production'; | ||
|
||
class Command extends EventEmitter { | ||
/** | ||
* Initialize a new `Command`. | ||
|
@@ -265,6 +267,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(`Command '${cmd.name()}' passed to .addCommand() already has a parent | ||
- subcommands cannot be shared`); | ||
} | ||
|
||
opts = opts || {}; | ||
if (opts.isDefault) this._defaultCommandName = cmd._name; | ||
|
@@ -887,6 +893,27 @@ 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) { | ||
const methodName = async ? 'parseAsync' : 'parse'; | ||
if (!PRODUCTION && this.parent) { | ||
console.warn(`Called .${methodName}() on subcommand '${this._name}'. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It seems a little weird to add the error in
No, just trying on the library developer mindset and thinking of different ways it could be used 🙂
More shared code can be added to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The high level author error that does sometimes happen is chucking
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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about we propagate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The suggestion from the first comment would also become irrelevant. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As of now, there are no meaningful semantics / intended usage patterns for There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
Call on top-level command instead`); | ||
} | ||
|
||
const userArgs = this._prepareUserArgs(argv, parseOptions); | ||
return userArgsCallback(userArgs); | ||
} | ||
|
||
/** | ||
* Parse `argv`, setting options and invoking commands when defined. | ||
* | ||
|
@@ -905,10 +932,14 @@ Expecting one of '${allowedValues.join("', '")}'`); | |
*/ | ||
|
||
parse(argv, parseOptions) { | ||
const userArgs = this._prepareUserArgs(argv, parseOptions); | ||
this._parseCommand([], userArgs); | ||
|
||
return this; | ||
return this._parseSubroutine(false, (userArgs) => { | ||
const result = this._parseCommand([], userArgs); | ||
if (!PRODUCTION && isThenable(result)) { | ||
console.warn(`.parse() is incompatible with async hooks and actions. | ||
Use .parseAsync() instead.`); | ||
} | ||
return this; | ||
}, argv, parseOptions); | ||
} | ||
|
||
/** | ||
|
@@ -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); | ||
} | ||
|
||
/** | ||
|
@@ -1188,8 +1219,7 @@ 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()); | ||
} | ||
|
@@ -2193,4 +2223,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; |
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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 usingparse()
.