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

fix: --verbose option not working for run-ios #1571

Closed
wants to merge 1 commit into from
Closed

fix: --verbose option not working for run-ios #1571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
90 changes: 90 additions & 0 deletions packages/cli/src/__tests__/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
import commander from 'commander';
import {attachCommand} from '../index';

jest.mock('@react-native-community/cli-config', () => ({
commands: [],
}));

describe('attachCommand', () => {
it('should not pass verbose flag to sub-commands with their own verbose flag, given flag not set', () => {
const commandFunction = jest.fn();
const command = {
name: 'command-with-verbose-option',
func: commandFunction,
options: [
{name: '--verbose'},
{name: '--other-option-set'},
{name: '--other-option-not-set'},
],
};
attachCommand(command);

const argv = [
'node',
'index.ts',
'command-with-verbose-option',
'--other-option-set',
];
commander.parse(argv);

expect(commandFunction).toBeCalledWith(expect.any(Object), undefined, {
verbose: undefined,
otherOptionSet: true,
otherOptionNotSet: undefined,
});
});

it('should pass verbose flag to sub-commands with their own verbose flag, given flag set', () => {
const commandFunction = jest.fn();
const command = {
name: 'command-with-verbose-option',
func: commandFunction,
options: [
{name: '--verbose'},
{name: '--other-option-set'},
{name: '--other-option-not-set'},
],
};
attachCommand(command);

const argv = [
'node',
'index.ts',
'command-with-verbose-option',
'--verbose',
'--other-option-set',
];
commander.parse(argv);

expect(commandFunction).toBeCalledWith(expect.any(Object), undefined, {
verbose: true,
otherOptionSet: true,
otherOptionNotSet: undefined,
});
});

it('should not pass verbose flag to sub-commands without their own verbose flag, given flag set', () => {
const commandFunction = jest.fn();
const command = {
name: 'command-without-verbose-option',
func: commandFunction,
options: [{name: '--other-option-set'}, {name: '--other-option-not-set'}],
};
attachCommand(command);

const argv = [
'node',
'index.ts',
'command-without-verbose-option',
'--verbose',
'--other-option-set',
];
commander.parse(argv);

expect(commandFunction).toBeCalledWith(expect.any(Object), undefined, {
otherOptionSet: true,
otherOptionNotSet: undefined,
});
expect(commandFunction.mock.calls[0][2]).not.toHaveProperty('verbose');
});
});
9 changes: 8 additions & 1 deletion packages/cli/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,13 @@ function attachCommand<IsDetached extends boolean>(
const passedOptions = this.opts();
const argv = Array.from(args).slice(0, -1);

// Copy parent commander verbose value to the sub-command if the sub-command also has a verbose option
// This is a workaround that can potentially be removed if commander is upgraded to v7+
Copy link
Member

Choose a reason for hiding this comment

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

commander is now at v9. Is this fix relevant then? If so, please update the comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I wasn't aware commander had been upgraded, that's good! 😄 That does potentially change things a bit.

How that affects the direction of the PR will depend a bit on how the core maintainers want things to work overall in the CLI. Some of the routes now open to us would affect option parsing across all commands.

Firstly, I believe the current workarounds would still work in the new version of commander but I've not tested them yet.

As for alternatives, there are multiple option parsing methods in the newer version of commander which are potentially relevant (full docs w/ examples);

.enablePositionalOptions()

With positional options, the -b is a program option in the first line and a subcommand option in the second line:

program -b subcommand
program subcommand -b

So, if this was enabled react-native --verbose run-ios would be different to react-native run-ios --verbose. I guess you could also specify react-native --verbose run-ios --verbose to up the verbosity at both levels?

.passThroughOptions()

By default options are recognised before and after command-arguments. To only process options that come before the command-arguments, use .passThroughOptions(). This lets you pass the arguments and following options through to another program without needing to use -- to end the option processing. To use pass through options in a subcommand, the program needs to enable positional options.

With pass through options, the --port=80 is a program option in the first line and passed through as a command-argument in the second line:

program --port=80 arg
program arg --port=80

By default the option processing shows an error for an unknown option. To have an unknown option treated as an ordinary command-argument and continue looking for options, use .allowUnknownOption(). This lets you mix known and unknown options.

By default the argument processing does not display an error for more command-arguments than expected. To display an error for excess arguments, use .allowExcessArguments(false).

This one might also be relevant but I'm not sure how it would behave in the CLI until I have a chance to play with it (due to the dynamic attachCommand setup).

Also, in this specific situation with --verbose I don't yet know if the recognised options (e.g. top level --verbose) still get hidden from the pass-through.

Is there a preferred way the core maintainers would like the options to behave in the CLI? If so I can look at changing the PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So I think we can use enablePositionalOptions(), this fixes this issue and we don't need to make workarounds.

// See https://github.com/react-native-community/cli/issues/1391#issuecomment-813078097 for more info
if (passedOptions.hasOwnProperty('verbose')) {
passedOptions.verbose = commander.verbose;
}

try {
if (isDetachedCommand(command)) {
await command.func(argv, passedOptions);
Expand Down Expand Up @@ -254,4 +261,4 @@ async function setupAndRun() {

const bin = require.resolve('./bin');

export {run, bin, loadConfig};
export {run, bin, loadConfig, attachCommand};