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

Implement lazily created sub commands. #1276

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 35 additions & 16 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ class Command extends EventEmitter {
cmd._passCommandToAction = this._passCommandToAction;

cmd._executableFile = opts.executableFile || null; // Custom name for executable file, set missing to null to match constructor
cmd._actionHandler = opts.action || null; // A handler for sub commands.
this.commands.push(cmd);
cmd._parseExpectedArgs(args);
cmd.parent = this;
Expand Down Expand Up @@ -228,8 +229,8 @@ class Command extends EventEmitter {
// Fail fast and detect when adding rather than later when parsing.
function checkExplicitNames(commandArray) {
commandArray.forEach((cmd) => {
if (cmd._executableHandler && !cmd._executableFile) {
throw new Error(`Must specify executableFile for deeply nested executable: ${cmd.name()}`);
if (cmd._executableHandler && !cmd._executableFile && !cmd._actionHandler) {
throw new Error(`Must specify executableFile or action for deeply nested executable: ${cmd.name()}`);
}
checkExplicitNames(cmd.commands);
});
Expand Down Expand Up @@ -668,6 +669,11 @@ class Command extends EventEmitter {
*/

parse(argv, parseOptions) {
this._parseProgram(argv, parseOptions);
return this;
}

_parseProgram(argv, parseOptions) {
if (argv !== undefined && !Array.isArray(argv)) {
throw new Error('first parameter to parse must be array or undefined');
}
Expand Down Expand Up @@ -714,9 +720,7 @@ class Command extends EventEmitter {
this._name = this._name || (this._scriptPath && path.basename(this._scriptPath, path.extname(this._scriptPath)));

// Let's go!
this._parseCommand([], userArgs);

return this;
return this.parseCommand([], userArgs);
};

/**
Expand All @@ -736,13 +740,14 @@ class Command extends EventEmitter {
* @param {string[]} [argv]
* @param {Object} [parseOptions]
* @param {string} parseOptions.from - where the args are from: 'node', 'user', 'electron'
* @return {Promise}
* @return {Promise} promise `this`
* @api public
*/

parseAsync(argv, parseOptions) {
this.parse(argv, parseOptions);
return Promise.all(this._actionResults).then(() => this);
return this._parseProgram(argv, parseOptions).then(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of scattering Promise.resolve through the code in places that do not use Promises, could call Promise.resolve here instead to minimise the code churn.

return Promise.all(this._actionResults).then(() => this);
});
};

/**
Expand Down Expand Up @@ -850,45 +855,58 @@ class Command extends EventEmitter {

// Store the reference to the child process
this.runningCommand = proc;
return Promise.resolve();
};

/**
* @return {Promise}
* @api private
*/
_dispatchSubcommand(commandName, operands, unknown) {
const subCommand = this._findCommand(commandName);
if (!subCommand) this._helpAndError();

if (subCommand._executableHandler) {
this._executeSubCommand(subCommand, operands.concat(unknown));
} else {
subCommand._parseCommand(operands, unknown);
if (subCommand._actionHandler) {
const actionPromise = subCommand._actionHandler({ command: this, operands, unknown });
if (actionPromise instanceof Promise) {
return actionPromise;
}
return Promise.resolve(actionPromise);
Comment on lines +872 to +875
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would probably be more consistent with JavaScript conventions to check for "thenable" than for a Promise as such, but in fact can just drop the test entirely as Promise.resolve handles Promises too.

}
return this._executeSubCommand(subCommand, operands.concat(unknown));
}
return subCommand.parseCommand(operands, unknown);
};

/**
* Process arguments in context of this command.
*
* @api private
* @param {string[]} [operands]
* @param {string[]} [unknown]
* @return {Promise}
*/

_parseCommand(operands, unknown) {
parseCommand(operands = [], unknown = []) {
const parsed = this.parseOptions(unknown);
operands = operands.concat(parsed.operands);
unknown = parsed.unknown;
this.args = operands.concat(unknown);

this.emit('commandParsed:' + this.name(), this);

let commandPromise = Promise.resolve();
if (operands && this._findCommand(operands[0])) {
this._dispatchSubcommand(operands[0], operands.slice(1), unknown);
commandPromise = this._dispatchSubcommand(operands[0], operands.slice(1), unknown);
} else if (this._lazyHasImplicitHelpCommand() && operands[0] === this._helpCommandName) {
if (operands.length === 1) {
this.help();
} else {
this._dispatchSubcommand(operands[1], [], [this._helpLongFlag]);
commandPromise = this._dispatchSubcommand(operands[1], [], [this._helpLongFlag]);
}
} else if (this._defaultCommandName) {
outputHelpIfRequested(this, unknown); // Run the help for default command from parent rather than passing to default command
this._dispatchSubcommand(this._defaultCommandName, operands, unknown);
commandPromise = 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
Expand Down Expand Up @@ -928,6 +946,7 @@ class Command extends EventEmitter {
// fall through for caller to handle after calling .parse()
}
}
return commandPromise;
};

/**
Expand Down
108 changes: 108 additions & 0 deletions tests/command.subCommandPromise.action.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
const commander = require('../');

// Test some behaviours of .action not covered in more specific tests.

describe('with action', () => {
test('when .action called then builder action is executed', () => {
const actionMock = jest.fn();
const program = new commander.Command();
program.command('run', 'run description', {
action: actionOptions => {
return actionOptions
.command
.command('run')
.action(actionMock)
.parseCommand(actionOptions.operands, actionOptions.unknown);
}
});
program.parse(['node', 'test', 'run']);
expect(actionMock).toHaveBeenCalled();
});

test('when .action called action is executed', () => {
const actionMock = jest.fn();
const program = new commander.Command();
program.command('run', 'run description', {
action: actionOptions => {
return Promise.resolve(actionOptions
.command
.command('run')
.action(actionMock)
.parseCommand(actionOptions.operands, actionOptions.unknown));
}
});
return program.parseAsync(['node', 'test', 'run']).then(() => {
expect(actionMock).toHaveBeenCalled();
});
});

test('arguments is resolved and passed to action', () => {
const actionSpy = jest.fn();
return new commander.Command()
.command('run <action>', 'run description', {
action: actionOptions => {
return Promise.resolve(actionOptions
.command
.command('run <arg>')
.action((arg) => {
expect(arg).toEqual('foo');
actionSpy();
})
.parseCommand(actionOptions.operands, actionOptions.unknown));
}
})
.parseAsync(['node', 'test', 'run', 'foo']).then(() => {
expect(actionSpy).toHaveBeenCalled();
});
});

test('options is resolved and passed to action', () => {
const actionSpy = jest.fn();
return new commander.Command()
.command('run <action>', 'run description', {
action: actionOptions => {
return Promise.resolve(actionOptions
.command
.command('run')
.requiredOption('--test-option <val>', 'test value')
.action(command => {
expect(command.testOption).toEqual('bar');
actionSpy();
})
.parseCommand(actionOptions.operands, actionOptions.unknown)
);
}
})
.parseAsync(['node', 'test', 'run', '--test-option', 'bar']).then(() => {
expect(actionSpy).toHaveBeenCalled();
});
});

test('recursive sub command action', () => {
const infoSpy = jest.fn();
const runSpy = jest.fn();
const runCommandBuilder = actionOptions => {
return actionOptions.command.command('run <action>', 'run description', {
action: _actionOptions => {
if (_actionOptions.operands && _actionOptions.operands[0] === 'run') {
runSpy();
return runCommandBuilder(_actionOptions)
.parseCommand(_actionOptions.operands, _actionOptions.unknown);
}
return Promise.resolve(actionOptions
.command
.command('info')
.action(infoSpy)
.parseCommand(actionOptions.operands, actionOptions.unknown)
);
}
});
};
return runCommandBuilder({ command: new commander.Command() })
.parseAsync(['node', 'test', 'run', 'run', 'run', 'run', 'info']).then(() => {
expect(infoSpy).toHaveBeenCalled();
expect(runSpy).toHaveBeenCalledTimes(3);
})
;
});
});