From 7171bfaf193e2092a6e12237dfdc1b32915ddb6e Mon Sep 17 00:00:00 2001 From: John Gee <john@ruru.gen.nz> Date: Mon, 24 Jul 2023 22:15:09 +1200 Subject: [PATCH 01/12] Add _settleOptionPromises and call before preAction hook --- lib/command.js | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/lib/command.js b/lib/command.js index 590a271dd..920600fe7 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()); } @@ -1249,6 +1248,26 @@ Expecting one of '${allowedValues.join("', '")}'`); return result; } + /** + * @api private + */ + _settleOptionPromises() { + const promises = []; + + Object.entries(this.opts()).forEach(([key, value]) => { + if (thenable(value)) { + promises.push(value); + value.then((settledValue) => { + this.setOptionValueWithSource(key, settledValue, this.getOptionValueSource(key)); + }); + } + }); + + if (promises.length > 0) { + return Promise.all(promises); + } + } + /** * Process arguments in context of this command. * Returns action result, in case it is a promise. @@ -1296,6 +1315,7 @@ Expecting one of '${allowedValues.join("', '")}'`); this._processArguments(); let actionResult; + actionResult = this._chainOrCall(actionResult, () => this._settleOptionPromises()); actionResult = this._chainOrCallHooks(actionResult, 'preAction'); actionResult = this._chainOrCall(actionResult, () => this._actionHandler(this.processedArgs)); if (this.parent) { @@ -2193,4 +2213,14 @@ function getCommandAndParents(startCommand) { return result; } +/** + * @param {Object} obj + * @returns {boolean} + * @api private + */ + +function thenable(obj) { + return (obj && obj.then && typeof obj.then === 'function'); +} + exports.Command = Command; From d3eb58ee2f0f76d65a0ad077dbb988e2d6803b1f Mon Sep 17 00:00:00 2001 From: John Gee <john@ruru.gen.nz> Date: Mon, 24 Jul 2023 22:38:10 +1200 Subject: [PATCH 02/12] Add _settleOptionPromises in _dispatchSubcommand --- lib/command.js | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/lib/command.js b/lib/command.js index 920600fe7..acdd1bc66 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1072,16 +1072,16 @@ Expecting one of '${allowedValues.join("', '")}'`); const subCommand = this._findCommand(commandName); if (!subCommand) this.help({ error: true }); - let hookResult; - hookResult = this._chainOrCallSubCommandHook(hookResult, subCommand, 'preSubcommand'); - hookResult = this._chainOrCall(hookResult, () => { + let chain = this._settleOptionPromises(); + chain = this._chainOrCallSubCommandHook(chain, subCommand, 'preSubcommand'); + chain = this._chainOrCall(chain, () => { if (subCommand._executableHandler) { this._executeSubCommand(subCommand, operands.concat(unknown)); } else { return subCommand._parseCommand(operands, unknown); } }); - return hookResult; + return chain; } /** @@ -1314,17 +1314,16 @@ Expecting one of '${allowedValues.join("', '")}'`); checkForUnknownOptions(); this._processArguments(); - let actionResult; - actionResult = this._chainOrCall(actionResult, () => this._settleOptionPromises()); - actionResult = this._chainOrCallHooks(actionResult, 'preAction'); - actionResult = this._chainOrCall(actionResult, () => this._actionHandler(this.processedArgs)); + let chain = this._settleOptionPromises(); + chain = this._chainOrCallHooks(chain, 'preAction'); + chain = this._chainOrCall(chain, () => this._actionHandler(this.processedArgs)); if (this.parent) { - actionResult = this._chainOrCall(actionResult, () => { + chain = this._chainOrCall(chain, () => { this.parent.emit(commandEvent, operands, unknown); // legacy }); } - actionResult = this._chainOrCallHooks(actionResult, 'postAction'); - return actionResult; + chain = this._chainOrCallHooks(chain, 'postAction'); + return chain; } if (this.parent && this.parent.listenerCount(commandEvent)) { checkForUnknownOptions(); From 80f475ee9dff7d90907b4005ad742593e89a4ce4 Mon Sep 17 00:00:00 2001 From: John Gee <john@ruru.gen.nz> Date: Mon, 24 Jul 2023 23:01:17 +1200 Subject: [PATCH 03/12] Call _settleOptionPromises when no action handler --- lib/command.js | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/lib/command.js b/lib/command.js index acdd1bc66..4cc7a6d5b 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1308,13 +1308,16 @@ Expecting one of '${allowedValues.join("', '")}'`); this.unknownOption(parsed.unknown[0]); } }; + const settleAndProcessArguments = () => { + const chain = this._settleOptionPromises(); + return this._chainOrCall(chain, () => this._processArguments()); + }; + let chain; const commandEvent = `command:${this.name()}`; if (this._actionHandler) { checkForUnknownOptions(); - this._processArguments(); - - let chain = this._settleOptionPromises(); + chain = settleAndProcessArguments(); chain = this._chainOrCallHooks(chain, 'preAction'); chain = this._chainOrCall(chain, () => this._actionHandler(this.processedArgs)); if (this.parent) { @@ -1323,12 +1326,10 @@ Expecting one of '${allowedValues.join("', '")}'`); }); } chain = this._chainOrCallHooks(chain, 'postAction'); - return chain; - } - if (this.parent && this.parent.listenerCount(commandEvent)) { + } else if (this.parent && this.parent.listenerCount(commandEvent)) { checkForUnknownOptions(); - this._processArguments(); - this.parent.emit(commandEvent, operands, unknown); // legacy + chain = settleAndProcessArguments(); + 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); @@ -1340,7 +1341,7 @@ Expecting one of '${allowedValues.join("', '")}'`); this.unknownCommand(); } else { checkForUnknownOptions(); - this._processArguments(); + chain = settleAndProcessArguments(); } } else if (this.commands.length) { checkForUnknownOptions(); @@ -1348,9 +1349,10 @@ Expecting one of '${allowedValues.join("', '")}'`); this.help({ error: true }); } else { checkForUnknownOptions(); - this._processArguments(); + chain = settleAndProcessArguments(); // fall through for caller to handle after calling .parse() } + return chain; } /** From 6b7cee7d3ec4c51d269aa1b9f7997b98140d126a Mon Sep 17 00:00:00 2001 From: John Gee <john@ruru.gen.nz> Date: Tue, 25 Jul 2023 14:51:21 +1200 Subject: [PATCH 04/12] Add custom error handling for async Option.parseArgs --- lib/command.js | 48 +++++++++++++++++++++++++++++------------------- 1 file changed, 29 insertions(+), 19 deletions(-) diff --git a/lib/command.js b/lib/command.js index 4cc7a6d5b..29e35a366 100644 --- a/lib/command.js +++ b/lib/command.js @@ -535,15 +535,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._parseArgWithMessage(invalidValueMessage, () => option.parseArg(val, oldValue)); } else if (val !== null && option.variadic) { val = option._concatValue(val, oldValue); } @@ -1134,18 +1126,10 @@ Expecting one of '${allowedValues.join("', '")}'`); _processArguments() { const myParseArg = (argument, value, previous) => { - // 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; - } + const errorMessage = `error: command-argument value '${value}' is invalid for argument '${argument.name()}'.`; + parsedValue = this._parseArgWithMessage(errorMessage, () => argument.parseArg(value, previous)); } return parsedValue; }; @@ -1251,6 +1235,32 @@ Expecting one of '${allowedValues.join("', '")}'`); /** * @api private */ + + _parseArgWithMessage(invalidArgumentMessage, parseArgFn) { + const refineError = (err) => { + if (err.code === 'commander.invalidArgument') { + const message = `${invalidArgumentMessage} ${err.message}`; + this.error(message, { exitCode: err.exitCode, code: err.code }); + } + throw err; + }; + + let result; + try { + result = parseArgFn(); + if (thenable(result)) { + result.catch(refineError); + } + } catch (err) { + refineError(err); + } + return result; + } + + /** + * @api private + */ + _settleOptionPromises() { const promises = []; From e03456b40f5661c4630868828740d081fe8a8740 Mon Sep 17 00:00:00 2001 From: John Gee <john@ruru.gen.nz> Date: Tue, 25 Jul 2023 15:04:16 +1200 Subject: [PATCH 05/12] Add comment --- lib/command.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/command.js b/lib/command.js index 29e35a366..3780f7f97 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1267,6 +1267,7 @@ Expecting one of '${allowedValues.join("', '")}'`); Object.entries(this.opts()).forEach(([key, value]) => { if (thenable(value)) { promises.push(value); + // Resave value after promise resolves. value.then((settledValue) => { this.setOptionValueWithSource(key, settledValue, this.getOptionValueSource(key)); }); From 750b513c2e9768c62ccbe2750e5f186eaccc30b5 Mon Sep 17 00:00:00 2001 From: John Gee <john@ruru.gen.nz> Date: Tue, 25 Jul 2023 19:56:01 +1200 Subject: [PATCH 06/12] Add _settleArgumentPromises --- lib/command.js | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/lib/command.js b/lib/command.js index 3780f7f97..da3884b93 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1160,6 +1160,7 @@ Expecting one of '${allowedValues.join("', '")}'`); processedArgs[index] = value; }); this.processedArgs = processedArgs; + return this._settleArgumentPromises(); } /** @@ -1264,6 +1265,7 @@ Expecting one of '${allowedValues.join("', '")}'`); _settleOptionPromises() { const promises = []; + // Look through the options for promises from async parseArgs (or other sources). Object.entries(this.opts()).forEach(([key, value]) => { if (thenable(value)) { promises.push(value); @@ -1279,6 +1281,29 @@ Expecting one of '${allowedValues.join("', '")}'`); } } + /** + * @api private + */ + + _settleArgumentPromises() { + const promises = []; + + // Variadic array is either ordinary, or a promise for an array. + this.processedArgs.forEach((arg, index) => { + if (thenable(arg)) { + promises.push(arg); + // Resave value after promise resolves. + arg.then((settledArg) => { + this.processedArgs[index] = settledArg; + }); + } + }); + + if (promises.length > 0) { + return Promise.all(promises); + } + } + /** * Process arguments in context of this command. * Returns action result, in case it is a promise. From 30dcb28329dbdfe9b6129772683d9ef7e0ed23b2 Mon Sep 17 00:00:00 2001 From: John Gee <john@ruru.gen.nz> Date: Wed, 26 Jul 2023 15:34:17 +1200 Subject: [PATCH 07/12] Add support for resolving previous value before calling parseArgs --- lib/command.js | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/lib/command.js b/lib/command.js index da3884b93..8ac1b5beb 100644 --- a/lib/command.js +++ b/lib/command.js @@ -535,7 +535,7 @@ Expecting one of '${allowedValues.join("', '")}'`); // custom processing const oldValue = this.getOptionValue(name); if (val !== null && option.parseArg) { - val = this._parseArgWithMessage(invalidValueMessage, () => option.parseArg(val, oldValue)); + val = this._parseArg(option, val, oldValue, invalidValueMessage); } else if (val !== null && option.variadic) { val = option._concatValue(val, oldValue); } @@ -1129,7 +1129,7 @@ Expecting one of '${allowedValues.join("', '")}'`); let parsedValue = value; if (value !== null && argument.parseArg) { const errorMessage = `error: command-argument value '${value}' is invalid for argument '${argument.name()}'.`; - parsedValue = this._parseArgWithMessage(errorMessage, () => argument.parseArg(value, previous)); + parsedValue = this._parseArg(argument, value, previous, errorMessage); } return parsedValue; }; @@ -1234,10 +1234,18 @@ Expecting one of '${allowedValues.join("', '")}'`); } /** + * Call parseArgs with extra handling: + * - custom error message if parseArgs throws 'commander.invalidArgument' + * - if the previous value is a promise, chain the call to resolve the promise before parsing + * + * @param {Option | Argument} target + * @param {string} value + * @param {Promise<any> | any} previous + * @param {string} invalidArgumentMessage * @api private */ - _parseArgWithMessage(invalidArgumentMessage, parseArgFn) { + _parseArg(target, value, previous, invalidArgumentMessage) { const refineError = (err) => { if (err.code === 'commander.invalidArgument') { const message = `${invalidArgumentMessage} ${err.message}`; @@ -1247,14 +1255,17 @@ Expecting one of '${allowedValues.join("', '")}'`); }; let result; - try { - result = parseArgFn(); - if (thenable(result)) { - result.catch(refineError); + if (thenable(previous)) { + result = previous.then(resolvedPrevious => target.parseArg(value, resolvedPrevious)); + } else { + try { + result = target.parseArg(value, previous); + } catch (err) { + refineError(err); } - } catch (err) { - refineError(err); } + + if (thenable(result)) result.catch(refineError); return result; } From d243325bfca2c358b5541e65752b8bf4774c1209 Mon Sep 17 00:00:00 2001 From: John Gee <john@ruru.gen.nz> Date: Thu, 27 Jul 2023 14:00:48 +1200 Subject: [PATCH 08/12] Return updated promise after adding catch, and add catch using then() --- lib/command.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/command.js b/lib/command.js index 8ac1b5beb..03c232d5d 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1265,7 +1265,7 @@ Expecting one of '${allowedValues.join("', '")}'`); } } - if (thenable(result)) result.catch(refineError); + if (thenable(result)) result = result.then(null, refineError); // Using then() to do a catch since we only tested for then()). return result; } From 350450489750f4a5487d5a0a8079b5f9f31500c3 Mon Sep 17 00:00:00 2001 From: John Gee <john@ruru.gen.nz> Date: Thu, 27 Jul 2023 14:52:23 +1200 Subject: [PATCH 09/12] Fix promise collection --- lib/command.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/command.js b/lib/command.js index 03c232d5d..df1c0daca 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1279,11 +1279,11 @@ Expecting one of '${allowedValues.join("', '")}'`); // Look through the options for promises from async parseArgs (or other sources). Object.entries(this.opts()).forEach(([key, value]) => { if (thenable(value)) { - promises.push(value); // Resave value after promise resolves. - value.then((settledValue) => { + const promise = value.then((settledValue) => { this.setOptionValueWithSource(key, settledValue, this.getOptionValueSource(key)); }); + promises.push(promise); } }); @@ -1302,11 +1302,11 @@ Expecting one of '${allowedValues.join("', '")}'`); // Variadic array is either ordinary, or a promise for an array. this.processedArgs.forEach((arg, index) => { if (thenable(arg)) { - promises.push(arg); // Resave value after promise resolves. - arg.then((settledArg) => { + const promise = arg.then((settledArg) => { this.processedArgs[index] = settledArg; }); + promises.push(promise); } }); From a3c4cd32512caa3df80156dec99a2abe23690ade Mon Sep 17 00:00:00 2001 From: John Gee <john@ruru.gen.nz> Date: Thu, 27 Jul 2023 16:00:13 +1200 Subject: [PATCH 10/12] Add local catch for promise --- lib/command.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/command.js b/lib/command.js index df1c0daca..7b9eef37b 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1256,16 +1256,19 @@ Expecting one of '${allowedValues.join("', '")}'`); let result; if (thenable(previous)) { - result = previous.then(resolvedPrevious => target.parseArg(value, resolvedPrevious)); + result = previous.then(resolvedPrevious => { + let innerResult = target.parseArg(value, resolvedPrevious); + if (thenable(innerResult)) innerResult = innerResult.then(null, refineError); // .catch + return innerResult; + }); } else { try { result = target.parseArg(value, previous); + if (thenable(result)) result = result.then(null, refineError); // .catch } catch (err) { refineError(err); } } - - if (thenable(result)) result = result.then(null, refineError); // Using then() to do a catch since we only tested for then()). return result; } From 6fd35d2f530fc1ca706b8579f480c57ed388d76d Mon Sep 17 00:00:00 2001 From: John Gee <john@ruru.gen.nz> Date: Thu, 27 Jul 2023 17:17:17 +1200 Subject: [PATCH 11/12] Replace forEach --- lib/command.js | 34 +++++++++++++--------------------- 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/lib/command.js b/lib/command.js index 7b9eef37b..eccd1b006 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1277,18 +1277,14 @@ Expecting one of '${allowedValues.join("', '")}'`); */ _settleOptionPromises() { - const promises = []; - // Look through the options for promises from async parseArgs (or other sources). - Object.entries(this.opts()).forEach(([key, value]) => { - if (thenable(value)) { - // Resave value after promise resolves. - const promise = value.then((settledValue) => { - this.setOptionValueWithSource(key, settledValue, this.getOptionValueSource(key)); + const promises = Object.entries(this.opts()) + .filter(([key, maybePromise]) => thenable(maybePromise)) + .map(([key, promise]) => { + return promise.then(value => { + this.setOptionValueWithSource(key, value, this.getOptionValueSource(key)); }); - promises.push(promise); - } - }); + }); if (promises.length > 0) { return Promise.all(promises); @@ -1300,18 +1296,14 @@ Expecting one of '${allowedValues.join("', '")}'`); */ _settleArgumentPromises() { - const promises = []; - - // Variadic array is either ordinary, or a promise for an array. - this.processedArgs.forEach((arg, index) => { - if (thenable(arg)) { - // Resave value after promise resolves. - const promise = arg.then((settledArg) => { - this.processedArgs[index] = settledArg; + // Look through the arguments for promises from async parseArgs (or other sources). + const promises = this.processedArgs + .filter(thenable) + .map((promise, index) => { + return promise.then(value => { + this.processedArgs[index] = value; }); - promises.push(promise); - } - }); + }); if (promises.length > 0) { return Promise.all(promises); From c06d412dc0cdc91359eca2a95c8fe8230cd95eef Mon Sep 17 00:00:00 2001 From: John Gee <john@ruru.gen.nz> Date: Fri, 28 Jul 2023 16:01:05 +1200 Subject: [PATCH 12/12] Return boolean from thenable() --- lib/command.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/command.js b/lib/command.js index eccd1b006..2a4d617f9 100644 --- a/lib/command.js +++ b/lib/command.js @@ -2263,7 +2263,7 @@ function getCommandAndParents(startCommand) { */ function thenable(obj) { - return (obj && obj.then && typeof obj.then === 'function'); + return !!(obj && obj.then && typeof obj.then === 'function'); } exports.Command = Command;