From 2c6bcf41e9360a589c5f5eb2af270587875f80da Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Tue, 25 Jul 2023 11:14:19 +0200 Subject: [PATCH 01/31] Add thenable function Co-authored-by: John Gee --- lib/command.js | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/lib/command.js b/lib/command.js index 590a271dd..988aa0c34 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1188,8 +1188,7 @@ Expecting one of '${allowedValues.join("', '")}'`); */ _chainOrCall(promise, fn) { - // thenable - if (promise && promise.then && typeof promise.then === 'function') { + if (thenable(promise)) { // already have a promise, chain callback return promise.then(() => fn()); } @@ -2193,4 +2192,14 @@ function getCommandAndParents(startCommand) { return result; } +/** + * @param {*} value + * @returns {boolean} + * @api private + */ + +function thenable(value) { + return typeof value?.then === 'function'; +} + exports.Command = Command; From 9f292ad2aa74ee4a8a3eadcf6b4ec0d56c1c260c Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Tue, 25 Jul 2023 13:56:48 +0200 Subject: [PATCH 02/31] Chain parseArg calls when parseAsync() is called The behavior can be configured via chainArgParserCalls() --- lib/argument.js | 14 +++ lib/command.js | 124 ++++++++++++++++++----- lib/option.js | 14 +++ tests/argument.chain.test.js | 6 ++ tests/argument.custom-processing.test.js | 28 +++++ tests/command.argumentVariations.test.js | 18 ++-- tests/option.chain.test.js | 6 ++ tests/options.custom-processing.test.js | 46 +++++++++ typings/index.d.ts | 12 +++ typings/index.test-d.ts | 12 +++ 10 files changed, 250 insertions(+), 30 deletions(-) diff --git a/lib/argument.js b/lib/argument.js index c16430250..4ed6e1f80 100644 --- a/lib/argument.js +++ b/lib/argument.js @@ -16,6 +16,8 @@ class Argument { this.description = description || ''; this.variadic = false; this.parseArg = undefined; + /** @type {boolean | undefined} */ + this.chained = undefined; this.defaultValue = undefined; this.defaultValueDescription = undefined; this.argChoices = undefined; @@ -89,6 +91,18 @@ class Argument { return this; } + /** + * When set to true, next call to the function provided via .argParser() will be chained to its return value if it is thenable. + * When set to undefined (the default), only chain when .parseAsync() has been called. + * + * @param {boolean | undefined} [chained] + * @return {Argument} + */ + chainArgParserCalls(chained) { + this.chained = chained === undefined ? undefined : !!chained; + return this; + } + /** * Only allow argument value to be one of choices. * diff --git a/lib/command.js b/lib/command.js index 988aa0c34..eecff5553 100644 --- a/lib/command.js +++ b/lib/command.js @@ -57,6 +57,9 @@ class Command extends EventEmitter { this._showHelpAfterError = false; this._showSuggestionAfterError = true; + /** @type {boolean | undefined} */ + this._asyncParsing = undefined; + // see .configureOutput() for docs this._outputConfiguration = { writeOut: (str) => process.stdout.write(str), @@ -275,6 +278,67 @@ class Command extends EventEmitter { return this; } + /** + * @param {Argument|Option} target + * @return {boolean} + * @api private + */ + + _shouldChainArgParserCalls(target) { + return target.chained || (target.chained === undefined && this._asyncParsing); + } + + /** + * @param {Argument|Option} target + * @param {*} value + * @param {Function} handleError + * @api private + */ + + _catchChainError(target, value, handleError) { + if (this._shouldChainArgParserCalls(target) && thenable(value)) { + return value.then(x => x, handleError); + } + + return value; + } + + /** + * Internal implementation shared by ._processArguments() and option and optionEnv event listeners. + * + * @param {Argument|Option} target + * @param {*} value + * @param {*} previous + * @param {Function} handleError + * @return {*} + * @api private + */ + + _parseArg(target, value, previous, handleError) { + let parsedValue; + + if (this._shouldChainArgParserCalls(target) && thenable(previous)) { + // chain thenables + parsedValue = previous.then( + (result) => { + result = target.parseArg(value, result); + // call with result and not parsedValue to not catch handleError exception repeatedly + result = this._catchChainError(target, result, handleError); + return result; + } + ); + } else { + try { + parsedValue = target.parseArg(value, previous); + parsedValue = this._catchChainError(target, parsedValue, handleError); + } catch (err) { + handleError(err); + } + } + + return parsedValue; + } + /** * Factory routine to create a new unattached argument. * @@ -526,6 +590,14 @@ Expecting one of '${allowedValues.join("', '")}'`); // handler for cli and env supplied values const handleOptionValue = (val, invalidValueMessage, valueSource) => { + const handleError = (err) => { + if (err?.code === 'commander.invalidArgument') { + const message = `${invalidValueMessage} ${err.message}`; + this.error(message, { exitCode: err.exitCode, code: err.code }); + } + throw err; + }; + // val is null for optional option used without an optional-argument. // val is undefined for boolean and negated option. if (val == null && option.presetArg !== undefined) { @@ -535,15 +607,7 @@ Expecting one of '${allowedValues.join("', '")}'`); // custom processing const oldValue = this.getOptionValue(name); if (val !== null && option.parseArg) { - try { - val = option.parseArg(val, oldValue); - } catch (err) { - if (err.code === 'commander.invalidArgument') { - const message = `${invalidValueMessage} ${err.message}`; - this.error(message, { exitCode: err.exitCode, code: err.code }); - } - throw err; - } + val = this._parseArg(option, val, oldValue, handleError); } else if (val !== null && option.variadic) { val = option._concatValue(val, oldValue); } @@ -905,10 +969,16 @@ Expecting one of '${allowedValues.join("', '")}'`); */ parse(argv, parseOptions) { - const userArgs = this._prepareUserArgs(argv, parseOptions); - this._parseCommand([], userArgs); + this._asyncParsing = false; - return this; + try { + const userArgs = this._prepareUserArgs(argv, parseOptions); + this._parseCommand([], userArgs); + + return this; + } finally { + this._asyncParsing = undefined; + } } /** @@ -931,10 +1001,16 @@ Expecting one of '${allowedValues.join("', '")}'`); */ async parseAsync(argv, parseOptions) { - const userArgs = this._prepareUserArgs(argv, parseOptions); - await this._parseCommand([], userArgs); + this._asyncParsing = true; - return this; + try { + const userArgs = this._prepareUserArgs(argv, parseOptions); + await this._parseCommand([], userArgs); + + return this; + } finally { + this._asyncParsing = undefined; + } } /** @@ -1134,18 +1210,18 @@ Expecting one of '${allowedValues.join("', '")}'`); _processArguments() { const myParseArg = (argument, value, previous) => { + const handleError = (err) => { + if (err?.code === 'commander.invalidArgument') { + const message = `error: command-argument value '${value}' is invalid for argument '${argument.name()}'. ${err.message}`; + this.error(message, { exitCode: err.exitCode, code: err.code }); + } + throw err; + }; + // Extra processing for nice error message on parsing failure. let parsedValue = value; if (value !== null && argument.parseArg) { - try { - parsedValue = argument.parseArg(value, previous); - } catch (err) { - if (err.code === 'commander.invalidArgument') { - const message = `error: command-argument value '${value}' is invalid for argument '${argument.name()}'. ${err.message}`; - this.error(message, { exitCode: err.exitCode, code: err.code }); - } - throw err; - } + parsedValue = this._parseArg(argument, value, previous, handleError); } return parsedValue; }; diff --git a/lib/option.js b/lib/option.js index d61fc5f2f..d3c2f0624 100644 --- a/lib/option.js +++ b/lib/option.js @@ -31,6 +31,8 @@ class Option { this.presetArg = undefined; this.envVar = undefined; this.parseArg = undefined; + /** @type {boolean | undefined} */ + this.chained = undefined; this.hidden = false; this.argChoices = undefined; this.conflictsWith = []; @@ -135,6 +137,18 @@ class Option { return this; } + /** + * When set to true, next call to the function provided via .argParser() will be chained to its return value if it is thenable. + * When set to undefined (the default), only chain when .parseAsync() has been called. + * + * @param {boolean | undefined} [chained] + * @return {Argument} + */ + chainArgParserCalls(chained) { + this.chained = chained === undefined ? undefined : !!chained; + return this; + } + /** * Whether the option is mandatory and must have a value after parsing. * diff --git a/tests/argument.chain.test.js b/tests/argument.chain.test.js index dbc7947c2..b8057f4ac 100644 --- a/tests/argument.chain.test.js +++ b/tests/argument.chain.test.js @@ -13,6 +13,12 @@ describe('Argument methods that should return this for chaining', () => { expect(result).toBe(argument); }); + test('when call .chainArgParserCalls() then returns this', () => { + const argument = new Argument(''); + const result = argument.chainArgParserCalls(); + expect(result).toBe(argument); + }); + test('when call .choices() then returns this', () => { const argument = new Argument(''); const result = argument.choices(['a']); diff --git a/tests/argument.custom-processing.test.js b/tests/argument.custom-processing.test.js index ef3d16822..453c786e4 100644 --- a/tests/argument.custom-processing.test.js +++ b/tests/argument.custom-processing.test.js @@ -205,3 +205,31 @@ test('when custom processing for argument throws plain error then not CommanderE expect(caughtErr).toBeInstanceOf(Error); expect(caughtErr).not.toBeInstanceOf(commander.CommanderError); }); + +test('when custom with .chainArgParserCalls(true) then parsed to chain', async() => { + const args = ['1', '2']; + const resolvedValue = '12'; + const coercion = async(value, previousValue) => ( + previousValue === undefined ? value : previousValue + value + ); + const awaited = coercion(args[0], undefined); + const mockCoercion = jest.fn().mockImplementation(coercion); + + const argument = new commander.Argument('', 'desc') + .argParser(mockCoercion) + .chainArgParserCalls(true); + + let actionValue; + + const program = new commander.Command(); + program + .addArgument(argument) + .action((value) => { + actionValue = value; + }); + + program.parse(args, { from: 'user' }); + expect(program.processedArgs[0]).toEqual(awaited); + expect(actionValue).toEqual(awaited); + await expect(actionValue).resolves.toEqual(resolvedValue); +}); diff --git a/tests/command.argumentVariations.test.js b/tests/command.argumentVariations.test.js index a8a781625..ad7fa3003 100644 --- a/tests/command.argumentVariations.test.js +++ b/tests/command.argumentVariations.test.js @@ -9,7 +9,8 @@ test.each(getSingleArgCases(''))('when add "" using %s t _name: 'explicit-required', required: true, variadic: false, - description: '' + description: '', + chained: undefined }; expect(argument).toEqual(expectedShape); }); @@ -20,7 +21,8 @@ test.each(getSingleArgCases('implicit-required'))('when add "arg" using %s then _name: 'implicit-required', required: true, variadic: false, - description: '' + description: '', + chained: undefined }; expect(argument).toEqual(expectedShape); }); @@ -31,7 +33,8 @@ test.each(getSingleArgCases('[optional]'))('when add "[arg]" using %s then argum _name: 'optional', required: false, variadic: false, - description: '' + description: '', + chained: undefined }; expect(argument).toEqual(expectedShape); }); @@ -42,7 +45,8 @@ test.each(getSingleArgCases(''))('when add "" usin _name: 'explicit-required', required: true, variadic: true, - description: '' + description: '', + chained: undefined }; expect(argument).toEqual(expectedShape); }); @@ -53,7 +57,8 @@ test.each(getSingleArgCases('implicit-required...'))('when add "arg..." using %s _name: 'implicit-required', required: true, variadic: true, - description: '' + description: '', + chained: undefined }; expect(argument).toEqual(expectedShape); }); @@ -64,7 +69,8 @@ test.each(getSingleArgCases('[optional...]'))('when add "[arg...]" using %s then _name: 'optional', required: false, variadic: true, - description: '' + description: '', + chained: undefined }; expect(argument).toEqual(expectedShape); }); diff --git a/tests/option.chain.test.js b/tests/option.chain.test.js index 3ed59688d..6d23bce7b 100644 --- a/tests/option.chain.test.js +++ b/tests/option.chain.test.js @@ -13,6 +13,12 @@ describe('Option methods that should return this for chaining', () => { expect(result).toBe(option); }); + test('when call .chainArgParserCalls() then returns this', () => { + const option = new Option('-e,--example '); + const result = option.chainArgParserCalls(); + expect(result).toBe(option); + }); + test('when call .makeOptionMandatory() then returns this', () => { const option = new Option('-e,--example '); const result = option.makeOptionMandatory(); diff --git a/tests/options.custom-processing.test.js b/tests/options.custom-processing.test.js index aa1022fbb..499e700cc 100644 --- a/tests/options.custom-processing.test.js +++ b/tests/options.custom-processing.test.js @@ -139,3 +139,49 @@ test('when commaSeparatedList x,y,z then value is [x, y, z]', () => { program.parse(['node', 'test', '--list', 'x,y,z']); expect(program.opts().list).toEqual(['x', 'y', 'z']); }); + +test('when custom non-variadic repeated with .chainArgParserCalls(true) then parsed to chain', async() => { + const args = ['-a', '1', '-a', '2']; + const resolvedValue = '12'; + const coercion = async(value, previousValue) => ( + previousValue === undefined ? value : previousValue + value + ); + const awaited = coercion(args[1], undefined); + const mockCoercion = jest.fn().mockImplementation(coercion); + + const option = new commander.Option('-a ', 'desc') + .argParser(mockCoercion) + .chainArgParserCalls(true); + + const program = new commander.Command(); + program + .addOption(option) + .action(() => {}); + + program.parse(args, { from: 'user' }); + expect(program.opts()).toEqual({ a: awaited }); + await expect(program.opts().a).resolves.toEqual(resolvedValue); +}); + +test('when custom variadic with .chainArgParserCalls(true) then parsed to chain', async() => { + const args = ['-a', '1', '2']; + const resolvedValue = '12'; + const coercion = async(value, previousValue) => ( + previousValue === undefined ? value : previousValue + value + ); + const awaited = coercion(args[1], undefined); + const mockCoercion = jest.fn().mockImplementation(coercion); + + const option = new commander.Option('-a ', 'desc') + .argParser(mockCoercion) + .chainArgParserCalls(true); + + const program = new commander.Command(); + program + .addOption(option) + .action(() => {}); + + program.parse(args, { from: 'user' }); + expect(program.opts()).toEqual({ a: awaited }); + await expect(program.opts().a).resolves.toEqual(resolvedValue); +}); diff --git a/typings/index.d.ts b/typings/index.d.ts index 695c3bd25..3e64e12e5 100644 --- a/typings/index.d.ts +++ b/typings/index.d.ts @@ -65,6 +65,12 @@ export class Argument { */ argParser(fn: (value: string, previous: T) => T): this; + /** + * When set to true, next call to the function provided via .argParser() will be chained to its return value if it is thenable. + * When set to undefined (the default), only chain when .parseAsync() has been called. + */ + chainArgParserCalls(chained?: boolean | undefined): this; + /** * Only allow argument value to be one of choices. */ @@ -159,6 +165,12 @@ export class Option { */ argParser(fn: (value: string, previous: T) => T): this; + /** + * When set to true, next call to the function provided via .argParser() will be chained to its return value if it is thenable. + * When set to undefined (the default), only chain when .parseAsync() has been called. + */ + chainArgParserCalls(chained?: boolean | undefined): this; + /** * Whether the option is mandatory and must have a value after parsing. */ diff --git a/typings/index.test-d.ts b/typings/index.test-d.ts index 734036fad..513fba723 100644 --- a/typings/index.test-d.ts +++ b/typings/index.test-d.ts @@ -418,6 +418,12 @@ expectType(baseOption.fullDescription()); expectType(baseOption.argParser((value: string) => parseInt(value))); expectType(baseOption.argParser((value: string, previous: string[]) => { return previous.concat(value); })); +// chainArgParserCalls +expectType(baseOption.chainArgParserCalls()); +expectType(baseOption.chainArgParserCalls(true)); +expectType(baseOption.chainArgParserCalls(false)); +expectType(baseOption.chainArgParserCalls(undefined)); + // makeOptionMandatory expectType(baseOption.makeOptionMandatory()); expectType(baseOption.makeOptionMandatory(true)); @@ -466,6 +472,12 @@ expectType(baseArgument.default(60, 'one minute')); expectType(baseArgument.argParser((value: string) => parseInt(value))); expectType(baseArgument.argParser((value: string, previous: string[]) => { return previous.concat(value); })); +// chainArgParserCalls +expectType(baseArgument.chainArgParserCalls()); +expectType(baseArgument.chainArgParserCalls(true)); +expectType(baseArgument.chainArgParserCalls(false)); +expectType(baseArgument.chainArgParserCalls(undefined)); + // choices expectType(baseArgument.choices(['a', 'b'])); expectType(baseArgument.choices(['a', 'b'] as const)); From d1ac0cdbdcc8dc69f5cb2c515e9c159c62efda77 Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Tue, 25 Jul 2023 15:50:40 +0200 Subject: [PATCH 03/31] Improve JSDoc readability for chainArgParserCalls --- lib/argument.js | 4 ++-- lib/option.js | 4 ++-- typings/index.d.ts | 8 ++++---- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/argument.js b/lib/argument.js index 4ed6e1f80..b3e053f88 100644 --- a/lib/argument.js +++ b/lib/argument.js @@ -92,8 +92,8 @@ class Argument { } /** - * When set to true, next call to the function provided via .argParser() will be chained to its return value if it is thenable. - * When set to undefined (the default), only chain when .parseAsync() has been called. + * When set to `true`, next call to the function provided via `.argParser()` will be chained to its return value if it is thenable. + * When set to `undefined` (the default), only chain when `.parseAsync()` has been called. * * @param {boolean | undefined} [chained] * @return {Argument} diff --git a/lib/option.js b/lib/option.js index d3c2f0624..473c32295 100644 --- a/lib/option.js +++ b/lib/option.js @@ -138,8 +138,8 @@ class Option { } /** - * When set to true, next call to the function provided via .argParser() will be chained to its return value if it is thenable. - * When set to undefined (the default), only chain when .parseAsync() has been called. + * When set to `true`, next call to the function provided via `.argParser()` will be chained to its return value if it is thenable. + * When set to `undefined` (the default), only chain when `.parseAsync()` has been called. * * @param {boolean | undefined} [chained] * @return {Argument} diff --git a/typings/index.d.ts b/typings/index.d.ts index 3e64e12e5..06110dd69 100644 --- a/typings/index.d.ts +++ b/typings/index.d.ts @@ -66,8 +66,8 @@ export class Argument { argParser(fn: (value: string, previous: T) => T): this; /** - * When set to true, next call to the function provided via .argParser() will be chained to its return value if it is thenable. - * When set to undefined (the default), only chain when .parseAsync() has been called. + * When set to `true`, next call to the function provided via `.argParser()` will be chained to its return value if it is thenable. + * When set to `undefined` (the default), only chain when `.parseAsync()` has been called. */ chainArgParserCalls(chained?: boolean | undefined): this; @@ -166,8 +166,8 @@ export class Option { argParser(fn: (value: string, previous: T) => T): this; /** - * When set to true, next call to the function provided via .argParser() will be chained to its return value if it is thenable. - * When set to undefined (the default), only chain when .parseAsync() has been called. + * When set to `true`, next call to the function provided via `.argParser()` will be chained to its return value if it is thenable. + * When set to `undefined` (the default), only chain when `.parseAsync()` has been called. */ chainArgParserCalls(chained?: boolean | undefined): this; From 2e7ccd16bcc991cd157eb6410409867e125b9068 Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Tue, 25 Jul 2023 18:05:11 +0200 Subject: [PATCH 04/31] Await thenable option and argument values when parseAsync() is called The behavior can be configured via await() Co-authored-by: John Gee --- lib/command.js | 241 ++++++++++++++++++++++++-------- tests/command.await.test.js | 267 ++++++++++++++++++++++++++++++++++++ tests/command.chain.test.js | 6 + tests/command.parse.test.js | 95 +++++++++++++ typings/index.d.ts | 7 + typings/index.test-d.ts | 6 + 6 files changed, 566 insertions(+), 56 deletions(-) create mode 100644 tests/command.await.test.js diff --git a/lib/command.js b/lib/command.js index eecff5553..5f5115ba2 100644 --- a/lib/command.js +++ b/lib/command.js @@ -57,8 +57,11 @@ class Command extends EventEmitter { this._showHelpAfterError = false; this._showSuggestionAfterError = true; + this._overwrittenOptionValues = {}; /** @type {boolean | undefined} */ this._asyncParsing = undefined; + /** @type {boolean | undefined} */ + this._await = undefined; // see .configureOutput() for docs this._outputConfiguration = { @@ -607,6 +610,14 @@ Expecting one of '${allowedValues.join("', '")}'`); // custom processing const oldValue = this.getOptionValue(name); if (val !== null && option.parseArg) { + const optionValues = this._storeOptionsAsProperties + ? this + : this._optionValues; + const overwrite = name in optionValues; + if (overwrite) { + this._overwrittenOptionValues[name] ??= []; + this._overwrittenOptionValues[name].push(this.getOptionValue(name)); + } val = this._parseArg(option, val, oldValue, handleError); } else if (val !== null && option.variadic) { val = option._concatValue(val, oldValue); @@ -1013,6 +1024,112 @@ Expecting one of '${allowedValues.join("', '")}'`); } } + /** + * When set to `true`, thenable option and argument values will be awaited right after they are parsed and processed. + * When set to `undefined` (the default), inherit the behaviour from ancestor commands, or only await when `.parseAsync()` has been called. + * Useful for asynchronous custom processing (`parseArg`) of arguments and option-arguments. + * + * @param {boolean | undefined} [enabled] + * @return {Command} `this` command for chaining + */ + await(enabled) { + this._await = enabled === undefined ? undefined : !!enabled; + return this; + } + + /** + * If `_await` is `undefined`, inherit await behaviour or base it on whether `.parseAsync()` has been called on the top-level command. + * + * @api private + */ + + _shouldAwait() { + let shouldAwait; + let cmd = this; + do { + shouldAwait = cmd._await ?? cmd._asyncParsing; + cmd = cmd.parent; + } while (shouldAwait === undefined); + return shouldAwait; + } + + /** + * @return {Promise[]} + * @api private + */ + + _getOptionResavePromises() { + const promises = []; + + Object.entries(this.opts()).forEach(([key, value]) => { + if (thenable(value)) { + promises.push((async() => { + this.setOptionValueWithSource( + key, await value, this.getOptionValueSource(key) + ); + })()); + } + }); + + Object.entries(this._overwrittenOptionValues).forEach(([key, values]) => { + values.forEach(value => { + if (thenable(value)) { + promises.push((async() => { + await value; + })()); + } + }); + }); + + return promises; + } + + /** + * @return {Promise[]} + * @api private + */ + + _getArgumentResavePromises() { + const promises = []; + + this.processedArgs.forEach((value, i) => { + if (thenable(value)) { + promises.push((async() => { + this.processedArgs[i] = await value; + })()); + } + }); + + return promises; + } + + /** + * @api private + */ + + _optionalAwait(getPromises) { + if (this._shouldAwait()) { + const toAwait = getPromises(); + if (toAwait.length) return Promise.all(toAwait); + } + } + + /** + * @api private + */ + + _awaitOptionResavePromises() { + return this._optionalAwait(() => this._getOptionResavePromises()); + } + + /** + * @api private + */ + + _awaitArgumentResavePromises() { + return this._optionalAwait(() => this._getArgumentResavePromises()); + } + /** * Execute a sub-command executable. * @@ -1332,81 +1449,93 @@ Expecting one of '${allowedValues.join("', '")}'`); */ _parseCommand(operands, unknown) { + let chain; + const parsed = this.parseOptions(unknown); this._parseOptionsEnv(); // after cli, so parseArg not called on both cli and env this._parseOptionsImplied(); + chain = this._chainOrCall(chain, () => this._awaitOptionResavePromises()); + operands = operands.concat(parsed.operands); unknown = parsed.unknown; this.args = operands.concat(unknown); if (operands && this._findCommand(operands[0])) { - return this._dispatchSubcommand(operands[0], operands.slice(1), unknown); - } - if (this._hasImplicitHelpCommand() && operands[0] === this._helpCommandName) { - return this._dispatchHelpCommand(operands[1]); - } - if (this._defaultCommandName) { + chain = this._chainOrCall(chain, () => this._dispatchSubcommand( + operands[0], operands.slice(1), unknown + )); + } else if (this._hasImplicitHelpCommand() && operands[0] === this._helpCommandName) { + chain = this._chainOrCall(chain, () => this._dispatchHelpCommand(operands[1])); + } else if (this._defaultCommandName) { outputHelpIfRequested(this, unknown); // Run the help for default command from parent rather than passing to default command - return this._dispatchSubcommand(this._defaultCommandName, operands, unknown); - } - if (this.commands.length && this.args.length === 0 && !this._actionHandler && !this._defaultCommandName) { - // probably missing subcommand and no handler, user needs help (and exit) - this.help({ error: true }); - } - - outputHelpIfRequested(this, parsed.unknown); - this._checkForMissingMandatoryOptions(); - this._checkForConflictingOptions(); - - // We do not always call this check to avoid masking a "better" error, like unknown command. - const checkForUnknownOptions = () => { - if (parsed.unknown.length > 0) { - this.unknownOption(parsed.unknown[0]); + chain = this._chainOrCall(chain, () => this._dispatchSubcommand( + this._defaultCommandName, operands, unknown + )); + } else { + if (this.commands.length && this.args.length === 0 && !this._actionHandler && !this._defaultCommandName) { + // probably missing subcommand and no handler, user needs help (and exit) + this.help({ error: true }); } - }; - const commandEvent = `command:${this.name()}`; - if (this._actionHandler) { - checkForUnknownOptions(); - this._processArguments(); + outputHelpIfRequested(this, parsed.unknown); + this._checkForMissingMandatoryOptions(); + this._checkForConflictingOptions(); - let actionResult; - actionResult = this._chainOrCallHooks(actionResult, 'preAction'); - actionResult = this._chainOrCall(actionResult, () => this._actionHandler(this.processedArgs)); - if (this.parent) { - actionResult = this._chainOrCall(actionResult, () => { + // We do not always call this check to avoid masking a "better" error, like unknown command. + const checkForUnknownOptions = () => { + if (parsed.unknown.length > 0) { + this.unknownOption(parsed.unknown[0]); + } + }; + const processAndAwaitArguments = () => { + checkForUnknownOptions(); + this._processArguments(); + chain = this._chainOrCall(chain, () => this._awaitArgumentResavePromises()); + }; + + const commandEvent = `command:${this.name()}`; + if (this._actionHandler) { + checkForUnknownOptions(); + processAndAwaitArguments(); + chain = this._chainOrCallHooks(chain, 'preAction'); + chain = this._chainOrCall(chain, () => this._actionHandler(this.processedArgs)); + if (this.parent) { + chain = this._chainOrCall(chain, () => { + this.parent.emit(commandEvent, operands, unknown); // legacy + }); + } + chain = this._chainOrCallHooks(chain, 'postAction'); + } else if (this.parent && this.parent.listenerCount(commandEvent)) { + checkForUnknownOptions(); + processAndAwaitArguments(); + chain = this._chainOrCall(chain, () => { this.parent.emit(commandEvent, operands, unknown); // legacy }); - } - actionResult = this._chainOrCallHooks(actionResult, 'postAction'); - return actionResult; - } - if (this.parent && this.parent.listenerCount(commandEvent)) { - checkForUnknownOptions(); - this._processArguments(); - this.parent.emit(commandEvent, operands, unknown); // legacy - } else if (operands.length) { - if (this._findCommand('*')) { // legacy default command - return this._dispatchSubcommand('*', operands, unknown); - } - if (this.listenerCount('command:*')) { - // skip option check, emit event for possible misspelling suggestion - this.emit('command:*', operands, unknown); + } else if (operands.length) { + if (this._findCommand('*')) { // legacy default command + return this._dispatchSubcommand('*', operands, unknown); + } + if (this.listenerCount('command:*')) { + // skip option check, emit event for possible misspelling suggestion + this.emit('command:*', operands, unknown); + } else if (this.commands.length) { + this.unknownCommand(); + } else { + checkForUnknownOptions(); + processAndAwaitArguments(); + } } else if (this.commands.length) { - this.unknownCommand(); + checkForUnknownOptions(); + // This command has subcommands and nothing hooked up at this level, so display help (and exit). + this.help({ error: true }); } else { checkForUnknownOptions(); - this._processArguments(); + processAndAwaitArguments(); + // fall through for caller to handle after calling .parse() } - } else if (this.commands.length) { - checkForUnknownOptions(); - // This command has subcommands and nothing hooked up at this level, so display help (and exit). - this.help({ error: true }); - } else { - checkForUnknownOptions(); - this._processArguments(); - // fall through for caller to handle after calling .parse() } + + return chain; } /** diff --git a/tests/command.await.test.js b/tests/command.await.test.js new file mode 100644 index 000000000..84cf543eb --- /dev/null +++ b/tests/command.await.test.js @@ -0,0 +1,267 @@ +/* eslint 'jest/expect-expect': [ + 'warn', + { + assertFunctionNames: ['expect', 'testWithArguments', 'testWithOptions'] + } +] */ + +const commander = require('../'); + +const makeThenable = (function() { + const cache = new Map(); + return (value) => { + if (cache.has(value)) { + return cache.get(value); + } + const thenable = { + then: (fn) => makeThenable(fn(value)) + }; + cache.set(value, thenable); + return thenable; + }; +})(); + +const chainedAwaited = async(coercion, args) => ( + args.reduce(async(promise, v) => { + const { thenable } = await promise; + return { thenable: makeThenable(coercion(v, await thenable)) }; + }, { thenable: makeThenable(undefined) }) +); + +describe('await arguments', () => { + async function testWithArguments(program, args, resolvedValues, awaited) { + let actionValues; + program + .action((...args) => { + actionValues = args.slice(0, resolvedValues.length); + }); + + const result = program.parseAsync(args, { from: 'user' }); + awaited.forEach((value, i) => { + expect(program.processedArgs[i]).toBe(value); + }); + await result; + expect(program.processedArgs).toEqual(resolvedValues); + expect(actionValues).toEqual(resolvedValues); + } + + test('when arguments with custom processing then .processedArgs and action arguments resolved from callback', async() => { + const args = ['1', '2']; + const resolvedValues = [3, 4]; + const awaited = [ + makeThenable(resolvedValues[0]), + resolvedValues[1] + ]; + const mockCoercions = awaited.map( + value => jest.fn().mockImplementation(() => value) + ); + + const program = new commander.Command(); + program + .argument('', 'desc', mockCoercions[0]) + .argument('[arg]', 'desc', mockCoercions[1]); + + await testWithArguments(program, args, resolvedValues, awaited); + }); + + test('when arguments not specified with default values then .processedArgs and action arguments resolved from default values', async() => { + const args = []; + const resolvedValues = [1, 2]; + const awaited = [ + makeThenable(resolvedValues[0]), + resolvedValues[1] + ]; + + const program = new commander.Command(); + program + .argument('[arg]', 'desc', awaited[0]) + .argument('[arg]', 'desc', awaited[1]); + + await testWithArguments(program, args, resolvedValues, awaited); + }); + + test('when variadic argument with chained asynchronous custom processing then .processedArgs and action arguments resolved from chain', async() => { + const args = ['1', '2']; + const resolvedValues = ['12']; + const coercion = (value, previousValue) => { + const coerced = previousValue === undefined + ? value + : previousValue + value; + return makeThenable(coerced); + }; + const awaited = [(await chainedAwaited(coercion, args)).thenable]; + const mockCoercion = jest.fn().mockImplementation(coercion); + + const argument = new commander.Argument('', 'desc') + .argParser(mockCoercion) + .chainArgParserCalls(); + + const program = new commander.Command(); + program + .addArgument(argument); + + await testWithArguments(program, args, resolvedValues, awaited); + }); +}); + +describe('await options', () => { + async function testWithOptions(program, args, resolvedValues, awaited) { + program + .action(() => {}); + + const result = program.parseAsync(args, { from: 'user' }); + Object.entries(awaited).forEach(([key, value]) => { + expect(program.opts()[key]).toBe(value); + }); + await result; + expect(program.opts()).toEqual(resolvedValues); + } + + test('when options with custom processing then .opts() resolved from callback', async() => { + const args = ['-a', '1', '-b', '2']; + const resolvedValues = { a: 3, b: 4 }; + const awaited = { + a: makeThenable(resolvedValues.a), + b: resolvedValues.b + }; + const mockCoercions = Object.entries(awaited).reduce((acc, [key, value]) => { + acc[key] = jest.fn().mockImplementation(() => value); + return acc; + }, {}); + + const program = new commander.Command(); + program + .option('-a ', 'desc', mockCoercions.a) + .option('-b [arg]', 'desc', mockCoercions.b); + + await testWithOptions(program, args, resolvedValues, awaited); + expect(program.getOptionValueSource('a')).toEqual('cli'); + expect(program.getOptionValueSource('b')).toEqual('cli'); + }); + + test('when options not specified with default values then .opts() resolved from default values', async() => { + const args = []; + const resolvedValues = { a: 1, b: 2 }; + const awaited = { + a: makeThenable(resolvedValues.a), + b: resolvedValues.b + }; + + const program = new commander.Command(); + program + .option('-a ', 'desc', awaited.a) + .option('-b [arg]', 'desc', awaited.b); + + await testWithOptions(program, args, resolvedValues, awaited); + expect(program.getOptionValueSource('a')).toEqual('default'); + expect(program.getOptionValueSource('b')).toEqual('default'); + }); + + test('when implied option values then .opts() resolved from implied values', async() => { + const args = ['-c']; + const resolvedValues = { a: 1, b: 2, c: true }; + const awaited = { + a: makeThenable(resolvedValues.a), + b: resolvedValues.b, + c: true + }; + + const option = new commander.Option('-c').implies(awaited); + const program = new commander.Command(); + program + .option('-a ') + .option('-b [arg]') + .addOption(option); + + await testWithOptions(program, args, resolvedValues, awaited); + expect(program.getOptionValueSource('a')).toEqual('implied'); + expect(program.getOptionValueSource('b')).toEqual('implied'); + }); + + test('when non-variadic repeated option with chained asynchronous custom processing then .opts() resolved from chain', async() => { + const args = ['-a', '1', '-a', '2']; + const resolvedValues = { a: '12' }; + const coercion = (value, previousValue) => { + const coerced = previousValue === undefined + ? value + : previousValue + value; + return makeThenable(coerced); + }; + const awaited = { + a: (await chainedAwaited( + coercion, args.filter((_, i) => i % 2)) + ).thenable + }; + const mockCoercion = jest.fn().mockImplementation(coercion); + + const option = new commander.Option('-a [arg]', 'desc') + .argParser(mockCoercion) + .chainArgParserCalls(); + + const program = new commander.Command(); + program + .addOption(option); + + await testWithOptions(program, args, resolvedValues, awaited); + expect(program.getOptionValueSource('a')).toEqual('cli'); + }); + + test('when variadic option with chained asynchronous custom processing then .opts() resolved from chain', async() => { + const args = ['-a', '1', '2']; + const resolvedValues = { a: '12' }; + const coercion = (value, previousValue) => { + const coerced = previousValue === undefined + ? value + : previousValue + value; + return makeThenable(coerced); + }; + const awaited = { + a: (await chainedAwaited( + coercion, args.slice(1)) + ).thenable + }; + const mockCoercion = jest.fn().mockImplementation(coercion); + + const option = new commander.Option('-a ', 'desc') + .argParser(mockCoercion) + .chainArgParserCalls(); + + const program = new commander.Command(); + program + .addOption(option); + + await testWithOptions(program, args, resolvedValues, awaited); + expect(program.getOptionValueSource('a')).toEqual('cli'); + }); + + test('when subcommand and options with custom processing then global .opts() resolved from callback before local option-argument parser is called', async() => { + const arr = []; + + const coercion = (value) => { + return new Promise((resolve) => { + setImmediate(() => { + arr.push(value); + resolve(value); + }); + }); + }; + const mockCoercion = jest.fn().mockImplementation(coercion); + + const syncCoercion = (value) => { + arr.push(value); + return value; + }; + const mockSyncCoercion = jest.fn().mockImplementation(syncCoercion); + + const program = new commander.Command(); + program + .option('-a ', 'desc', mockCoercion) + .command('subcommand') + .option('-b ', 'desc', mockSyncCoercion) + .action(() => {}); + + const args = ['-a', '1', 'subcommand', '-b', '2']; + await program.parseAsync(args, { from: 'user' }); + expect(arr).toEqual(['1', '2']); + }); +}); diff --git a/tests/command.chain.test.js b/tests/command.chain.test.js index 3f907c869..7e60cf80d 100644 --- a/tests/command.chain.test.js +++ b/tests/command.chain.test.js @@ -166,6 +166,12 @@ describe('Command methods that should return this for chaining', () => { expect(result).toBe(program); }); + test('when call .await() then returns this', () => { + const program = new Command(); + const result = program.await(); + expect(result).toBe(program); + }); + test('when call .hook() then returns this', () => { const program = new Command(); const result = program.hook('preAction', () => {}); diff --git a/tests/command.parse.test.js b/tests/command.parse.test.js index b9903725f..2ab5c6696 100644 --- a/tests/command.parse.test.js +++ b/tests/command.parse.test.js @@ -87,6 +87,101 @@ describe('return type', () => { const result = await program.parseAsync(['node', 'test']); expect(result).toBe(program); }); + + const makeMockCoercion = (errors, promises, condition) => ( + jest.fn().mockImplementation( + (value) => { + if (condition?.()) { + return value; + } + + const error = new Error(); + errors.push(error); + const promise = Promise.reject(error); + promises.push(promise); + return promise; + } + ) + ); + + test('when await .parseAsync and asynchronous custom processing for arguments fails then rejects', async() => { + const promises = []; + const errors = []; + const mockCoercion = makeMockCoercion(errors, promises); + + const program = new commander.Command(); + program + .argument('[arg]', 'desc', mockCoercion) + .argument('[arg]', 'desc', mockCoercion) + .action(() => { }); + + const result = program.parseAsync(['1', '2'], { from: 'user' }); + + let caught; + try { + await result; + } catch (value) { + caught = value; + } + + expect(errors).toContain(caught); + expect(mockCoercion).toHaveBeenCalledTimes(2); + }); + + test('when await .parseAsync and asynchronous custom processing for options fails then rejects', async() => { + const promises = []; + const errors = []; + const mockCoercion = makeMockCoercion(errors, promises); + + const program = new commander.Command(); + program + .option('-a [arg]', 'desc', mockCoercion) + .option('-b [arg]', 'desc', mockCoercion) + .action(() => { }); + + const result = program.parseAsync(['-a', '1', '-b', '2'], { from: 'user' }); + + let caught; + try { + await result; + } catch (value) { + caught = value; + } + + expect(errors).toContain(caught); + expect(mockCoercion).toHaveBeenCalledTimes(2); + }); + + test('when await .parseAsync and not chained asynchronous custom processing fails for overwritten non-variadic option then rejects', async() => { + const promises = []; + const errors = []; + const mockCoercion = makeMockCoercion( + errors, promises, () => promises.length + ); + + const option = new commander.Option('-a [arg]', 'desc') + .argParser(mockCoercion) + .chainArgParserCalls(false); + + const program = new commander.Command(); + program + .addOption(option) + .action(() => { }); + + const result = program.parseAsync( + ['-a', '1', '-a', '2', '-a', '3'], { from: 'user' } + ); + + let caught; + try { + await result; + } catch (value) { + caught = value; + } + + expect(errors[0]).toBe(caught); + expect(mockCoercion).toHaveBeenCalledTimes(3); + }); }); // Easy mistake to make when writing unit tests diff --git a/typings/index.d.ts b/typings/index.d.ts index 06110dd69..e6a619250 100644 --- a/typings/index.d.ts +++ b/typings/index.d.ts @@ -704,6 +704,13 @@ export class Command { */ parseAsync(argv?: readonly string[], options?: ParseOptions): Promise; + /** + * When set to `true`, thenable option and argument values will be awaited right after they are parsed and processed. + * When set to `undefined` (the default), inherit the behaviour from ancestor commands, or only await when `.parseAsync()` has been called. + * Useful for asynchronous custom processing (`parseArg`) of arguments and option-arguments. + */ + await(enabled?: boolean | undefined): this; + /** * Parse options from `argv` removing known options, * and return argv split into operands and unknown arguments. diff --git a/typings/index.test-d.ts b/typings/index.test-d.ts index 513fba723..cf4c8a490 100644 --- a/typings/index.test-d.ts +++ b/typings/index.test-d.ts @@ -214,6 +214,12 @@ expectType>(program.parseAsync(['node', 'script.js'], expectType>(program.parseAsync(['--option'], { from: 'user' })); expectType>(program.parseAsync(['node', 'script.js'] as const)); +// await +expectType(program.await()); +expectType(program.await(true)); +expectType(program.await(false)); +expectType(program.await(undefined)); + // parseOptions (and ParseOptionsResult) expectType<{ operands: string[]; unknown: string[] }>(program.parseOptions(['node', 'script.js', 'hello'])); From 2d0c78f10525d9b4859e5e51c7a974ab9bd02ae8 Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Tue, 25 Jul 2023 19:12:07 +0200 Subject: [PATCH 05/31] Leave argument variations test untouched Possible since undefined properties are not checked --- tests/command.argumentVariations.test.js | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/tests/command.argumentVariations.test.js b/tests/command.argumentVariations.test.js index ad7fa3003..a8a781625 100644 --- a/tests/command.argumentVariations.test.js +++ b/tests/command.argumentVariations.test.js @@ -9,8 +9,7 @@ test.each(getSingleArgCases(''))('when add "" using %s t _name: 'explicit-required', required: true, variadic: false, - description: '', - chained: undefined + description: '' }; expect(argument).toEqual(expectedShape); }); @@ -21,8 +20,7 @@ test.each(getSingleArgCases('implicit-required'))('when add "arg" using %s then _name: 'implicit-required', required: true, variadic: false, - description: '', - chained: undefined + description: '' }; expect(argument).toEqual(expectedShape); }); @@ -33,8 +31,7 @@ test.each(getSingleArgCases('[optional]'))('when add "[arg]" using %s then argum _name: 'optional', required: false, variadic: false, - description: '', - chained: undefined + description: '' }; expect(argument).toEqual(expectedShape); }); @@ -45,8 +42,7 @@ test.each(getSingleArgCases(''))('when add "" usin _name: 'explicit-required', required: true, variadic: true, - description: '', - chained: undefined + description: '' }; expect(argument).toEqual(expectedShape); }); @@ -57,8 +53,7 @@ test.each(getSingleArgCases('implicit-required...'))('when add "arg..." using %s _name: 'implicit-required', required: true, variadic: true, - description: '', - chained: undefined + description: '' }; expect(argument).toEqual(expectedShape); }); @@ -69,8 +64,7 @@ test.each(getSingleArgCases('[optional...]'))('when add "[arg...]" using %s then _name: 'optional', required: false, variadic: true, - description: '', - chained: undefined + description: '' }; expect(argument).toEqual(expectedShape); }); From 905dac07a17d017029afc7d1a7de1540ed93a228 Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Wed, 26 Jul 2023 02:49:25 +0300 Subject: [PATCH 06/31] Remove redundant checkForUnknownOptions() call Co-authored-by: John Gee --- lib/command.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/command.js b/lib/command.js index 5f5115ba2..b75db2b9a 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1488,7 +1488,6 @@ Expecting one of '${allowedValues.join("', '")}'`); } }; const processAndAwaitArguments = () => { - checkForUnknownOptions(); this._processArguments(); chain = this._chainOrCall(chain, () => this._awaitArgumentResavePromises()); }; From dacdde0ac09dc5e7d213e3308863312c76885a24 Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Wed, 26 Jul 2023 16:42:39 +0300 Subject: [PATCH 07/31] Rework _parseArg() --- lib/command.js | 53 +++++++++++++++++++++----------------------------- 1 file changed, 22 insertions(+), 31 deletions(-) diff --git a/lib/command.js b/lib/command.js index b75db2b9a..3b85c2307 100644 --- a/lib/command.js +++ b/lib/command.js @@ -294,16 +294,18 @@ class Command extends EventEmitter { /** * @param {Argument|Option} target * @param {*} value + * @param {*} previous * @param {Function} handleError + * @return {*} * @api private */ - _catchChainError(target, value, handleError) { - if (this._shouldChainArgParserCalls(target) && thenable(value)) { - return value.then(x => x, handleError); + _parseArgAgnostic(target, value, previous, handleError) { + let result = target.parseArg(value, previous); + if (this._shouldChainArgParserCalls(target) && thenable(result)) { + result = result.then(x => x, handleError); } - - return value; + return result; } /** @@ -318,28 +320,17 @@ class Command extends EventEmitter { */ _parseArg(target, value, previous, handleError) { - let parsedValue; - - if (this._shouldChainArgParserCalls(target) && thenable(previous)) { - // chain thenables - parsedValue = previous.then( - (result) => { - result = target.parseArg(value, result); - // call with result and not parsedValue to not catch handleError exception repeatedly - result = this._catchChainError(target, result, handleError); - return result; - } - ); - } else { - try { - parsedValue = target.parseArg(value, previous); - parsedValue = this._catchChainError(target, parsedValue, handleError); - } catch (err) { - handleError(err); + try { + if (this._shouldChainArgParserCalls(target)) { + return this._chainOrCall(previous, (resolvedPrevious) => ( + this._parseArgAgnostic(target, value, resolvedPrevious, handleError) + )); } - } - return parsedValue; + return this._parseArgAgnostic(target, value, previous, handleError); + } catch (err) { + handleError(err); + } } /** @@ -1374,19 +1365,19 @@ Expecting one of '${allowedValues.join("', '")}'`); /** * Once we have a promise we chain, but call synchronously until then. * - * @param {Promise|undefined} promise + * @param {*} chain * @param {Function} fn - * @return {Promise|undefined} + * @return {*} * @api private */ - _chainOrCall(promise, fn) { - if (thenable(promise)) { + _chainOrCall(chain, fn) { + if (thenable(chain)) { // already have a promise, chain callback - return promise.then(() => fn()); + return chain.then(fn); } // callback might return a promise - return fn(); + return fn(chain); } /** From b64f29833af20602bfe2a0d8ea5cf37596ebc55a Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Wed, 26 Jul 2023 19:27:38 +0300 Subject: [PATCH 08/31] Simplify overwritten option value handling --- lib/command.js | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/lib/command.js b/lib/command.js index 3b85c2307..79f1e16cc 100644 --- a/lib/command.js +++ b/lib/command.js @@ -57,7 +57,8 @@ class Command extends EventEmitter { this._showHelpAfterError = false; this._showSuggestionAfterError = true; - this._overwrittenOptionValues = {}; + /** @type {Array} */ + this._overwrittenThenableOptionValues = []; /** @type {boolean | undefined} */ this._asyncParsing = undefined; /** @type {boolean | undefined} */ @@ -601,13 +602,14 @@ Expecting one of '${allowedValues.join("', '")}'`); // custom processing const oldValue = this.getOptionValue(name); if (val !== null && option.parseArg) { - const optionValues = this._storeOptionsAsProperties - ? this - : this._optionValues; - const overwrite = name in optionValues; - if (overwrite) { - this._overwrittenOptionValues[name] ??= []; - this._overwrittenOptionValues[name].push(this.getOptionValue(name)); + if (thenable(oldValue)) { + const optionValues = this._storeOptionsAsProperties + ? this + : this._optionValues; + const overwrite = name in optionValues; + if (overwrite) { + this._overwrittenThenableOptionValues.push(oldValue); + } } val = this._parseArg(option, val, oldValue, handleError); } else if (val !== null && option.variadic) { @@ -1050,7 +1052,9 @@ Expecting one of '${allowedValues.join("', '")}'`); */ _getOptionResavePromises() { - const promises = []; + const promises = this._overwrittenThenableOptionValues.map((value) => ( + async() => { await value; } + )()); Object.entries(this.opts()).forEach(([key, value]) => { if (thenable(value)) { @@ -1062,16 +1066,6 @@ Expecting one of '${allowedValues.join("', '")}'`); } }); - Object.entries(this._overwrittenOptionValues).forEach(([key, values]) => { - values.forEach(value => { - if (thenable(value)) { - promises.push((async() => { - await value; - })()); - } - }); - }); - return promises; } From 254865cd333ce2326c1dfcb5583162961da93ffd Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Wed, 26 Jul 2023 19:40:26 +0300 Subject: [PATCH 09/31] Handle thenable errors not only when chaining but only when awaiting --- lib/command.js | 41 ++++++++++++------------ tests/argument.custom-processing.test.js | 2 +- 2 files changed, 22 insertions(+), 21 deletions(-) diff --git a/lib/command.js b/lib/command.js index 79f1e16cc..dc10b2143 100644 --- a/lib/command.js +++ b/lib/command.js @@ -292,23 +292,6 @@ class Command extends EventEmitter { return target.chained || (target.chained === undefined && this._asyncParsing); } - /** - * @param {Argument|Option} target - * @param {*} value - * @param {*} previous - * @param {Function} handleError - * @return {*} - * @api private - */ - - _parseArgAgnostic(target, value, previous, handleError) { - let result = target.parseArg(value, previous); - if (this._shouldChainArgParserCalls(target) && thenable(result)) { - result = result.then(x => x, handleError); - } - return result; - } - /** * Internal implementation shared by ._processArguments() and option and optionEnv event listeners. * @@ -321,14 +304,16 @@ class Command extends EventEmitter { */ _parseArg(target, value, previous, handleError) { + const handleThenableError = this._shouldAwait() ? handleError : undefined; + try { if (this._shouldChainArgParserCalls(target)) { return this._chainOrCall(previous, (resolvedPrevious) => ( - this._parseArgAgnostic(target, value, resolvedPrevious, handleError) + parseArg(target, value, resolvedPrevious, handleThenableError) )); } - return this._parseArgAgnostic(target, value, previous, handleError); + return parseArg(target, value, previous, handleThenableError); } catch (err) { handleError(err); } @@ -1320,7 +1305,6 @@ Expecting one of '${allowedValues.join("', '")}'`); throw err; }; - // Extra processing for nice error message on parsing failure. let parsedValue = value; if (value !== null && argument.parseArg) { parsedValue = this._parseArg(argument, value, previous, handleError); @@ -2391,4 +2375,21 @@ function thenable(value) { return typeof value?.then === 'function'; } +/** + * @param {Argument|Option} target + * @param {*} value + * @param {*} previous + * @param {Function} [handleThenableError] + * @returns {*} + * @api private + */ + +function parseArg(target, value, previous, handleThenableError) { + let result = target.parseArg(value, previous); + if (handleThenableError && thenable(result)) { + result = result.then(x => x, handleThenableError); + } + return result; +} + exports.Command = Command; diff --git a/tests/argument.custom-processing.test.js b/tests/argument.custom-processing.test.js index 453c786e4..72ce05d1c 100644 --- a/tests/argument.custom-processing.test.js +++ b/tests/argument.custom-processing.test.js @@ -206,7 +206,7 @@ test('when custom processing for argument throws plain error then not CommanderE expect(caughtErr).not.toBeInstanceOf(commander.CommanderError); }); -test('when custom with .chainArgParserCalls(true) then parsed to chain', async() => { +test('when custom variadic with .chainArgParserCalls(true) then parsed to chain', async() => { const args = ['1', '2']; const resolvedValue = '12'; const coercion = async(value, previousValue) => ( From e66eda541045f5307fbff7db30536fb5254099c6 Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Wed, 26 Jul 2023 20:01:33 +0300 Subject: [PATCH 10/31] Precompute _shouldAwait to avoid repeating costly computation --- lib/command.js | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/lib/command.js b/lib/command.js index dc10b2143..b70e2d8b0 100644 --- a/lib/command.js +++ b/lib/command.js @@ -63,6 +63,8 @@ class Command extends EventEmitter { this._asyncParsing = undefined; /** @type {boolean | undefined} */ this._await = undefined; + /** @type {boolean | undefined} */ + this._shouldAwait = undefined; // see .configureOutput() for docs this._outputConfiguration = { @@ -304,7 +306,7 @@ class Command extends EventEmitter { */ _parseArg(target, value, previous, handleError) { - const handleThenableError = this._shouldAwait() ? handleError : undefined; + const handleThenableError = this._shouldAwait ? handleError : undefined; try { if (this._shouldChainArgParserCalls(target)) { @@ -959,6 +961,7 @@ Expecting one of '${allowedValues.join("', '")}'`); parse(argv, parseOptions) { this._asyncParsing = false; + this._setShouldAwait(); try { const userArgs = this._prepareUserArgs(argv, parseOptions); @@ -967,6 +970,7 @@ Expecting one of '${allowedValues.join("', '")}'`); return this; } finally { this._asyncParsing = undefined; + this._shouldAwait = undefined; } } @@ -991,6 +995,7 @@ Expecting one of '${allowedValues.join("', '")}'`); async parseAsync(argv, parseOptions) { this._asyncParsing = true; + this._setShouldAwait(); try { const userArgs = this._prepareUserArgs(argv, parseOptions); @@ -999,6 +1004,7 @@ Expecting one of '${allowedValues.join("', '")}'`); return this; } finally { this._asyncParsing = undefined; + this._shouldAwait = undefined; } } @@ -1021,14 +1027,10 @@ Expecting one of '${allowedValues.join("', '")}'`); * @api private */ - _shouldAwait() { - let shouldAwait; - let cmd = this; - do { - shouldAwait = cmd._await ?? cmd._asyncParsing; - cmd = cmd.parent; - } while (shouldAwait === undefined); - return shouldAwait; + _setShouldAwait() { + for (let cmd = this; this._shouldAwait === undefined; cmd = cmd.parent) { + this._shouldAwait = cmd._await ?? cmd._asyncParsing; + } } /** @@ -1078,7 +1080,7 @@ Expecting one of '${allowedValues.join("', '")}'`); */ _optionalAwait(getPromises) { - if (this._shouldAwait()) { + if (this._shouldAwait) { const toAwait = getPromises(); if (toAwait.length) return Promise.all(toAwait); } From fb48134a7937e1596ebeb39da2ea05ebf7f588ad Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Wed, 26 Jul 2023 20:06:56 +0300 Subject: [PATCH 11/31] Position _parseArg() better --- lib/command.js | 74 +++++++++++++++++++++++++------------------------- 1 file changed, 37 insertions(+), 37 deletions(-) diff --git a/lib/command.js b/lib/command.js index b70e2d8b0..d0baa74c2 100644 --- a/lib/command.js +++ b/lib/command.js @@ -284,43 +284,6 @@ class Command extends EventEmitter { return this; } - /** - * @param {Argument|Option} target - * @return {boolean} - * @api private - */ - - _shouldChainArgParserCalls(target) { - return target.chained || (target.chained === undefined && this._asyncParsing); - } - - /** - * Internal implementation shared by ._processArguments() and option and optionEnv event listeners. - * - * @param {Argument|Option} target - * @param {*} value - * @param {*} previous - * @param {Function} handleError - * @return {*} - * @api private - */ - - _parseArg(target, value, previous, handleError) { - const handleThenableError = this._shouldAwait ? handleError : undefined; - - try { - if (this._shouldChainArgParserCalls(target)) { - return this._chainOrCall(previous, (resolvedPrevious) => ( - parseArg(target, value, resolvedPrevious, handleThenableError) - )); - } - - return parseArg(target, value, previous, handleThenableError); - } catch (err) { - handleError(err); - } - } - /** * Factory routine to create a new unattached argument. * @@ -1412,6 +1375,43 @@ Expecting one of '${allowedValues.join("', '")}'`); return result; } + /** + * @param {Argument|Option} target + * @return {boolean} + * @api private + */ + + _shouldChainArgParserCalls(target) { + return target.chained || (target.chained === undefined && this._asyncParsing); + } + + /** + * Internal implementation shared by ._processArguments() and option and optionEnv event listeners. + * + * @param {Argument|Option} target + * @param {*} value + * @param {*} previous + * @param {Function} handleError + * @return {*} + * @api private + */ + + _parseArg(target, value, previous, handleError) { + const handleThenableError = this._shouldAwait ? handleError : undefined; + + try { + if (this._shouldChainArgParserCalls(target)) { + return this._chainOrCall(previous, (resolvedPrevious) => ( + parseArg(target, value, resolvedPrevious, handleThenableError) + )); + } + + return parseArg(target, value, previous, handleThenableError); + } catch (err) { + handleError(err); + } + } + /** * Process arguments in context of this command. * Returns action result, in case it is a promise. From c34a8393f7beeb746c782494bb063e1e34207b7d Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Wed, 26 Jul 2023 20:30:20 +0300 Subject: [PATCH 12/31] Inherit _asyncParsing. Do not unset it and _shouldAwait This is in accordance with how other properties containing information about the last parse call such as args & processedArgs do not get unset. Consistency in how repeated parse calls are handled could be improved by resolving #1916. --- lib/command.js | 33 ++++++++++++++------------------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/lib/command.js b/lib/command.js index d0baa74c2..e803b7710 100644 --- a/lib/command.js +++ b/lib/command.js @@ -926,15 +926,10 @@ Expecting one of '${allowedValues.join("', '")}'`); this._asyncParsing = false; this._setShouldAwait(); - try { - const userArgs = this._prepareUserArgs(argv, parseOptions); - this._parseCommand([], userArgs); + const userArgs = this._prepareUserArgs(argv, parseOptions); + this._parseCommand([], userArgs); - return this; - } finally { - this._asyncParsing = undefined; - this._shouldAwait = undefined; - } + return this; } /** @@ -960,15 +955,10 @@ Expecting one of '${allowedValues.join("', '")}'`); this._asyncParsing = true; this._setShouldAwait(); - try { - const userArgs = this._prepareUserArgs(argv, parseOptions); - await this._parseCommand([], userArgs); + const userArgs = this._prepareUserArgs(argv, parseOptions); + await this._parseCommand([], userArgs); - return this; - } finally { - this._asyncParsing = undefined; - this._shouldAwait = undefined; - } + return this; } /** @@ -991,9 +981,13 @@ Expecting one of '${allowedValues.join("', '")}'`); */ _setShouldAwait() { - for (let cmd = this; this._shouldAwait === undefined; cmd = cmd.parent) { - this._shouldAwait = cmd._await ?? cmd._asyncParsing; - } + this._shouldAwait = undefined; + let cmd = this; + do { + this._shouldAwait = cmd._await; + cmd = cmd.parent; + } while (this._shouldAwait === undefined && cmd); + this._shouldAwait ??= this._asyncParsing; } /** @@ -1199,6 +1193,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'); From 4d65c6ca226cb1a553ba95ea9b5f74c15f791b1d Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Wed, 26 Jul 2023 22:33:38 +0300 Subject: [PATCH 13/31] Simplify _shouldAwait computation --- lib/command.js | 23 +++++------------------ 1 file changed, 5 insertions(+), 18 deletions(-) diff --git a/lib/command.js b/lib/command.js index e803b7710..7f7799d69 100644 --- a/lib/command.js +++ b/lib/command.js @@ -924,7 +924,6 @@ Expecting one of '${allowedValues.join("', '")}'`); parse(argv, parseOptions) { this._asyncParsing = false; - this._setShouldAwait(); const userArgs = this._prepareUserArgs(argv, parseOptions); this._parseCommand([], userArgs); @@ -953,7 +952,6 @@ Expecting one of '${allowedValues.join("', '")}'`); async parseAsync(argv, parseOptions) { this._asyncParsing = true; - this._setShouldAwait(); const userArgs = this._prepareUserArgs(argv, parseOptions); await this._parseCommand([], userArgs); @@ -974,22 +972,6 @@ Expecting one of '${allowedValues.join("', '")}'`); return this; } - /** - * If `_await` is `undefined`, inherit await behaviour or base it on whether `.parseAsync()` has been called on the top-level command. - * - * @api private - */ - - _setShouldAwait() { - this._shouldAwait = undefined; - let cmd = this; - do { - this._shouldAwait = cmd._await; - cmd = cmd.parent; - } while (this._shouldAwait === undefined && cmd); - this._shouldAwait ??= this._asyncParsing; - } - /** * @return {Promise[]} * @api private @@ -1415,6 +1397,11 @@ Expecting one of '${allowedValues.join("', '")}'`); */ _parseCommand(operands, unknown) { + // Use explicitly set await mode, + // or inherit await behaviour from parent, + // or base it on whether .parseAsync() has been called. + this._shouldAwait = this._await ?? this.parent?._shouldAwait ?? this._asyncParsing; + let chain; const parsed = this.parseOptions(unknown); From 679bcb571837bc37c435fbbf10442a3bbfa959b1 Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Thu, 27 Jul 2023 15:20:04 +0300 Subject: [PATCH 14/31] Inherit await behavior the commander way --- lib/command.js | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/lib/command.js b/lib/command.js index 7f7799d69..58d2429e8 100644 --- a/lib/command.js +++ b/lib/command.js @@ -63,8 +63,6 @@ class Command extends EventEmitter { this._asyncParsing = undefined; /** @type {boolean | undefined} */ this._await = undefined; - /** @type {boolean | undefined} */ - this._shouldAwait = undefined; // see .configureOutput() for docs this._outputConfiguration = { @@ -114,6 +112,7 @@ class Command extends EventEmitter { this._enablePositionalOptions = sourceCommand._enablePositionalOptions; this._showHelpAfterError = sourceCommand._showHelpAfterError; this._showSuggestionAfterError = sourceCommand._showSuggestionAfterError; + this._await = sourceCommand._await; return this; } @@ -972,6 +971,14 @@ Expecting one of '${allowedValues.join("', '")}'`); return this; } + /** + * @api private + */ + + _shouldAwait() { + return this._await || (this._await === undefined && this._asyncParsing); + } + /** * @return {Promise[]} * @api private @@ -1019,7 +1026,7 @@ Expecting one of '${allowedValues.join("', '")}'`); */ _optionalAwait(getPromises) { - if (this._shouldAwait) { + if (this._shouldAwait()) { const toAwait = getPromises(); if (toAwait.length) return Promise.all(toAwait); } @@ -1374,7 +1381,7 @@ Expecting one of '${allowedValues.join("', '")}'`); */ _parseArg(target, value, previous, handleError) { - const handleThenableError = this._shouldAwait ? handleError : undefined; + const handleThenableError = this._shouldAwait() ? handleError : undefined; try { if (this._shouldChainArgParserCalls(target)) { @@ -1397,11 +1404,6 @@ Expecting one of '${allowedValues.join("', '")}'`); */ _parseCommand(operands, unknown) { - // Use explicitly set await mode, - // or inherit await behaviour from parent, - // or base it on whether .parseAsync() has been called. - this._shouldAwait = this._await ?? this.parent?._shouldAwait ?? this._asyncParsing; - let chain; const parsed = this.parseOptions(unknown); From ade5f95304e9b4887a37f0d17bc7951a881d8868 Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Thu, 27 Jul 2023 19:10:04 +0300 Subject: [PATCH 15/31] Handle overwritten not chained options correctly --- lib/command.js | 124 +++++++++++++++++++----------------- tests/command.parse.test.js | 4 +- 2 files changed, 66 insertions(+), 62 deletions(-) diff --git a/lib/command.js b/lib/command.js index 58d2429e8..c7c520b95 100644 --- a/lib/command.js +++ b/lib/command.js @@ -57,12 +57,11 @@ class Command extends EventEmitter { this._showHelpAfterError = false; this._showSuggestionAfterError = true; - /** @type {Array} */ - this._overwrittenThenableOptionValues = []; /** @type {boolean | undefined} */ this._asyncParsing = undefined; /** @type {boolean | undefined} */ this._await = undefined; + this._handledNotChainedNonVariadicOptionValues = {}; // see .configureOutput() for docs this._outputConfiguration = { @@ -551,15 +550,6 @@ Expecting one of '${allowedValues.join("', '")}'`); // custom processing const oldValue = this.getOptionValue(name); if (val !== null && option.parseArg) { - if (thenable(oldValue)) { - const optionValues = this._storeOptionsAsProperties - ? this - : this._optionValues; - const overwrite = name in optionValues; - if (overwrite) { - this._overwrittenThenableOptionValues.push(oldValue); - } - } val = this._parseArg(option, val, oldValue, handleError); } else if (val !== null && option.variadic) { val = option._concatValue(val, oldValue); @@ -985,21 +975,28 @@ Expecting one of '${allowedValues.join("', '")}'`); */ _getOptionResavePromises() { - const promises = this._overwrittenThenableOptionValues.map((value) => ( - async() => { await value; } - )()); - - Object.entries(this.opts()).forEach(([key, value]) => { - if (thenable(value)) { - promises.push((async() => { - this.setOptionValueWithSource( - key, await value, this.getOptionValueSource(key) - ); - })()); - } - }); - - return promises; + const chainedPromises = Object.entries(this.opts()) + .filter(([_, value]) => isThenable(value)) + .map(([key, thenable]) => (async() => { + this.setOptionValueWithSource( + key, await thenable, this.getOptionValueSource(key) + ); + })()); + + const notChainedNonVariadicPromises = Object.entries( + this._handledNotChainedNonVariadicOptionValues + ) + .map(([_, values]) => ( + // Only await overwritten thenables so that behaviour is consistent with how not chained non-variadic arguments are handled. + values.slice(0, -1) + )) + .flat() + .filter(isThenable) + .map((thenable) => (async() => { + await thenable; + })()); + + return chainedPromises.concat(notChainedNonVariadicPromises); } /** @@ -1008,17 +1005,11 @@ Expecting one of '${allowedValues.join("', '")}'`); */ _getArgumentResavePromises() { - const promises = []; - - this.processedArgs.forEach((value, i) => { - if (thenable(value)) { - promises.push((async() => { - this.processedArgs[i] = await value; - })()); - } - }); - - return promises; + return this.processedArgs + .filter(isThenable) + .map((thenable, i) => (async() => { + this.processedArgs[i] = await thenable; + })()); } /** @@ -1299,7 +1290,7 @@ Expecting one of '${allowedValues.join("', '")}'`); */ _chainOrCall(chain, fn) { - if (thenable(chain)) { + if (isThenable(chain)) { // already have a promise, chain callback return chain.then(fn); } @@ -1369,6 +1360,38 @@ Expecting one of '${allowedValues.join("', '")}'`); return target.chained || (target.chained === undefined && this._asyncParsing); } + /** + * @param {Argument|Option} target + * @param {*} value + * @param {*} previous + * @param {Function} handleError + * @return {*} + * @api private + */ + + _parseArgSubroutine(target, value, previous, handleError) { + let result = target.parseArg(value, previous); + if (this._shouldAwait()) { + const chained = this._shouldChainArgParserCalls(target); + const nonVariadicOption = target instanceof Option && !target.variadic; + let handledResult = result; + if (isThenable(result)) { + handledResult = result.then(x => x, handleError); + if (chained) { + result = handledResult; + } // Otherwise pass original thenable to next parser call + } + if (!chained && nonVariadicOption) { + // For variadic options and arguments, it is well-justified to assume previous thenable is handled by parser, but this cannot be assumed for non-variadic options. + // End user might repeat an option that is not supposed to be repeated, i.e. an option whose parser ignores the second parameter, so special non-variadic option handling is necessary. + const name = target.attributeName(); + this._handledNotChainedNonVariadicOptionValues[name] ??= []; + this._handledNotChainedNonVariadicOptionValues[name].push(handledResult); + } + } + return result; + } + /** * Internal implementation shared by ._processArguments() and option and optionEnv event listeners. * @@ -1381,16 +1404,14 @@ Expecting one of '${allowedValues.join("', '")}'`); */ _parseArg(target, value, previous, handleError) { - const handleThenableError = this._shouldAwait() ? handleError : undefined; - try { if (this._shouldChainArgParserCalls(target)) { return this._chainOrCall(previous, (resolvedPrevious) => ( - parseArg(target, value, resolvedPrevious, handleThenableError) + this._parseArgSubroutine(target, value, resolvedPrevious, handleError) )); } - return parseArg(target, value, previous, handleThenableError); + return this._parseArgSubroutine(target, value, previous, handleError); } catch (err) { handleError(err); } @@ -2357,25 +2378,8 @@ function getCommandAndParents(startCommand) { * @api private */ -function thenable(value) { +function isThenable(value) { return typeof value?.then === 'function'; } -/** - * @param {Argument|Option} target - * @param {*} value - * @param {*} previous - * @param {Function} [handleThenableError] - * @returns {*} - * @api private - */ - -function parseArg(target, value, previous, handleThenableError) { - let result = target.parseArg(value, previous); - if (handleThenableError && thenable(result)) { - result = result.then(x => x, handleThenableError); - } - return result; -} - exports.Command = Command; diff --git a/tests/command.parse.test.js b/tests/command.parse.test.js index 2ab5c6696..6ef14330d 100644 --- a/tests/command.parse.test.js +++ b/tests/command.parse.test.js @@ -95,7 +95,7 @@ describe('return type', () => { return value; } - const error = new Error(); + const error = {}; // not Error so that unhandled promise rejections are properly reported errors.push(error); const promise = Promise.reject(error); promises.push(promise); @@ -179,7 +179,7 @@ describe('return type', () => { caught = value; } - expect(errors[0]).toBe(caught); + expect(caught).toBe(errors[0]); expect(mockCoercion).toHaveBeenCalledTimes(3); }); }); From d5799cb813ddbf7d1b8a6ed57cd21bc038b46cc7 Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Thu, 27 Jul 2023 19:50:16 +0300 Subject: [PATCH 16/31] Aggressively prevent unhandled rejections due to overwritten options --- lib/command.js | 99 +++++++++++++++++++++----------------------------- 1 file changed, 41 insertions(+), 58 deletions(-) diff --git a/lib/command.js b/lib/command.js index c7c520b95..da5230c7c 100644 --- a/lib/command.js +++ b/lib/command.js @@ -970,18 +970,19 @@ Expecting one of '${allowedValues.join("', '")}'`); } /** - * @return {Promise[]} * @api private */ - _getOptionResavePromises() { - const chainedPromises = Object.entries(this.opts()) - .filter(([_, value]) => isThenable(value)) - .map(([key, thenable]) => (async() => { - this.setOptionValueWithSource( - key, await thenable, this.getOptionValueSource(key) - ); - })()); + _awaitThenableOptions() { + const chainedPromises = this._shouldAwait() + ? Object.entries(this.opts()) + .filter(([_, value]) => isThenable(value)) + .map(([key, thenable]) => (async() => { + this.setOptionValueWithSource( + key, await thenable, this.getOptionValueSource(key) + ); + })()) + : []; const notChainedNonVariadicPromises = Object.entries( this._handledNotChainedNonVariadicOptionValues @@ -996,49 +997,25 @@ Expecting one of '${allowedValues.join("', '")}'`); await thenable; })()); - return chainedPromises.concat(notChainedNonVariadicPromises); - } - - /** - * @return {Promise[]} - * @api private - */ - - _getArgumentResavePromises() { - return this.processedArgs - .filter(isThenable) - .map((thenable, i) => (async() => { - this.processedArgs[i] = await thenable; - })()); + const toAwait = chainedPromises.concat(notChainedNonVariadicPromises); + if (toAwait.length) return Promise.all(toAwait); } /** * @api private */ - _optionalAwait(getPromises) { + _awaitThenableArguments() { if (this._shouldAwait()) { - const toAwait = getPromises(); + const toAwait = this.processedArgs + .filter(isThenable) + .map((thenable, i) => (async() => { + this.processedArgs[i] = await thenable; + })()); if (toAwait.length) return Promise.all(toAwait); } } - /** - * @api private - */ - - _awaitOptionResavePromises() { - return this._optionalAwait(() => this._getOptionResavePromises()); - } - - /** - * @api private - */ - - _awaitArgumentResavePromises() { - return this._optionalAwait(() => this._getArgumentResavePromises()); - } - /** * Execute a sub-command executable. * @@ -1370,25 +1347,31 @@ Expecting one of '${allowedValues.join("', '")}'`); */ _parseArgSubroutine(target, value, previous, handleError) { + const chained = this._shouldChainArgParserCalls(target); + const notChainedNonVariadicOption = !chained + && target instanceof Option + && !target.variadic; + let result = target.parseArg(value, previous); - if (this._shouldAwait()) { - const chained = this._shouldChainArgParserCalls(target); - const nonVariadicOption = target instanceof Option && !target.variadic; - let handledResult = result; - if (isThenable(result)) { + let handledResult = result; + + if (isThenable(result)) { + if (this._shouldAwait() || notChainedNonVariadicOption) { handledResult = result.then(x => x, handleError); - if (chained) { - result = handledResult; - } // Otherwise pass original thenable to next parser call - } - if (!chained && nonVariadicOption) { - // For variadic options and arguments, it is well-justified to assume previous thenable is handled by parser, but this cannot be assumed for non-variadic options. - // End user might repeat an option that is not supposed to be repeated, i.e. an option whose parser ignores the second parameter, so special non-variadic option handling is necessary. - const name = target.attributeName(); - this._handledNotChainedNonVariadicOptionValues[name] ??= []; - this._handledNotChainedNonVariadicOptionValues[name].push(handledResult); } + if (this._shouldAwait() && chained) { + result = handledResult; + } // otherwise pass original thenable to next parser call } + + if (notChainedNonVariadicOption) { + // For variadic options and arguments, it is well-justified to assume previous thenable is handled by parser, but this cannot be assumed for non-variadic options. + // End user might repeat an option that is not supposed to be repeated, i.e. an option whose parser ignores the second parameter, so special overwritten option handling is necessary. + const name = target.attributeName(); + this._handledNotChainedNonVariadicOptionValues[name] ??= []; + this._handledNotChainedNonVariadicOptionValues[name].push(handledResult); + } + return result; } @@ -1430,7 +1413,7 @@ Expecting one of '${allowedValues.join("', '")}'`); const parsed = this.parseOptions(unknown); this._parseOptionsEnv(); // after cli, so parseArg not called on both cli and env this._parseOptionsImplied(); - chain = this._chainOrCall(chain, () => this._awaitOptionResavePromises()); + chain = this._chainOrCall(chain, () => this._awaitThenableOptions()); operands = operands.concat(parsed.operands); unknown = parsed.unknown; @@ -1465,7 +1448,7 @@ Expecting one of '${allowedValues.join("', '")}'`); }; const processAndAwaitArguments = () => { this._processArguments(); - chain = this._chainOrCall(chain, () => this._awaitArgumentResavePromises()); + chain = this._chainOrCall(chain, () => this._awaitThenableArguments()); }; const commandEvent = `command:${this.name()}`; From f5a05570f6252d6f2db9826be55bea4d5e4c768c Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Thu, 27 Jul 2023 19:53:32 +0300 Subject: [PATCH 17/31] Fix linting errors --- lib/command.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/command.js b/lib/command.js index da5230c7c..b26903b81 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1348,9 +1348,9 @@ Expecting one of '${allowedValues.join("', '")}'`); _parseArgSubroutine(target, value, previous, handleError) { const chained = this._shouldChainArgParserCalls(target); - const notChainedNonVariadicOption = !chained - && target instanceof Option - && !target.variadic; + const notChainedNonVariadicOption = !chained && + target instanceof Option && + !target.variadic; let result = target.parseArg(value, previous); let handledResult = result; From ea251708588074b074917a62fb784e1f8fe8cc0a Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Fri, 28 Jul 2023 00:30:26 +0300 Subject: [PATCH 18/31] Forbid parse() usage with async calls Better than having to deal with unhandled rejections. --- lib/command.js | 10 +++++- lib/error.js | 2 +- tests/argument.custom-processing.test.js | 28 +++++----------- tests/options.custom-processing.test.js | 42 +++++++++--------------- 4 files changed, 35 insertions(+), 47 deletions(-) diff --git a/lib/command.js b/lib/command.js index b26903b81..cab885e4a 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1271,8 +1271,16 @@ Expecting one of '${allowedValues.join("', '")}'`); // already have a promise, chain callback return chain.then(fn); } + // callback might return a promise - return fn(chain); + chain = fn(chain); + if (isThenable(chain) && !this._asyncParsing) { + chain.then(x => x, () => {}); // prevent unhandled rejection + process.exitCode = 1; // additionally indicates to the user that user code (parsers, hooks, action) is not the error source + throw new Error(`.parse() is incompatible with async argParsers, hooks and actions. +Use .parseAsync() instead.`); + } + return chain; } /** diff --git a/lib/error.js b/lib/error.js index e7cde9cc7..7f166a3bd 100644 --- a/lib/error.js +++ b/lib/error.js @@ -9,7 +9,7 @@ class CommanderError extends Error { * Constructs the CommanderError class * @param {number} exitCode suggested exit code which could be used with process.exit * @param {string} code an id string representing the error - * @param {string} message human-readable description of the error + * @param {string} [message] human-readable description of the error * @constructor */ constructor(exitCode, code, message) { diff --git a/tests/argument.custom-processing.test.js b/tests/argument.custom-processing.test.js index 72ce05d1c..16572a329 100644 --- a/tests/argument.custom-processing.test.js +++ b/tests/argument.custom-processing.test.js @@ -206,30 +206,20 @@ test('when custom processing for argument throws plain error then not CommanderE expect(caughtErr).not.toBeInstanceOf(commander.CommanderError); }); -test('when custom variadic with .chainArgParserCalls(true) then parsed to chain', async() => { +test('when async custom variadic then parsed to chain', async() => { const args = ['1', '2']; const resolvedValue = '12'; - const coercion = async(value, previousValue) => ( - previousValue === undefined ? value : previousValue + value + const mockCoercion = jest.fn().mockImplementation( + async(value, previousValue) => ( + previousValue === undefined ? value : previousValue + value + ) ); - const awaited = coercion(args[0], undefined); - const mockCoercion = jest.fn().mockImplementation(coercion); - - const argument = new commander.Argument('', 'desc') - .argParser(mockCoercion) - .chainArgParserCalls(true); - - let actionValue; const program = new commander.Command(); program - .addArgument(argument) - .action((value) => { - actionValue = value; - }); + .argument('', 'desc', mockCoercion) + .action(() => {}); - program.parse(args, { from: 'user' }); - expect(program.processedArgs[0]).toEqual(awaited); - expect(actionValue).toEqual(awaited); - await expect(actionValue).resolves.toEqual(resolvedValue); + await program.parseAsync(args, { from: 'user' }); + expect(program.processedArgs[0]).toEqual(resolvedValue); }); diff --git a/tests/options.custom-processing.test.js b/tests/options.custom-processing.test.js index 499e700cc..e79ee3fff 100644 --- a/tests/options.custom-processing.test.js +++ b/tests/options.custom-processing.test.js @@ -140,48 +140,38 @@ test('when commaSeparatedList x,y,z then value is [x, y, z]', () => { expect(program.opts().list).toEqual(['x', 'y', 'z']); }); -test('when custom non-variadic repeated with .chainArgParserCalls(true) then parsed to chain', async() => { +test('when async custom non-variadic repeated then parsed to chain', async() => { const args = ['-a', '1', '-a', '2']; const resolvedValue = '12'; - const coercion = async(value, previousValue) => ( - previousValue === undefined ? value : previousValue + value + const mockCoercion = jest.fn().mockImplementation( + async(value, previousValue) => ( + previousValue === undefined ? value : previousValue + value + ) ); - const awaited = coercion(args[1], undefined); - const mockCoercion = jest.fn().mockImplementation(coercion); - - const option = new commander.Option('-a ', 'desc') - .argParser(mockCoercion) - .chainArgParserCalls(true); const program = new commander.Command(); program - .addOption(option) + .option('-a ', 'desc', mockCoercion) .action(() => {}); - program.parse(args, { from: 'user' }); - expect(program.opts()).toEqual({ a: awaited }); - await expect(program.opts().a).resolves.toEqual(resolvedValue); + await program.parseAsync(args, { from: 'user' }); + expect(program.opts().a).toEqual(resolvedValue); }); -test('when custom variadic with .chainArgParserCalls(true) then parsed to chain', async() => { +test('when async custom variadic then parsed to chain', async() => { const args = ['-a', '1', '2']; const resolvedValue = '12'; - const coercion = async(value, previousValue) => ( - previousValue === undefined ? value : previousValue + value + const mockCoercion = jest.fn().mockImplementation( + async(value, previousValue) => ( + previousValue === undefined ? value : previousValue + value + ) ); - const awaited = coercion(args[1], undefined); - const mockCoercion = jest.fn().mockImplementation(coercion); - - const option = new commander.Option('-a ', 'desc') - .argParser(mockCoercion) - .chainArgParserCalls(true); const program = new commander.Command(); program - .addOption(option) + .option('-a ', 'desc', mockCoercion) .action(() => {}); - program.parse(args, { from: 'user' }); - expect(program.opts()).toEqual({ a: awaited }); - await expect(program.opts().a).resolves.toEqual(resolvedValue); + await program.parseAsync(args, { from: 'user' }); + expect(program.opts().a).toEqual(resolvedValue); }); From b9643bc082d49e7030cf3e1285125c0c9b58644b Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Fri, 28 Jul 2023 01:28:01 +0300 Subject: [PATCH 19/31] Remove await configuration and unnecessary chain mode The simplification is possible now since async calls are forbidden in parse(). --- lib/argument.js | 12 ++- lib/command.js | 105 ++++++++--------------- lib/option.js | 12 ++- tests/argument.custom-processing.test.js | 13 +-- tests/command.argumentVariations.test.js | 18 ++-- tests/command.await.test.js | 24 ++---- tests/command.chain.test.js | 6 -- tests/options.custom-processing.test.js | 26 +++--- typings/index.d.ts | 17 +--- typings/index.test-d.ts | 8 -- 10 files changed, 89 insertions(+), 152 deletions(-) diff --git a/lib/argument.js b/lib/argument.js index b3e053f88..7bea70ce4 100644 --- a/lib/argument.js +++ b/lib/argument.js @@ -16,8 +16,7 @@ class Argument { this.description = description || ''; this.variadic = false; this.parseArg = undefined; - /** @type {boolean | undefined} */ - this.chained = undefined; + this.chained = true; this.defaultValue = undefined; this.defaultValueDescription = undefined; this.argChoices = undefined; @@ -92,14 +91,13 @@ class Argument { } /** - * When set to `true`, next call to the function provided via `.argParser()` will be chained to its return value if it is thenable. - * When set to `undefined` (the default), only chain when `.parseAsync()` has been called. + * When set to `true` (the default), next call to the function provided via `.argParser()` will be chained to its return value if it is thenable. * - * @param {boolean | undefined} [chained] + * @param {boolean} [chained=true] * @return {Argument} */ - chainArgParserCalls(chained) { - this.chained = chained === undefined ? undefined : !!chained; + chainArgParserCalls(chained = true) { + this.chained = !!chained; return this; } diff --git a/lib/command.js b/lib/command.js index cab885e4a..7ae3dcf5c 100644 --- a/lib/command.js +++ b/lib/command.js @@ -59,8 +59,6 @@ class Command extends EventEmitter { /** @type {boolean | undefined} */ this._asyncParsing = undefined; - /** @type {boolean | undefined} */ - this._await = undefined; this._handledNotChainedNonVariadicOptionValues = {}; // see .configureOutput() for docs @@ -111,7 +109,6 @@ class Command extends EventEmitter { this._enablePositionalOptions = sourceCommand._enablePositionalOptions; this._showHelpAfterError = sourceCommand._showHelpAfterError; this._showSuggestionAfterError = sourceCommand._showSuggestionAfterError; - this._await = sourceCommand._await; return this; } @@ -948,25 +945,15 @@ Expecting one of '${allowedValues.join("', '")}'`); return this; } - /** - * When set to `true`, thenable option and argument values will be awaited right after they are parsed and processed. - * When set to `undefined` (the default), inherit the behaviour from ancestor commands, or only await when `.parseAsync()` has been called. - * Useful for asynchronous custom processing (`parseArg`) of arguments and option-arguments. - * - * @param {boolean | undefined} [enabled] - * @return {Command} `this` command for chaining - */ - await(enabled) { - this._await = enabled === undefined ? undefined : !!enabled; - return this; - } - /** * @api private */ - _shouldAwait() { - return this._await || (this._await === undefined && this._asyncParsing); + _optionalAwait(getPromises) { + if (this._asyncParsing) { + const toAwait = getPromises(); + if (toAwait.length) return Promise.all(toAwait); + } } /** @@ -974,31 +961,27 @@ Expecting one of '${allowedValues.join("', '")}'`); */ _awaitThenableOptions() { - const chainedPromises = this._shouldAwait() - ? Object.entries(this.opts()) + return this._optionalAwait(() => { + const chainedPromises = Object.entries(this.opts()) .filter(([_, value]) => isThenable(value)) .map(([key, thenable]) => (async() => { this.setOptionValueWithSource( key, await thenable, this.getOptionValueSource(key) ); - })()) - : []; + })()); - const notChainedNonVariadicPromises = Object.entries( - this._handledNotChainedNonVariadicOptionValues - ) - .map(([_, values]) => ( - // Only await overwritten thenables so that behaviour is consistent with how not chained non-variadic arguments are handled. - values.slice(0, -1) - )) - .flat() - .filter(isThenable) - .map((thenable) => (async() => { - await thenable; - })()); + const notChainedNonVariadicPromises = Object.entries( + this._handledNotChainedNonVariadicOptionValues + ) + .map(([_, values]) => values.slice(0, -1)) // only overwritten + .flat() + .filter(isThenable) + .map((thenable) => (async() => { + await thenable; + })()); - const toAwait = chainedPromises.concat(notChainedNonVariadicPromises); - if (toAwait.length) return Promise.all(toAwait); + return chainedPromises.concat(notChainedNonVariadicPromises); + }); } /** @@ -1006,14 +989,13 @@ Expecting one of '${allowedValues.join("', '")}'`); */ _awaitThenableArguments() { - if (this._shouldAwait()) { - const toAwait = this.processedArgs + return this._optionalAwait(() => { + return this.processedArgs .filter(isThenable) .map((thenable, i) => (async() => { this.processedArgs[i] = await thenable; })()); - if (toAwait.length) return Promise.all(toAwait); - } + }); } /** @@ -1335,16 +1317,6 @@ Use .parseAsync() instead.`); return result; } - /** - * @param {Argument|Option} target - * @return {boolean} - * @api private - */ - - _shouldChainArgParserCalls(target) { - return target.chained || (target.chained === undefined && this._asyncParsing); - } - /** * @param {Argument|Option} target * @param {*} value @@ -1355,29 +1327,24 @@ Use .parseAsync() instead.`); */ _parseArgSubroutine(target, value, previous, handleError) { - const chained = this._shouldChainArgParserCalls(target); - const notChainedNonVariadicOption = !chained && - target instanceof Option && - !target.variadic; - let result = target.parseArg(value, previous); - let handledResult = result; - if (isThenable(result)) { - if (this._shouldAwait() || notChainedNonVariadicOption) { + if (this._asyncParsing) { + let handledResult = result; + if (isThenable(result)) { handledResult = result.then(x => x, handleError); + if (target.chained) { + result = handledResult; + } // otherwise pass original thenable to next parser call } - if (this._shouldAwait() && chained) { - result = handledResult; - } // otherwise pass original thenable to next parser call - } - if (notChainedNonVariadicOption) { - // For variadic options and arguments, it is well-justified to assume previous thenable is handled by parser, but this cannot be assumed for non-variadic options. - // End user might repeat an option that is not supposed to be repeated, i.e. an option whose parser ignores the second parameter, so special overwritten option handling is necessary. - const name = target.attributeName(); - this._handledNotChainedNonVariadicOptionValues[name] ??= []; - this._handledNotChainedNonVariadicOptionValues[name].push(handledResult); + if (target instanceof Option && !target.chained && !target.variadic) { + // For variadic options and arguments, it is well-justified to assume previous thenable is handled by parser, but this cannot be assumed for non-variadic options. + // End user might repeat an option that is not supposed to be repeated, i.e. an option whose parser ignores the second parameter, so special overwritten option handling is necessary. + const name = target.attributeName(); + this._handledNotChainedNonVariadicOptionValues[name] ??= []; + this._handledNotChainedNonVariadicOptionValues[name].push(handledResult); + } } return result; @@ -1396,7 +1363,7 @@ Use .parseAsync() instead.`); _parseArg(target, value, previous, handleError) { try { - if (this._shouldChainArgParserCalls(target)) { + if (target.chained) { return this._chainOrCall(previous, (resolvedPrevious) => ( this._parseArgSubroutine(target, value, resolvedPrevious, handleError) )); diff --git a/lib/option.js b/lib/option.js index 473c32295..78a83f92e 100644 --- a/lib/option.js +++ b/lib/option.js @@ -31,8 +31,7 @@ class Option { this.presetArg = undefined; this.envVar = undefined; this.parseArg = undefined; - /** @type {boolean | undefined} */ - this.chained = undefined; + this.chained = true; this.hidden = false; this.argChoices = undefined; this.conflictsWith = []; @@ -138,14 +137,13 @@ class Option { } /** - * When set to `true`, next call to the function provided via `.argParser()` will be chained to its return value if it is thenable. - * When set to `undefined` (the default), only chain when `.parseAsync()` has been called. + * When set to `true` (the default), next call to the function provided via `.argParser()` will be chained to its return value if it is thenable. * - * @param {boolean | undefined} [chained] + * @param {boolean} [chained=true] * @return {Argument} */ - chainArgParserCalls(chained) { - this.chained = chained === undefined ? undefined : !!chained; + chainArgParserCalls(chained = true) { + this.chained = !!chained; return this; } diff --git a/tests/argument.custom-processing.test.js b/tests/argument.custom-processing.test.js index 16572a329..52a5c098f 100644 --- a/tests/argument.custom-processing.test.js +++ b/tests/argument.custom-processing.test.js @@ -209,17 +209,18 @@ test('when custom processing for argument throws plain error then not CommanderE test('when async custom variadic then parsed to chain', async() => { const args = ['1', '2']; const resolvedValue = '12'; - const mockCoercion = jest.fn().mockImplementation( - async(value, previousValue) => ( - previousValue === undefined ? value : previousValue + value - ) + const coercion = async(value, previousValue) => ( + previousValue === undefined ? value : previousValue + value ); + const awaited = coercion(args[0], undefined); + const mockCoercion = jest.fn().mockImplementation(coercion); const program = new commander.Command(); program .argument('', 'desc', mockCoercion) .action(() => {}); - await program.parseAsync(args, { from: 'user' }); - expect(program.processedArgs[0]).toEqual(resolvedValue); + program.parseAsync(args, { from: 'user' }); + expect(program.processedArgs[0]).toEqual(awaited); + await expect(program.processedArgs[0]).resolves.toEqual(resolvedValue); }); diff --git a/tests/command.argumentVariations.test.js b/tests/command.argumentVariations.test.js index a8a781625..0591f140d 100644 --- a/tests/command.argumentVariations.test.js +++ b/tests/command.argumentVariations.test.js @@ -9,7 +9,8 @@ test.each(getSingleArgCases(''))('when add "" using %s t _name: 'explicit-required', required: true, variadic: false, - description: '' + description: '', + chained: true }; expect(argument).toEqual(expectedShape); }); @@ -20,7 +21,8 @@ test.each(getSingleArgCases('implicit-required'))('when add "arg" using %s then _name: 'implicit-required', required: true, variadic: false, - description: '' + description: '', + chained: true }; expect(argument).toEqual(expectedShape); }); @@ -31,7 +33,8 @@ test.each(getSingleArgCases('[optional]'))('when add "[arg]" using %s then argum _name: 'optional', required: false, variadic: false, - description: '' + description: '', + chained: true }; expect(argument).toEqual(expectedShape); }); @@ -42,7 +45,8 @@ test.each(getSingleArgCases(''))('when add "" usin _name: 'explicit-required', required: true, variadic: true, - description: '' + description: '', + chained: true }; expect(argument).toEqual(expectedShape); }); @@ -53,7 +57,8 @@ test.each(getSingleArgCases('implicit-required...'))('when add "arg..." using %s _name: 'implicit-required', required: true, variadic: true, - description: '' + description: '', + chained: true }; expect(argument).toEqual(expectedShape); }); @@ -64,7 +69,8 @@ test.each(getSingleArgCases('[optional...]'))('when add "[arg...]" using %s then _name: 'optional', required: false, variadic: true, - description: '' + description: '', + chained: true }; expect(argument).toEqual(expectedShape); }); diff --git a/tests/command.await.test.js b/tests/command.await.test.js index 84cf543eb..2d8178175 100644 --- a/tests/command.await.test.js +++ b/tests/command.await.test.js @@ -80,7 +80,7 @@ describe('await arguments', () => { await testWithArguments(program, args, resolvedValues, awaited); }); - test('when variadic argument with chained asynchronous custom processing then .processedArgs and action arguments resolved from chain', async() => { + test('when variadic argument with asynchronous custom processing then .processedArgs and action arguments resolved from chain', async() => { const args = ['1', '2']; const resolvedValues = ['12']; const coercion = (value, previousValue) => { @@ -92,13 +92,9 @@ describe('await arguments', () => { const awaited = [(await chainedAwaited(coercion, args)).thenable]; const mockCoercion = jest.fn().mockImplementation(coercion); - const argument = new commander.Argument('', 'desc') - .argParser(mockCoercion) - .chainArgParserCalls(); - const program = new commander.Command(); program - .addArgument(argument); + .argument('', 'desc', mockCoercion); await testWithArguments(program, args, resolvedValues, awaited); }); @@ -178,7 +174,7 @@ describe('await options', () => { expect(program.getOptionValueSource('b')).toEqual('implied'); }); - test('when non-variadic repeated option with chained asynchronous custom processing then .opts() resolved from chain', async() => { + test('when non-variadic repeated option with asynchronous custom processing then .opts() resolved from chain', async() => { const args = ['-a', '1', '-a', '2']; const resolvedValues = { a: '12' }; const coercion = (value, previousValue) => { @@ -194,19 +190,15 @@ describe('await options', () => { }; const mockCoercion = jest.fn().mockImplementation(coercion); - const option = new commander.Option('-a [arg]', 'desc') - .argParser(mockCoercion) - .chainArgParserCalls(); - const program = new commander.Command(); program - .addOption(option); + .option('-a [arg]', 'desc', mockCoercion); await testWithOptions(program, args, resolvedValues, awaited); expect(program.getOptionValueSource('a')).toEqual('cli'); }); - test('when variadic option with chained asynchronous custom processing then .opts() resolved from chain', async() => { + test('when variadic option with asynchronous custom processing then .opts() resolved from chain', async() => { const args = ['-a', '1', '2']; const resolvedValues = { a: '12' }; const coercion = (value, previousValue) => { @@ -222,13 +214,9 @@ describe('await options', () => { }; const mockCoercion = jest.fn().mockImplementation(coercion); - const option = new commander.Option('-a ', 'desc') - .argParser(mockCoercion) - .chainArgParserCalls(); - const program = new commander.Command(); program - .addOption(option); + .option('-a ', 'desc', mockCoercion); await testWithOptions(program, args, resolvedValues, awaited); expect(program.getOptionValueSource('a')).toEqual('cli'); diff --git a/tests/command.chain.test.js b/tests/command.chain.test.js index 7e60cf80d..3f907c869 100644 --- a/tests/command.chain.test.js +++ b/tests/command.chain.test.js @@ -166,12 +166,6 @@ describe('Command methods that should return this for chaining', () => { expect(result).toBe(program); }); - test('when call .await() then returns this', () => { - const program = new Command(); - const result = program.await(); - expect(result).toBe(program); - }); - test('when call .hook() then returns this', () => { const program = new Command(); const result = program.hook('preAction', () => {}); diff --git a/tests/options.custom-processing.test.js b/tests/options.custom-processing.test.js index e79ee3fff..08613c67a 100644 --- a/tests/options.custom-processing.test.js +++ b/tests/options.custom-processing.test.js @@ -143,35 +143,37 @@ test('when commaSeparatedList x,y,z then value is [x, y, z]', () => { test('when async custom non-variadic repeated then parsed to chain', async() => { const args = ['-a', '1', '-a', '2']; const resolvedValue = '12'; - const mockCoercion = jest.fn().mockImplementation( - async(value, previousValue) => ( - previousValue === undefined ? value : previousValue + value - ) + const coercion = async(value, previousValue) => ( + previousValue === undefined ? value : previousValue + value ); + const awaited = coercion(args[1], undefined); + const mockCoercion = jest.fn().mockImplementation(coercion); const program = new commander.Command(); program .option('-a ', 'desc', mockCoercion) .action(() => {}); - await program.parseAsync(args, { from: 'user' }); - expect(program.opts().a).toEqual(resolvedValue); + program.parseAsync(args, { from: 'user' }); + expect(program.opts()).toEqual({ a: awaited }); + await expect(program.opts().a).resolves.toEqual(resolvedValue); }); test('when async custom variadic then parsed to chain', async() => { const args = ['-a', '1', '2']; const resolvedValue = '12'; - const mockCoercion = jest.fn().mockImplementation( - async(value, previousValue) => ( - previousValue === undefined ? value : previousValue + value - ) + const coercion = async(value, previousValue) => ( + previousValue === undefined ? value : previousValue + value ); + const awaited = coercion(args[1], undefined); + const mockCoercion = jest.fn().mockImplementation(coercion); const program = new commander.Command(); program .option('-a ', 'desc', mockCoercion) .action(() => {}); - await program.parseAsync(args, { from: 'user' }); - expect(program.opts().a).toEqual(resolvedValue); + program.parseAsync(args, { from: 'user' }); + expect(program.opts()).toEqual({ a: awaited }); + await expect(program.opts().a).resolves.toEqual(resolvedValue); }); diff --git a/typings/index.d.ts b/typings/index.d.ts index e6a619250..1a065f5f8 100644 --- a/typings/index.d.ts +++ b/typings/index.d.ts @@ -66,10 +66,9 @@ export class Argument { argParser(fn: (value: string, previous: T) => T): this; /** - * When set to `true`, next call to the function provided via `.argParser()` will be chained to its return value if it is thenable. - * When set to `undefined` (the default), only chain when `.parseAsync()` has been called. + * When set to `true` (the default), next call to the function provided via `.argParser()` will be chained to its return value if it is thenable. */ - chainArgParserCalls(chained?: boolean | undefined): this; + chainArgParserCalls(chained?: boolean): this; /** * Only allow argument value to be one of choices. @@ -166,10 +165,9 @@ export class Option { argParser(fn: (value: string, previous: T) => T): this; /** - * When set to `true`, next call to the function provided via `.argParser()` will be chained to its return value if it is thenable. - * When set to `undefined` (the default), only chain when `.parseAsync()` has been called. + * When set to `true` (the default), next call to the function provided via `.argParser()` will be chained to its return value if it is thenable. */ - chainArgParserCalls(chained?: boolean | undefined): this; + chainArgParserCalls(chained?: boolean): this; /** * Whether the option is mandatory and must have a value after parsing. @@ -704,13 +702,6 @@ export class Command { */ parseAsync(argv?: readonly string[], options?: ParseOptions): Promise; - /** - * When set to `true`, thenable option and argument values will be awaited right after they are parsed and processed. - * When set to `undefined` (the default), inherit the behaviour from ancestor commands, or only await when `.parseAsync()` has been called. - * Useful for asynchronous custom processing (`parseArg`) of arguments and option-arguments. - */ - await(enabled?: boolean | undefined): this; - /** * Parse options from `argv` removing known options, * and return argv split into operands and unknown arguments. diff --git a/typings/index.test-d.ts b/typings/index.test-d.ts index cf4c8a490..7c1f5dd6c 100644 --- a/typings/index.test-d.ts +++ b/typings/index.test-d.ts @@ -214,12 +214,6 @@ expectType>(program.parseAsync(['node', 'script.js'], expectType>(program.parseAsync(['--option'], { from: 'user' })); expectType>(program.parseAsync(['node', 'script.js'] as const)); -// await -expectType(program.await()); -expectType(program.await(true)); -expectType(program.await(false)); -expectType(program.await(undefined)); - // parseOptions (and ParseOptionsResult) expectType<{ operands: string[]; unknown: string[] }>(program.parseOptions(['node', 'script.js', 'hello'])); @@ -428,7 +422,6 @@ expectType(baseOption.argParser((value: string, previous: stri expectType(baseOption.chainArgParserCalls()); expectType(baseOption.chainArgParserCalls(true)); expectType(baseOption.chainArgParserCalls(false)); -expectType(baseOption.chainArgParserCalls(undefined)); // makeOptionMandatory expectType(baseOption.makeOptionMandatory()); @@ -482,7 +475,6 @@ expectType(baseArgument.argParser((value: string, previous: expectType(baseArgument.chainArgParserCalls()); expectType(baseArgument.chainArgParserCalls(true)); expectType(baseArgument.chainArgParserCalls(false)); -expectType(baseArgument.chainArgParserCalls(undefined)); // choices expectType(baseArgument.choices(['a', 'b'])); From 385fe48b00ce0bfdab8702bf83e899893ef65b5f Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Fri, 28 Jul 2023 10:50:53 +0300 Subject: [PATCH 20/31] Warn about incorrect parse() instead of throwing --- lib/command.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/command.js b/lib/command.js index 7ae3dcf5c..02bc495d4 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1256,10 +1256,8 @@ Expecting one of '${allowedValues.join("', '")}'`); // callback might return a promise chain = fn(chain); - if (isThenable(chain) && !this._asyncParsing) { - chain.then(x => x, () => {}); // prevent unhandled rejection - process.exitCode = 1; // additionally indicates to the user that user code (parsers, hooks, action) is not the error source - throw new Error(`.parse() is incompatible with async argParsers, hooks and actions. + if (!this._asyncParsing && isThenable(chain) && process.env.NODE_ENV !== 'production') { + console.warn(`.parse() is incompatible with async argParsers, hooks and actions. Use .parseAsync() instead.`); } return chain; From d9dcc57ba57df907b8391d9533aabe0860be18a7 Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Fri, 28 Jul 2023 13:04:39 +0300 Subject: [PATCH 21/31] Separation of concerns: add callback to _parseArg() --- lib/command.js | 65 +++++++++++++++++++++++++------------------------- 1 file changed, 33 insertions(+), 32 deletions(-) diff --git a/lib/command.js b/lib/command.js index 02bc495d4..04bcb64f9 100644 --- a/lib/command.js +++ b/lib/command.js @@ -530,14 +530,6 @@ Expecting one of '${allowedValues.join("', '")}'`); // handler for cli and env supplied values const handleOptionValue = (val, invalidValueMessage, valueSource) => { - const handleError = (err) => { - if (err?.code === 'commander.invalidArgument') { - const message = `${invalidValueMessage} ${err.message}`; - this.error(message, { exitCode: err.exitCode, code: err.code }); - } - throw err; - }; - // val is null for optional option used without an optional-argument. // val is undefined for boolean and negated option. if (val == null && option.presetArg !== undefined) { @@ -547,7 +539,22 @@ Expecting one of '${allowedValues.join("', '")}'`); // custom processing const oldValue = this.getOptionValue(name); if (val !== null && option.parseArg) { - val = this._parseArg(option, val, oldValue, handleError); + const handleError = (err) => { + if (err?.code === 'commander.invalidArgument') { + const message = `${invalidValueMessage} ${err.message}`; + this.error(message, { exitCode: err.exitCode, code: err.code }); + } + throw err; + }; + const handledResultCallback = (handledResult) => { + if (!option.chained && !option.variadic) { + // For variadic options and arguments, it is well-justified to assume previous thenable is handled by parser, but this cannot be assumed for non-variadic options. + // End user might repeat an option that is not supposed to be repeated, i.e. an option whose parser ignores the second parameter, so special overwritten option handling is necessary. + this._handledNotChainedNonVariadicOptionValues[name] ??= []; + this._handledNotChainedNonVariadicOptionValues[name].push(handledResult); + } + }; + val = this._parseArg(option, val, oldValue, handleError, handledResultCallback); } else if (val !== null && option.variadic) { val = option._concatValue(val, oldValue); } @@ -1196,16 +1203,15 @@ Expecting one of '${allowedValues.join("', '")}'`); _processArguments() { const myParseArg = (argument, value, previous) => { - const handleError = (err) => { - if (err?.code === 'commander.invalidArgument') { - const message = `error: command-argument value '${value}' is invalid for argument '${argument.name()}'. ${err.message}`; - this.error(message, { exitCode: err.exitCode, code: err.code }); - } - throw err; - }; - let parsedValue = value; if (value !== null && argument.parseArg) { + const handleError = (err) => { + if (err?.code === 'commander.invalidArgument') { + const message = `error: command-argument value '${value}' is invalid for argument '${argument.name()}'. ${err.message}`; + this.error(message, { exitCode: err.exitCode, code: err.code }); + } + throw err; + }; parsedValue = this._parseArg(argument, value, previous, handleError); } return parsedValue; @@ -1320,13 +1326,13 @@ Use .parseAsync() instead.`); * @param {*} value * @param {*} previous * @param {Function} handleError - * @return {*} + * @param {Function} [handledResultCallback] + * @returns {*} * @api private */ - _parseArgSubroutine(target, value, previous, handleError) { + _parseArgSubroutine(target, value, previous, handleError, handledResultCallback) { let result = target.parseArg(value, previous); - if (this._asyncParsing) { let handledResult = result; if (isThenable(result)) { @@ -1335,16 +1341,8 @@ Use .parseAsync() instead.`); result = handledResult; } // otherwise pass original thenable to next parser call } - - if (target instanceof Option && !target.chained && !target.variadic) { - // For variadic options and arguments, it is well-justified to assume previous thenable is handled by parser, but this cannot be assumed for non-variadic options. - // End user might repeat an option that is not supposed to be repeated, i.e. an option whose parser ignores the second parameter, so special overwritten option handling is necessary. - const name = target.attributeName(); - this._handledNotChainedNonVariadicOptionValues[name] ??= []; - this._handledNotChainedNonVariadicOptionValues[name].push(handledResult); - } + handledResultCallback?.(handledResult); } - return result; } @@ -1355,19 +1353,22 @@ Use .parseAsync() instead.`); * @param {*} value * @param {*} previous * @param {Function} handleError + * @param {Function} [handledResultCallback] * @return {*} * @api private */ - _parseArg(target, value, previous, handleError) { + _parseArg(target, value, previous, handleError, handledResultCallback) { try { if (target.chained) { return this._chainOrCall(previous, (resolvedPrevious) => ( - this._parseArgSubroutine(target, value, resolvedPrevious, handleError) + this._parseArgSubroutine( + target, value, resolvedPrevious, handleError, handledResultCallback + ) )); } - return this._parseArgSubroutine(target, value, previous, handleError); + return this._parseArgSubroutine(...arguments); } catch (err) { handleError(err); } From 3e0e30517b08719b9e2ddf5a82181d7d06e48fbb Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Fri, 28 Jul 2023 13:14:36 +0300 Subject: [PATCH 22/31] Position awaiting methods better --- lib/command.js | 106 ++++++++++++++++++++++++------------------------- 1 file changed, 53 insertions(+), 53 deletions(-) diff --git a/lib/command.js b/lib/command.js index 04bcb64f9..4ec660b7f 100644 --- a/lib/command.js +++ b/lib/command.js @@ -952,59 +952,6 @@ Expecting one of '${allowedValues.join("', '")}'`); return this; } - /** - * @api private - */ - - _optionalAwait(getPromises) { - if (this._asyncParsing) { - const toAwait = getPromises(); - if (toAwait.length) return Promise.all(toAwait); - } - } - - /** - * @api private - */ - - _awaitThenableOptions() { - return this._optionalAwait(() => { - const chainedPromises = Object.entries(this.opts()) - .filter(([_, value]) => isThenable(value)) - .map(([key, thenable]) => (async() => { - this.setOptionValueWithSource( - key, await thenable, this.getOptionValueSource(key) - ); - })()); - - const notChainedNonVariadicPromises = Object.entries( - this._handledNotChainedNonVariadicOptionValues - ) - .map(([_, values]) => values.slice(0, -1)) // only overwritten - .flat() - .filter(isThenable) - .map((thenable) => (async() => { - await thenable; - })()); - - return chainedPromises.concat(notChainedNonVariadicPromises); - }); - } - - /** - * @api private - */ - - _awaitThenableArguments() { - return this._optionalAwait(() => { - return this.processedArgs - .filter(isThenable) - .map((thenable, i) => (async() => { - this.processedArgs[i] = await thenable; - })()); - }); - } - /** * Execute a sub-command executable. * @@ -1374,6 +1321,59 @@ Use .parseAsync() instead.`); } } + /** + * @api private + */ + + _optionalAwait(getPromises) { + if (this._asyncParsing) { + const toAwait = getPromises(); + if (toAwait.length) return Promise.all(toAwait); + } + } + + /** + * @api private + */ + + _awaitThenableOptions() { + return this._optionalAwait(() => { + const chainedPromises = Object.entries(this.opts()) + .filter(([_, value]) => isThenable(value)) + .map(([key, thenable]) => (async() => { + this.setOptionValueWithSource( + key, await thenable, this.getOptionValueSource(key) + ); + })()); + + const notChainedNonVariadicPromises = Object.entries( + this._handledNotChainedNonVariadicOptionValues + ) + .map(([_, values]) => values.slice(0, -1)) // only overwritten + .flat() + .filter(isThenable) + .map((thenable) => (async() => { + await thenable; + })()); + + return chainedPromises.concat(notChainedNonVariadicPromises); + }); + } + + /** + * @api private + */ + + _awaitThenableArguments() { + return this._optionalAwait(() => { + return this.processedArgs + .filter(isThenable) + .map((thenable, i) => (async() => { + this.processedArgs[i] = await thenable; + })()); + }); + } + /** * Process arguments in context of this command. * Returns action result, in case it is a promise. From 608d91c7d641059b50e0fe6a1741c192c93214aa Mon Sep 17 00:00:00 2001 From: aweebit <36817090+aweebit@users.noreply.github.com> Date: Fri, 28 Jul 2023 16:38:47 +0200 Subject: [PATCH 23/31] Update life cycle description --- docs/parsing-and-hooks.md | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/docs/parsing-and-hooks.md b/docs/parsing-and-hooks.md index 122cad6f7..8fd801c22 100644 --- a/docs/parsing-and-hooks.md +++ b/docs/parsing-and-hooks.md @@ -1,23 +1,29 @@ # Parsing life cycle and hooks -The processing starts with an array of args. Each command processes and removes the options it understands, and passes the remaining args to the next subcommand. -The final command calls the action handler. +The processing starts with an array of args. Each command processes and removes the options it understands, and passes the remaining args to the next subcommand. The final command calls the action handler. Starting with top-level command (program): -- parse options: parse recognised options (for this command) and remove from args -- parse env: look for environment variables (for this command) -- process implied: set any implied option values (for this command) -- if the first arg is a subcommand - - call `preSubcommand` hooks - - pass remaining arguments to subcommand, and process same way +* process options for this command (includes calling or chaining provided argParsers[^1]) + * recognised options from args (and remove them) + * options based on environment variables +* set implied option values (for this command) +* await thenable option values if parsing with `parseAsync()` +* if the first arg is a subcommand + * call or chain `preSubcommand` hooks[^1] + * pass remaining arguments to subcommand, and process same way Once reach final (leaf) command: -- check for missing mandatory options -- check for conflicting options -- check for unknown options -- process remaining args as command-arguments -- call `preAction` hooks -- call action handler -- call `postAction` hooks +* check for missing mandatory options +* check for conflicting options +* check for unknown options +* process remaining args as command-arguments (includes calling or chaining provided argParsers[^1]) +* await thenable argument values if parsing with `parseAsync()` +* if an action handler is provided + * call or chain `preAction` hooks[^1] + * call or chain action handler[^1] + * call or chain `postAction` hooks[^1] + + +[^1]: output a warning suggesting use of `parseAsync()` when parsing with `parse()` and a thenable is returned, unless `process.env.NODE_ENV === 'production'` From 249a0c66b4d5b96b0cafdb712b0852b2e8853f10 Mon Sep 17 00:00:00 2001 From: aweebit <36817090+aweebit@users.noreply.github.com> Date: Fri, 28 Jul 2023 16:51:01 +0200 Subject: [PATCH 24/31] Update life cycle description: clarify and better match source code --- docs/parsing-and-hooks.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/parsing-and-hooks.md b/docs/parsing-and-hooks.md index 8fd801c22..2dbecff89 100644 --- a/docs/parsing-and-hooks.md +++ b/docs/parsing-and-hooks.md @@ -5,9 +5,10 @@ The processing starts with an array of args. Each command processes and removes Starting with top-level command (program): * process options for this command (includes calling or chaining provided argParsers[^1]) - * recognised options from args (and remove them) + * recognised options from args * options based on environment variables * set implied option values (for this command) +* remove recognised options from args * await thenable option values if parsing with `parseAsync()` * if the first arg is a subcommand * call or chain `preSubcommand` hooks[^1] From bf7e096cf73f5ddba284e7d4fbcfc033ef9762bc Mon Sep 17 00:00:00 2001 From: aweebit <36817090+aweebit@users.noreply.github.com> Date: Fri, 28 Jul 2023 17:38:26 +0200 Subject: [PATCH 25/31] Adhere to established dot usage pattern --- lib/command.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/command.js b/lib/command.js index 4ec660b7f..836153adb 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1211,7 +1211,7 @@ Expecting one of '${allowedValues.join("', '")}'`); chain = fn(chain); if (!this._asyncParsing && isThenable(chain) && process.env.NODE_ENV !== 'production') { console.warn(`.parse() is incompatible with async argParsers, hooks and actions. -Use .parseAsync() instead.`); +Use .parseAsync() instead`); } return chain; } From 6858bdf5626ba0b6a58b8ed8ba35831223e03337 Mon Sep 17 00:00:00 2001 From: aweebit <36817090+aweebit@users.noreply.github.com> Date: Sat, 29 Jul 2023 17:05:51 +0300 Subject: [PATCH 26/31] Place async argParser warning better --- lib/command.js | 186 ++++++++++++++++++++++++------------------------- 1 file changed, 93 insertions(+), 93 deletions(-) diff --git a/lib/command.js b/lib/command.js index 836153adb..6877b8840 100644 --- a/lib/command.js +++ b/lib/command.js @@ -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`. @@ -916,10 +918,8 @@ Expecting one of '${allowedValues.join("', '")}'`); */ parse(argv, parseOptions) { - this._asyncParsing = false; - const userArgs = this._prepareUserArgs(argv, parseOptions); - this._parseCommand([], userArgs); + this._parseCommand([], userArgs, false); return this; } @@ -944,10 +944,8 @@ Expecting one of '${allowedValues.join("', '")}'`); */ async parseAsync(argv, parseOptions) { - this._asyncParsing = true; - const userArgs = this._prepareUserArgs(argv, parseOptions); - await this._parseCommand([], userArgs); + await this._parseCommand([], userArgs, true); return this; } @@ -1086,7 +1084,6 @@ 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'); @@ -1094,7 +1091,7 @@ Expecting one of '${allowedValues.join("', '")}'`); if (subCommand._executableHandler) { this._executeSubCommand(subCommand, operands.concat(unknown)); } else { - return subCommand._parseCommand(operands, unknown); + return subCommand._parseCommand(operands, unknown, this._asyncParsing); } }); return hookResult; @@ -1206,14 +1203,8 @@ Expecting one of '${allowedValues.join("', '")}'`); // already have a promise, chain callback return chain.then(fn); } - // callback might return a promise - chain = fn(chain); - if (!this._asyncParsing && isThenable(chain) && process.env.NODE_ENV !== 'production') { - console.warn(`.parse() is incompatible with async argParsers, hooks and actions. -Use .parseAsync() instead`); - } - return chain; + return fn(chain); } /** @@ -1307,15 +1298,18 @@ Use .parseAsync() instead`); _parseArg(target, value, previous, handleError, handledResultCallback) { try { - if (target.chained) { - return this._chainOrCall(previous, (resolvedPrevious) => ( + const result = target.chained + ? this._chainOrCall(previous, (resolvedPrevious) => ( this._parseArgSubroutine( target, value, resolvedPrevious, handleError, handledResultCallback ) - )); + )) + : this._parseArgSubroutine(...arguments) + if (!PRODUCTION && !this._asyncParsing && isThenable(result)) { + console.warn(`.parse() is incompatible with async argParsers. +Use .parseAsync() instead`); } - - return this._parseArgSubroutine(...arguments); + return result; } catch (err) { handleError(err); } @@ -1381,93 +1375,99 @@ Use .parseAsync() instead`); * @api private */ - _parseCommand(operands, unknown) { - let chain; - - const parsed = this.parseOptions(unknown); - this._parseOptionsEnv(); // after cli, so parseArg not called on both cli and env - this._parseOptionsImplied(); - chain = this._chainOrCall(chain, () => this._awaitThenableOptions()); - - operands = operands.concat(parsed.operands); - unknown = parsed.unknown; - this.args = operands.concat(unknown); - - if (operands && this._findCommand(operands[0])) { - chain = this._chainOrCall(chain, () => this._dispatchSubcommand( - operands[0], operands.slice(1), unknown - )); - } else if (this._hasImplicitHelpCommand() && operands[0] === this._helpCommandName) { - chain = this._chainOrCall(chain, () => this._dispatchHelpCommand(operands[1])); - } else if (this._defaultCommandName) { - outputHelpIfRequested(this, unknown); // Run the help for default command from parent rather than passing to default command - chain = this._chainOrCall(chain, () => this._dispatchSubcommand( - this._defaultCommandName, operands, unknown - )); - } else { - if (this.commands.length && this.args.length === 0 && !this._actionHandler && !this._defaultCommandName) { - // probably missing subcommand and no handler, user needs help (and exit) - this.help({ error: true }); - } + _parseCommand(operands, unknown, async) { + this._asyncParsing = async; + + try { + let chain; + + const parsed = this.parseOptions(unknown); + this._parseOptionsEnv(); // after cli, so parseArg not called on both cli and env + this._parseOptionsImplied(); + chain = this._chainOrCall(chain, () => this._awaitThenableOptions()); - outputHelpIfRequested(this, parsed.unknown); - this._checkForMissingMandatoryOptions(); - this._checkForConflictingOptions(); + operands = operands.concat(parsed.operands); + unknown = parsed.unknown; + this.args = operands.concat(unknown); - // We do not always call this check to avoid masking a "better" error, like unknown command. - const checkForUnknownOptions = () => { - if (parsed.unknown.length > 0) { - this.unknownOption(parsed.unknown[0]); + if (operands && this._findCommand(operands[0])) { + chain = this._chainOrCall(chain, () => this._dispatchSubcommand( + operands[0], operands.slice(1), unknown + )); + } else if (this._hasImplicitHelpCommand() && operands[0] === this._helpCommandName) { + chain = this._chainOrCall(chain, () => this._dispatchHelpCommand(operands[1])); + } else if (this._defaultCommandName) { + outputHelpIfRequested(this, unknown); // Run the help for default command from parent rather than passing to default command + chain = this._chainOrCall(chain, () => this._dispatchSubcommand( + this._defaultCommandName, operands, unknown + )); + } else { + if (this.commands.length && this.args.length === 0 && !this._actionHandler && !this._defaultCommandName) { + // probably missing subcommand and no handler, user needs help (and exit) + this.help({ error: true }); } - }; - const processAndAwaitArguments = () => { - this._processArguments(); - chain = this._chainOrCall(chain, () => this._awaitThenableArguments()); - }; - const commandEvent = `command:${this.name()}`; - if (this._actionHandler) { - checkForUnknownOptions(); - processAndAwaitArguments(); - chain = this._chainOrCallHooks(chain, 'preAction'); - chain = this._chainOrCall(chain, () => this._actionHandler(this.processedArgs)); - if (this.parent) { + outputHelpIfRequested(this, parsed.unknown); + this._checkForMissingMandatoryOptions(); + this._checkForConflictingOptions(); + + // We do not always call this check to avoid masking a "better" error, like unknown command. + const checkForUnknownOptions = () => { + if (parsed.unknown.length > 0) { + this.unknownOption(parsed.unknown[0]); + } + }; + const processAndAwaitArguments = () => { + this._processArguments(); + chain = this._chainOrCall(chain, () => this._awaitThenableArguments()); + }; + + const commandEvent = `command:${this.name()}`; + if (this._actionHandler) { + checkForUnknownOptions(); + processAndAwaitArguments(); + chain = this._chainOrCallHooks(chain, 'preAction'); + chain = this._chainOrCall(chain, () => this._actionHandler(this.processedArgs)); + if (this.parent) { + chain = this._chainOrCall(chain, () => { + this.parent.emit(commandEvent, operands, unknown); // legacy + }); + } + chain = this._chainOrCallHooks(chain, 'postAction'); + } else if (this.parent && this.parent.listenerCount(commandEvent)) { + checkForUnknownOptions(); + processAndAwaitArguments(); chain = this._chainOrCall(chain, () => { this.parent.emit(commandEvent, operands, unknown); // legacy }); - } - chain = this._chainOrCallHooks(chain, 'postAction'); - } else if (this.parent && this.parent.listenerCount(commandEvent)) { - checkForUnknownOptions(); - processAndAwaitArguments(); - chain = this._chainOrCall(chain, () => { - this.parent.emit(commandEvent, operands, unknown); // legacy - }); - } else if (operands.length) { - if (this._findCommand('*')) { // legacy default command - return this._dispatchSubcommand('*', operands, unknown); - } - if (this.listenerCount('command:*')) { - // skip option check, emit event for possible misspelling suggestion - this.emit('command:*', operands, unknown); + } else if (operands.length) { + if (this._findCommand('*')) { // legacy default command + return this._dispatchSubcommand('*', operands, unknown); + } + if (this.listenerCount('command:*')) { + // skip option check, emit event for possible misspelling suggestion + this.emit('command:*', operands, unknown); + } else if (this.commands.length) { + this.unknownCommand(); + } else { + checkForUnknownOptions(); + processAndAwaitArguments(); + } } else if (this.commands.length) { - this.unknownCommand(); + checkForUnknownOptions(); + // This command has subcommands and nothing hooked up at this level, so display help (and exit). + this.help({ error: true }); } else { checkForUnknownOptions(); processAndAwaitArguments(); + // fall through for caller to handle after calling .parse() } - } else if (this.commands.length) { - checkForUnknownOptions(); - // This command has subcommands and nothing hooked up at this level, so display help (and exit). - this.help({ error: true }); - } else { - checkForUnknownOptions(); - processAndAwaitArguments(); - // fall through for caller to handle after calling .parse() } - } - return chain; + return chain; + } finally { + this._asyncParsing = undefined; + } } /** From 1cf9fed6471cca939cd3e9bdef8dfdecf3f101e9 Mon Sep 17 00:00:00 2001 From: aweebit <36817090+aweebit@users.noreply.github.com> Date: Sat, 29 Jul 2023 17:29:36 +0300 Subject: [PATCH 27/31] Add thenable default option value warning --- lib/command.js | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/lib/command.js b/lib/command.js index 6877b8840..bb3c457c3 100644 --- a/lib/command.js +++ b/lib/command.js @@ -517,14 +517,21 @@ Expecting one of '${allowedValues.join("', '")}'`); const name = option.attributeName(); // store default value - if (option.negate) { + const setDefaultOptionValue = (value) => { + this.setOptionValueWithSource(name, value, 'default'); + if (!PRODUCTION && !this._asyncParsing && isThenable(value)) { + console.warn(`.parse() is incompatible with thenable default option values. +Use .parseAsync() instead`); + } + }; + if (option.defaultValue !== undefined || !option.negate) { + setDefaultOptionValue(option.defaultValue); + } else { // --no-foo is special and defaults foo to true, unless a --foo option is already defined const positiveLongFlag = option.long.replace(/^--no-/, '--'); if (!this._findOption(positiveLongFlag)) { - this.setOptionValueWithSource(name, option.defaultValue === undefined ? true : option.defaultValue, 'default'); + setDefaultOptionValue(true); } - } else if (option.defaultValue !== undefined) { - this.setOptionValueWithSource(name, option.defaultValue, 'default'); } // register the option From aa6f7c9e9a8877535d5aee0d8acb03a89866e8b3 Mon Sep 17 00:00:00 2001 From: aweebit <36817090+aweebit@users.noreply.github.com> Date: Sat, 29 Jul 2023 17:50:17 +0300 Subject: [PATCH 28/31] Unify & place thenable option value warnings better --- lib/command.js | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/lib/command.js b/lib/command.js index bb3c457c3..50ccdb62e 100644 --- a/lib/command.js +++ b/lib/command.js @@ -517,20 +517,13 @@ Expecting one of '${allowedValues.join("', '")}'`); const name = option.attributeName(); // store default value - const setDefaultOptionValue = (value) => { - this.setOptionValueWithSource(name, value, 'default'); - if (!PRODUCTION && !this._asyncParsing && isThenable(value)) { - console.warn(`.parse() is incompatible with thenable default option values. -Use .parseAsync() instead`); - } - }; if (option.defaultValue !== undefined || !option.negate) { - setDefaultOptionValue(option.defaultValue); + this.setOptionValueWithSource(name, option.defaultValue, 'default'); } else { // --no-foo is special and defaults foo to true, unless a --foo option is already defined const positiveLongFlag = option.long.replace(/^--no-/, '--'); if (!this._findOption(positiveLongFlag)) { - setDefaultOptionValue(true); + this.setOptionValueWithSource(name, true, 'default'); } } @@ -813,6 +806,14 @@ Use .parseAsync() instead`); */ setOptionValueWithSource(key, value, source) { + if (!PRODUCTION && !this._asyncParsing && isThenable(value)) { + console.warn(`.parse() is incompatible with ${ + ['cli', 'env'].includes(source) + ? 'async argParsers' + : 'thenable option values' + }. +Use .parseAsync() instead`); + } if (this._storeOptionsAsProperties) { this[key] = value; } else { @@ -1305,18 +1306,15 @@ Use .parseAsync() instead`); _parseArg(target, value, previous, handleError, handledResultCallback) { try { - const result = target.chained - ? this._chainOrCall(previous, (resolvedPrevious) => ( + if (target.chained) { + return this._chainOrCall(previous, (resolvedPrevious) => ( this._parseArgSubroutine( target, value, resolvedPrevious, handleError, handledResultCallback ) - )) - : this._parseArgSubroutine(...arguments) - if (!PRODUCTION && !this._asyncParsing && isThenable(result)) { - console.warn(`.parse() is incompatible with async argParsers. -Use .parseAsync() instead`); + )); } - return result; + + return this._parseArgSubroutine(...arguments); } catch (err) { handleError(err); } From 9d6cee0a3ecb694e90533fbd73d1e50f94d3b1f6 Mon Sep 17 00:00:00 2001 From: aweebit <36817090+aweebit@users.noreply.github.com> Date: Sun, 30 Jul 2023 05:56:26 +0300 Subject: [PATCH 29/31] Fix linting error --- lib/command.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/command.js b/lib/command.js index 50ccdb62e..37c74babb 100644 --- a/lib/command.js +++ b/lib/command.js @@ -807,11 +807,11 @@ Expecting one of '${allowedValues.join("', '")}'`); setOptionValueWithSource(key, value, source) { if (!PRODUCTION && !this._asyncParsing && isThenable(value)) { - console.warn(`.parse() is incompatible with ${ - ['cli', 'env'].includes(source) - ? 'async argParsers' - : 'thenable option values' - }. + console.warn(`.parse() is incompatible with ${ + ['cli', 'env'].includes(source) + ? 'async argParsers' + : 'thenable option values' + }. Use .parseAsync() instead`); } if (this._storeOptionsAsProperties) { From 2d211121e3481f7783fa1d4ac7de04703f73b21d Mon Sep 17 00:00:00 2001 From: aweebit <36817090+aweebit@users.noreply.github.com> Date: Sun, 30 Jul 2023 08:27:23 +0300 Subject: [PATCH 30/31] Add missing thenable argument value warnings --- lib/command.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/command.js b/lib/command.js index 37c74babb..4a1abb3f6 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1192,6 +1192,12 @@ Use .parseAsync() instead`); value = myParseArg(declaredArg, value, declaredArg.defaultValue); } } + if (!PRODUCTION && !this._asyncParsing && isThenable(value)) { + console.warn(`.parse() is incompatible with ${ + declaredArg.parseArg ? 'async argParsers' : 'thenable argument values' + }. +Use .parseAsync() instead`); + } processedArgs[index] = value; }); this.processedArgs = processedArgs; From cb8c30b8937cb6112c1f2ded197a6c96e30e6015 Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Fri, 11 Aug 2023 20:46:07 +0200 Subject: [PATCH 31/31] Remove NODE_ENV check Motivation: https://github.com/tj/commander.js/pull/1955 --- lib/command.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/command.js b/lib/command.js index 4a1abb3f6..2c837851f 100644 --- a/lib/command.js +++ b/lib/command.js @@ -12,8 +12,6 @@ const { suggestSimilar } = require('./suggestSimilar'); // @ts-check -const PRODUCTION = process.env.NODE_ENV === 'production'; - class Command extends EventEmitter { /** * Initialize a new `Command`. @@ -806,7 +804,7 @@ Expecting one of '${allowedValues.join("', '")}'`); */ setOptionValueWithSource(key, value, source) { - if (!PRODUCTION && !this._asyncParsing && isThenable(value)) { + if (!this._asyncParsing && isThenable(value)) { console.warn(`.parse() is incompatible with ${ ['cli', 'env'].includes(source) ? 'async argParsers' @@ -1192,7 +1190,7 @@ Use .parseAsync() instead`); value = myParseArg(declaredArg, value, declaredArg.defaultValue); } } - if (!PRODUCTION && !this._asyncParsing && isThenable(value)) { + if (!this._asyncParsing && isThenable(value)) { console.warn(`.parse() is incompatible with ${ declaredArg.parseArg ? 'async argParsers' : 'thenable argument values' }.