Skip to content

Commit f9c491f

Browse files
authored
Convert CLI option parsing from yargs-parser to commander
- Replace yargs-parser with commander package (v10, Node 14+ compatible) - Remove yargs-parser and @types/yargs-parser dependencies - Ban unknown options (commander rejects unrecognized options) - Remove --bool=false and --bool false support in favor of --no-bool - Duplicate non-array options now use last-write-wins instead of throwing - CamelCase input (--gitTags) no longer accepted; use hyphenated (--git-tags) - Update tests to match new behavior - All existing options, aliases, and commands preserved Agent-Logs-Url: https://github.com/microsoft/beachball/sessions/b10c3147-797f-4524-a56d-1f2537833d47
1 parent 2898ef1 commit f9c491f

5 files changed

Lines changed: 160 additions & 189 deletions

File tree

package.json

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,15 +53,15 @@
5353
},
5454
"dependencies": {
5555
"@vercel/detect-agent": "^1.2.1",
56+
"commander": "^10.0.1",
5657
"cosmiconfig": "^9.0.0",
5758
"execa": "^5.0.0",
5859
"minimatch": "^3.0.4",
5960
"p-graph": "^1.1.2",
6061
"p-limit": "^3.0.2",
6162
"prompts": "^2.4.2",
6263
"semver": "^7.0.0",
63-
"workspace-tools": "^0.41.0",
64-
"yargs-parser": "^21.0.0"
64+
"workspace-tools": "^0.41.0"
6565
},
6666
"devDependencies": {
6767
"@jest/globals": "^29.0.0",
@@ -70,7 +70,6 @@
7070
"@types/prompts": "^2.4.2",
7171
"@types/semver": "^7.3.13",
7272
"@types/tmp": "^0.2.3",
73-
"@types/yargs-parser": "^21.0.0",
7473
"@typescript-eslint/eslint-plugin": "^5.0.0",
7574
"@typescript-eslint/parser": "^5.0.0",
7675
"@typescript-eslint/utils": "^5.0.0",

src/__functional__/options/getCliOptions.test.ts

Lines changed: 22 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,7 @@ jest.mock('workspace-tools', () => {
1111

1212
//
1313
// These tests cover a mix of built-in parser behavior, provided options, and custom overrides.
14-
// It's worth having tests for relevant built-in behaviors in case we change parsers in the future
15-
// (likely to commander), to ensure there are no undocumented breaking changes from the beachball
14+
// The parser is commander, and these tests document the expected behavior from the beachball
1615
// "end user" perspective.
1716
//
1817
describe('getCliOptions', () => {
@@ -76,29 +75,19 @@ describe('getCliOptions', () => {
7675
expect(options).toEqual({ ...defaults, scope: ['a,b', 'c,d'] });
7776
});
7877

79-
it('throws if non-array option is specified multiple times', () => {
80-
expect(() => getCliOptionsTest(['--tag', 'foo', '--tag', 'baz'])).toThrow();
78+
it('uses last value if non-array option is specified multiple times', () => {
79+
const options = getCliOptionsTest(['--tag', 'foo', '--tag', 'baz']);
80+
expect(options).toEqual({ ...defaults, tag: 'baz' });
8181
});
8282

8383
it('parses negated boolean option', () => {
8484
const options = getCliOptionsTest(['--no-fetch']);
8585
expect(options).toEqual({ ...defaults, fetch: false });
8686
});
8787

88-
it('parses valid boolean option values', () => {
89-
const falseOptions = getCliOptionsTest(['--fetch=false', '--yes', 'false']);
90-
expect(falseOptions).toEqual({ ...defaults, fetch: false, yes: false });
91-
92-
const trueOptions = getCliOptionsTest(['--fetch=true', '--yes', 'true']);
93-
expect(trueOptions).toEqual({ ...defaults, fetch: true, yes: true });
94-
});
95-
96-
it('parses boolean flag with valid value', () => {
97-
const falseOptions = getCliOptionsTest(['-y', 'false']);
98-
expect(falseOptions).toEqual({ ...defaults, yes: false });
99-
100-
const trueOptions = getCliOptionsTest(['-y', 'true']);
101-
expect(trueOptions).toEqual({ ...defaults, yes: true });
88+
it('parses negated boolean options with --no-X syntax', () => {
89+
const options = getCliOptionsTest(['--no-fetch', '--no-yes']);
90+
expect(options).toEqual({ ...defaults, fetch: false, yes: false });
10291
});
10392

10493
it('throws on invalid numeric value', () => {
@@ -110,10 +99,10 @@ describe('getCliOptions', () => {
11099
expect(options).toEqual({ ...defaults, gitTags: true, dependentChangeType: 'patch' });
111100
});
112101

113-
it('supports camel case for options defined as hyphenated', () => {
102+
it('requires hyphenated form for multi-word options', () => {
114103
const options = getCliOptionsTest([
115-
'--gitTags',
116-
'--dependentChangeType',
104+
'--git-tags',
105+
'--dependent-change-type',
117106
'patch',
118107
'--disallowed-change-types',
119108
'major',
@@ -175,37 +164,28 @@ describe('getCliOptions', () => {
175164
expect(getDefaultRemoteBranch).toHaveBeenCalledWith({ branch: 'foo', verbose: undefined, cwd: projectRoot });
176165
});
177166

178-
it('preserves additional string options', () => {
179-
const options = getCliOptionsTest(['--foo', 'bar', '--baz=qux']);
180-
expect(options).toEqual({ ...defaults, foo: 'bar', baz: 'qux' });
167+
it('throws on unknown string options', () => {
168+
expect(() => getCliOptionsTest(['--foo', 'bar'])).toThrow();
181169
});
182170

183-
it('handles additional boolean flags as booleans', () => {
184-
const options = getCliOptionsTest(['--foo', '--no-bar']);
185-
expect(options).toEqual({ ...defaults, foo: true, bar: false });
171+
it('throws on unknown boolean flags', () => {
172+
expect(() => getCliOptionsTest(['--foo'])).toThrow();
186173
});
187174

188-
it('handles additional boolean text values as booleans', () => {
189-
const options = getCliOptionsTest(['--foo', 'true', '--bar=false']);
190-
expect(options).toEqual({ ...defaults, foo: true, bar: false });
175+
it('throws on unknown negated boolean flags', () => {
176+
expect(() => getCliOptionsTest(['--no-bar'])).toThrow();
191177
});
192178

193-
it('handles additional numeric values as numbers', () => {
194-
const options = getCliOptionsTest(['--foo', '1', '--bar=2']);
195-
expect(options).toEqual({ ...defaults, foo: 1, bar: 2 });
179+
it('throws on unknown option with value', () => {
180+
expect(() => getCliOptionsTest(['--foo', 'true'])).toThrow();
196181
});
197182

198-
it('handles additional option specified multiple times as array', () => {
199-
const options = getCliOptionsTest(['--foo', 'bar', '--foo', 'baz']);
200-
expect(options).toEqual({ ...defaults, foo: ['bar', 'baz'] });
183+
it('throws on unknown option specified multiple times', () => {
184+
expect(() => getCliOptionsTest(['--foo', 'bar', '--foo', 'baz'])).toThrow();
201185
});
202186

203-
// documenting current behavior (doesn't have to stay this way)
204-
it('for additional options, does not handle multiple values as part of array', () => {
205-
// in this case the trailing value "baz" would be treated as the command since it's the first
206-
// positional option
207-
const options = getCliOptionsTest(['--foo', 'bar', 'baz']);
208-
expect(options).toEqual({ ...defaults, foo: 'bar', command: 'baz' });
187+
it('throws on unknown option followed by positional', () => {
188+
expect(() => getCliOptionsTest(['--foo', 'bar', 'baz'])).toThrow();
209189
});
210190

211191
describe('config command', () => {

src/__tests__/packageManager/listPackageVersions.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -318,7 +318,7 @@ describe('list npm versions', () => {
318318
npmMock.setRegistryData({ foo: { 'dist-tags': { latest: '1.0.0', beta: '2.0.0-beta' } } });
319319
const { packages, options } = getOptionsAndPackages({
320320
packages: { foo: {} },
321-
extraArgv: ['--authType', 'password', '--token', 'pass'],
321+
extraArgv: ['--auth-type', 'password', '--token', 'pass'],
322322
});
323323
expect(options).toMatchObject({ authType: 'password', token: 'pass' });
324324

0 commit comments

Comments
 (0)