Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature/async parse args experiment #1913

Closed
wants to merge 12 commits into from
Closed

Feature/async parse args experiment #1913

Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
7171bfa
Add _settleOptionPromises and call before preAction hook
shadowspawn Jul 24, 2023
d3eb58e
Add _settleOptionPromises in _dispatchSubcommand
shadowspawn Jul 24, 2023
80f475e
Call _settleOptionPromises when no action handler
shadowspawn Jul 24, 2023
6b7cee7
Add custom error handling for async Option.parseArgs
shadowspawn Jul 25, 2023
e03456b
Add comment
shadowspawn Jul 25, 2023
750b513
Add _settleArgumentPromises
shadowspawn Jul 25, 2023
30dcb28
Add support for resolving previous value before calling parseArgs
shadowspawn Jul 26, 2023
d243325
Return updated promise after adding catch, and add catch using then()
shadowspawn Jul 27, 2023
3504504
Fix promise collection
shadowspawn Jul 27, 2023
a3c4cd3
Add local catch for promise
shadowspawn Jul 27, 2023
6fd35d2
Replace forEach
shadowspawn Jul 27, 2023
c06d412
Return boolean from thenable()
shadowspawn Jul 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
156 changes: 117 additions & 39 deletions lib/command.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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._parseArg(option, val, oldValue, invalidValueMessage);
} else if (val !== null && option.variadic) {
val = option._concatValue(val, oldValue);
}
Expand Down Expand Up @@ -1072,16 +1064,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;
}

/**
Expand Down Expand Up @@ -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._parseArg(argument, value, previous, errorMessage);
}
return parsedValue;
};
Expand Down Expand Up @@ -1176,6 +1160,7 @@ Expecting one of '${allowedValues.join("', '")}'`);
processedArgs[index] = value;
});
this.processedArgs = processedArgs;
return this._settleArgumentPromises();
}

/**
Expand All @@ -1188,8 +1173,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());
}
Expand Down Expand Up @@ -1249,6 +1233,88 @@ Expecting one of '${allowedValues.join("', '")}'`);
return result;
}

/**
* 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
*/

_parseArg(target, value, previous, invalidArgumentMessage) {
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;
if (thenable(previous)) {
result = previous.then(resolvedPrevious => target.parseArg(value, resolvedPrevious));
} else {
try {
result = target.parseArg(value, previous);
} catch (err) {
refineError(err);
}
}

if (thenable(result)) result.catch(refineError);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refineError() throws, so there will be an unhandled promise rejection. The result of the catch() call needs to be assigned to result to prevent it.

But if previous is already thenable, it is better to call catch() in the previous.then() callback (and return the result). Otherwise, catch() might be called on an already failed promise chain in which an error has already been caught and rethrown (because that is what refineError() does), so refineError() would be called again which does not make sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I think calling result.then(x => x, refineError) instead of result.catch(refineError) is more robust.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result of the catch() call needs to be assigned to result

Oops, yes, good catch.

Otherwise, catch() might be called on an already failed promise chain in which an error has already been caught and rethrown

I think this is correct pattern, but I'll try it before explaining my reasoning.

Also, I think calling result.then(x => x, refineError) instead of result.catch(refineError) is more robust.

Because we only test for .then()? Yes, I have thought about that in past and then forgot. I want to just assume they are promise-like since the primary use-case is for async, but it isn't hard to only rely on then and cover a few more edge uses.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if previous is already thenable, it is better to call catch() in the previous.then() callback (and return the result). Otherwise, catch() might be called on an already failed promise chain in which an error has already been caught and rethrown (because that is what refineError() does), so refineError() would be called again which does not make sense.

Took me a while to understand this. I was expecting a long catch chain, but see it can be avoided. 👍

return result;
}

/**
* @api private
*/

_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);
// Resave value after promise resolves.
value.then((settledValue) => {
this.setOptionValueWithSource(key, settledValue, this.getOptionValueSource(key));
});
}
});

if (promises.length > 0) {
return Promise.all(promises);
}
}

/**
* @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.
Expand Down Expand Up @@ -1289,27 +1355,28 @@ 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 actionResult;
actionResult = this._chainOrCallHooks(actionResult, 'preAction');
actionResult = this._chainOrCall(actionResult, () => this._actionHandler(this.processedArgs));
chain = settleAndProcessArguments();
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;
}
if (this.parent && this.parent.listenerCount(commandEvent)) {
chain = this._chainOrCallHooks(chain, 'postAction');
} 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);
Expand All @@ -1321,17 +1388,18 @@ Expecting one of '${allowedValues.join("', '")}'`);
this.unknownCommand();
} else {
checkForUnknownOptions();
this._processArguments();
chain = settleAndProcessArguments();
}
} 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();
chain = settleAndProcessArguments();
// fall through for caller to handle after calling .parse()
}
return chain;
}

/**
Expand Down Expand Up @@ -2193,4 +2261,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;