Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add Command.awaitHook(), Argument.chainArgParserCalls() and Option.chainArgParserCalls() #1902

Closed
wants to merge 22 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
4061bff
Add Command.awaitHook() (#1900)
aweebit Jul 6, 2023
4a6e07c
Add support for variadic thenable chains (#1902)
aweebit Jul 6, 2023
4738e3c
Improve error handling and performance (#1902)
aweebit Jul 6, 2023
c0b7e74
Properly handle parseArg rejections (#1902)
aweebit Jul 7, 2023
200b31d
Improve rejection handling (#1902)
aweebit Jul 7, 2023
2114a72
Make parseArg() call chaining optional (#1902)
aweebit Jul 7, 2023
7d1038e
Only await chained arguments and options (#1902)
aweebit Jul 7, 2023
dbca267
Improve option lookup performance (#1902)
aweebit Jul 7, 2023
6f795f7
Fix asynchronous custom processing logic (#1902)
aweebit Jul 7, 2023
e632f84
Add chainArgParserCalls() tests (#1902)
aweebit Jul 7, 2023
b96474c
Add tests for repeated options (#1902)
aweebit Jul 7, 2023
6c44e2b
Improve thenable handling and use in tests (#1902)
aweebit Jul 8, 2023
ca48392
Get rid of linting errors
aweebit Jul 8, 2023
287cdc7
Add global option support for awaitHook() (#1902)
aweebit Jul 23, 2023
1830e14
Get rid of dangling promises (#1902)
aweebit Jul 23, 2023
a451ac5
parseAsync() with awaitHook() by default & postArguments hooks (#1902)
aweebit Jul 23, 2023
3a956b5
Make await hooks last by default (#1902)
aweebit Jul 23, 2023
96b9fc2
Add type tests (#1902)
aweebit Jul 23, 2023
62700a3
Avoid promise chaining when possible (#1902)
aweebit Jul 24, 2023
9a2a2b5
Add thenable function & name method better (#1902)
aweebit Jul 25, 2023
51d460c
Fix bug
aweebit Jul 25, 2023
94cb5ab
Recover old tests that now pass
aweebit Jul 25, 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
21 changes: 21 additions & 0 deletions lib/command.js
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,27 @@ Expecting one of '${allowedValues.join("', '")}'`);
return this;
}

/**
* Add hook to await argument and option values before calling action handlers for this command and its nested subcommands.
* Useful for asynchronous custom processing (parseArg) of arguments and option-arguments.
*/

awaitHook() {
this.hook('preAction', async function awaitHook(thisCommand, actionCommand) {
actionCommand.processedArgs = await Promise.all(
Copy link
Collaborator

Choose a reason for hiding this comment

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

processedArgs may include an element which is an array for a variadic argument. I am guessing this is not supported by Promise.all so might need more complex processing.

actionCommand.processedArgs
);

for (const [key, value] of Object.entries(actionCommand.opts())) {
actionCommand.setOptionValueWithSource(
key, await value, actionCommand._optionValueSources[key]
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. The option value may be an array of values for a variadic option.

  2. I think each call to await will delay execution until the next tick. I suggest only call for thenable values.

  3. I would slightly prefer not to call setOptionValueWithSource on non-promises, which also fits in with testing for thenable in above item.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is an existing test for thenable here:

if (promise && promise.then && typeof promise.then === 'function') {

Copy link
Contributor Author

@aweebit aweebit Jul 6, 2023

Choose a reason for hiding this comment

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

  1. Due to the "previous value" paradigm of custom variadic option processing, the only possible sources for such an array are default and implied values which are assumed to be preprocessed and should ideally be left untouched. If the array really needs to be awaited elementwise, one could simply use Promise.all(arr) as the default / implied value. The same applies to arguments.
  2. Solved by 4738e3c.
  3. Solved by 4738e3c.

);
}
});

return this;
}

/**
* Register callback to use as replacement for calling process.exit.
*
Expand Down
2 changes: 1 addition & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

128 changes: 128 additions & 0 deletions tests/command.awaitHook.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
const commander = require('../');

describe('awaitHook with arguments', () => {
test('when awaitHook and arguments with custom processing then .processedArgs and actioon arguments resolved from callback', async() => {
const resolvedValues = [3, 4];
const awaited = [
{ then: (fn) => fn(resolvedValues[0]) },
resolvedValues[1]
];
const mockCoercions = awaited.map(
value => jest.fn().mockImplementation(() => value)
);

let actionValues;
const program = new commander.Command();
program
.argument('<arg>', 'desc', mockCoercions[0])
.argument('[arg]', 'desc', mockCoercions[1])
.awaitHook()
.action((...args) => {
actionValues = args.slice(0, resolvedValues.length);
});

const result = program.parseAsync(['1', '2'], { from: 'user' });
expect(program.processedArgs).toEqual(awaited);
await result;
expect(program.processedArgs).toEqual(resolvedValues);
expect(actionValues).toEqual(resolvedValues);
});

test('when awaitHook and arguments not specified with default values then .processedArgs and actioon arguments resolved from default values', async() => {
const resolvedValues = [1, 2];
const awaited = [
{ then: (fn) => fn(resolvedValues[0]) },
resolvedValues[1]
];

let actionValues;
const program = new commander.Command();
program
.argument('[arg]', 'desc', awaited[0])
.argument('[arg]', 'desc', awaited[1])
.awaitHook()
.action((...args) => {
actionValues = args.slice(0, resolvedValues.length);
});

const result = program.parseAsync([], { from: 'user' });
expect(program.processedArgs).toEqual(awaited);
await result;
expect(program.processedArgs).toEqual(resolvedValues);
expect(actionValues).toEqual(resolvedValues);
});
});

describe('awaitHook with options', () => {
test('when awaitHook and options with custom processing then .opts() resolved from callback', async() => {
const resolvedValues = { a: 3, b: 4 };
const awaited = {
a: { then: (fn) => fn(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 <arg>', 'desc', mockCoercions.a)
.option('-b [arg]', 'desc', mockCoercions.b)
.awaitHook()
.action(() => {});

const result = program.parseAsync(['-a', '1', '-b', '2'], { from: 'user' });
expect(program.opts()).toEqual(awaited);
await result;
expect(program.opts()).toEqual(resolvedValues);
expect(program.getOptionValueSource('a')).toEqual('cli');
expect(program.getOptionValueSource('b')).toEqual('cli');
});

test('when awaitHook and options not specified with default values then .opts() resolved from default values', async() => {
const resolvedValues = { a: 1, b: 2 };
const awaited = {
a: { then: (fn) => fn(resolvedValues.a) },
b: resolvedValues.b
};

const program = new commander.Command();
program
.option('-a <arg>', 'desc', awaited.a)
.option('-b [arg]', 'desc', awaited.b)
.awaitHook()
.action(() => {});

const result = program.parseAsync([], { from: 'user' });
expect(program.opts()).toEqual(awaited);
await result;
expect(program.opts()).toEqual(resolvedValues);
expect(program.getOptionValueSource('a')).toEqual('default');
expect(program.getOptionValueSource('b')).toEqual('default');
});

test('when awaitHook and implied option values then .opts() resolved from implied values', async() => {
const resolvedValues = { a: 1, b: 2 };
const awaited = {
a: { then: (fn) => fn(resolvedValues.a) },
b: resolvedValues.b
};

const option = new commander.Option('-c').implies(awaited);
const program = new commander.Command();
program
.option('-a <arg>')
.option('-b [arg]')
.addOption(option)
.awaitHook()
.action(() => {});

const result = program.parseAsync(['-c'], { from: 'user' });
expect(program.opts()).toEqual({ ...awaited, c: true });
await result;
expect(program.opts()).toEqual({ ...resolvedValues, c: true });
expect(program.getOptionValueSource('a')).toEqual('implied');
expect(program.getOptionValueSource('b')).toEqual('implied');
});
});
6 changes: 6 additions & 0 deletions tests/command.chain.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,12 @@ describe('Command methods that should return this for chaining', () => {
expect(result).toBe(program);
});

test('when call .awaitHook() then returns this', () => {
const program = new Command();
const result = program.awaitHook();
expect(result).toBe(program);
});

test('when call .setOptionValue() then returns this', () => {
const program = new Command();
const result = program.setOptionValue('foo', 'bar');
Expand Down
6 changes: 6 additions & 0 deletions typings/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,12 @@ export class Command {
*/
hook(event: HookEvent, listener: (thisCommand: Command, actionCommand: Command) => void | Promise<void>): this;

/**
* Add hook to await argument and option values before calling action handlers for this command and its nested subcommands.
* Useful for asynchronous custom processing (parseArg) of arguments and option-arguments.
*/
awaitHook(): this;

/**
* Register callback to use as replacement for calling process.exit.
*/
Expand Down
3 changes: 3 additions & 0 deletions typings/index.test-d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,9 @@ expectType<commander.Command>(program.hook('preSubcommand', (thisCommand, subcom
expectType<commander.Command>(subcommand);
}));

// awaitHook
expectType<commander.Command>(program.awaitHook());

// action
expectType<commander.Command>(program.action(() => {}));
expectType<commander.Command>(program.action(async() => {}));
Expand Down