fix(cli): refactor CLI argument parser (#3765)

This commit refactors the argument parser we use for our CLI module.
Doing so fixes an issue with certain flags supported by Jest which can
be passed multiple times. For instance, in the following example:

```
jest --coverage --reporters="default" --reporters="jest-junit"
```

all of the values for the `--reporters` flag ("default" and
"jest-junit") should be collected into an array of values, instead of
simply recording whichever value is farther to the right (Stencil's
behavior before this commit).

To support passing such arguments to the `stencil test` subcommand this
commit adds a new recursive-descent parser in `src/cli/parse-flags.ts`
to replace the somewhat ad-hoc approach that was there previously. It
parses the following grammar:

```
CLIArguments    → ""
                | CLITerm ( " " CLITerm )* ;
CLITerm         → EqualsArg
                | AliasEqualsArg
                | AliasArg
                | NegativeDashArg
                | NegativeArg
                | SimpleArg ;
EqualsArg       → "--" ArgName "=" CLIValue ;
AliasEqualsArg  → "-" AliasName "=" CLIValue ;
AliasArg        → "-" AliasName ( " " CLIValue )? ;
NegativeDashArg → "--no-" ArgName ;
NegativeArg     → "--no" ArgName ;
SimpleArg       → "--" ArgName ( " " CLIValue )? ;
ArgName         → /^[a-zA-Z-]+$/ ;
AliasName       → /^[a-z]{1}$/ ;
CLIValue        → '"' /^[a-zA-Z0-9]+$/ '"'
                | /^[a-zA-Z0-9]+$/ ;
```

The regexes are a little fuzzy, but this is sort of an informal
presentation, and additionally there are other constraints implemented
in the code which handles all of these different terms.

Refactoring this to use a proper parser (albeit a pretty simple one)
allows our implementation to much more clearly conform to this defined
grammar, and should hopefully both help us avoid other bugs in the
future and be easier to maintain.

See #3712 for more details on the issues with the `--reporters` flag in
particular.

Author: alicewriteswrongs

src/cli/config-flags.ts

@@ -3,7 +3,7 @@ import type { LogLevel, TaskCommand } from '@stencil/core/declarations';
 /**
  * All the Boolean options supported by the Stencil CLI
  */
-export const BOOLEAN_CLI_ARGS = [
+export const BOOLEAN_CLI_FLAGS = [
   'build',
   'cache',
   'checkVersion',
@@ -88,7 +88,7 @@ export const BOOLEAN_CLI_ARGS = [
 /**
  * All the Number options supported by the Stencil CLI
  */
-export const NUMBER_CLI_ARGS = [
+export const NUMBER_CLI_FLAGS = [
   'port',
   // JEST CLI ARGS
   'maxConcurrency',
@@ -98,7 +98,7 @@ export const NUMBER_CLI_ARGS = [
 /**
  * All the String options supported by the Stencil CLI
  */
-export const STRING_CLI_ARGS = [
+export const STRING_CLI_FLAGS = [
   'address',
   'config',
   'docsApi',
@@ -138,8 +138,9 @@ export const STRING_CLI_ARGS = [
   'testURL',
   'timers',
   'transform',
+] as const;
 
-  // ARRAY ARGS
+export const STRING_ARRAY_CLI_FLAGS = [
   'collectCoverageOnlyFrom',
   'coveragePathIgnorePatterns',
   'coverageReporters',
@@ -169,7 +170,7 @@ export const STRING_CLI_ARGS = [
  * `maxWorkers` is an argument which is used both by Stencil _and_ by Jest,
  * which means that we need to support parsing both string and number values.
  */
-export const STRING_NUMBER_CLI_ARGS = ['maxWorkers'] as const;
+export const STRING_NUMBER_CLI_FLAGS = ['maxWorkers'] as const;
 
 /**
  * All the LogLevel-type options supported by the Stencil CLI
@@ -178,7 +179,7 @@ export const STRING_NUMBER_CLI_ARGS = ['maxWorkers'] as const;
  * but this approach lets us make sure that we're handling all
  * our arguments in a type-safe way.
  */
-export const LOG_LEVEL_CLI_ARGS = ['logLevel'] as const;
+export const LOG_LEVEL_CLI_FLAGS = ['logLevel'] as const;
 
 /**
  * A type which gives the members of a `ReadonlyArray<string>` as
@@ -187,26 +188,39 @@ export const LOG_LEVEL_CLI_ARGS = ['logLevel'] as const;
  */
 type ArrayValuesAsUnion<T extends ReadonlyArray<string>> = T[number];
 
-export type BooleanCLIArg = ArrayValuesAsUnion<typeof BOOLEAN_CLI_ARGS>;
-export type StringCLIArg = ArrayValuesAsUnion<typeof STRING_CLI_ARGS>;
-export type NumberCLIArg = ArrayValuesAsUnion<typeof NUMBER_CLI_ARGS>;
-export type StringNumberCLIArg = ArrayValuesAsUnion<typeof STRING_NUMBER_CLI_ARGS>;
-export type LogCLIArg = ArrayValuesAsUnion<typeof LOG_LEVEL_CLI_ARGS>;
+export type BooleanCLIFlag = ArrayValuesAsUnion<typeof BOOLEAN_CLI_FLAGS>;
+export type StringCLIFlag = ArrayValuesAsUnion<typeof STRING_CLI_FLAGS>;
+export type StringArrayCLIFlag = ArrayValuesAsUnion<typeof STRING_ARRAY_CLI_FLAGS>;
+export type NumberCLIFlag = ArrayValuesAsUnion<typeof NUMBER_CLI_FLAGS>;
+export type StringNumberCLIFlag = ArrayValuesAsUnion<typeof STRING_NUMBER_CLI_FLAGS>;
+export type LogCLIFlag = ArrayValuesAsUnion<typeof LOG_LEVEL_CLI_FLAGS>;
 
-type KnownCLIArg = BooleanCLIArg | StringCLIArg | NumberCLIArg | StringNumberCLIArg | LogCLIArg;
+export type KnownCLIFlag =
+  | BooleanCLIFlag
+  | StringCLIFlag
+  | StringArrayCLIFlag
+  | NumberCLIFlag
+  | StringNumberCLIFlag
+  | LogCLIFlag;
 
-type AliasMap = Partial<Record<KnownCLIArg, string>>;
+type AliasMap = Partial<Record<string, KnownCLIFlag>>;
 
 /**
  * For a small subset of CLI options we support a short alias e.g. `'h'` for `'help'`
  */
-export const CLI_ARG_ALIASES: AliasMap = {
-  config: 'c',
-  help: 'h',
-  port: 'p',
-  version: 'v',
+export const CLI_FLAG_ALIASES: AliasMap = {
+  c: 'config',
+  h: 'help',
+  p: 'port',
+  v: 'version',
 };
 
+/**
+ * A regular expression which can be used to match a CLI flag for one of our
+ * short aliases.
+ */
+export const CLI_FLAG_REGEX = new RegExp(`^-[chpv]{1}$`);
+
 /**
  * Given two types `K` and `T` where `K` extends `ReadonlyArray<string>`,
  * construct a type which maps the strings in `K` as keys to values of type `T`.
@@ -223,31 +237,37 @@ type ObjectFromKeys<K extends ReadonlyArray<string>, T> = {
  * Type containing the possible Boolean configuration flags, to be included
  * in ConfigFlags, below
  */
-type BooleanConfigFlags = ObjectFromKeys<typeof BOOLEAN_CLI_ARGS, boolean>;
+type BooleanConfigFlags = ObjectFromKeys<typeof BOOLEAN_CLI_FLAGS, boolean>;
 
 /**
  * Type containing the possible String configuration flags, to be included
  * in ConfigFlags, below
  */
-type StringConfigFlags = ObjectFromKeys<typeof STRING_CLI_ARGS, string>;
+type StringConfigFlags = ObjectFromKeys<typeof STRING_CLI_FLAGS, string>;
+
+/**
+ * Type containing the possible String Array configuration flags. This is
+ * one of the 'constituent types' for `ConfigFlags`.
+ */
+type StringArrayConfigFlags = ObjectFromKeys<typeof STRING_ARRAY_CLI_FLAGS, string[]>;
 
 /**
  * Type containing the possible numeric configuration flags, to be included
  * in ConfigFlags, below
  */
-type NumberConfigFlags = ObjectFromKeys<typeof NUMBER_CLI_ARGS, number>;
+type NumberConfigFlags = ObjectFromKeys<typeof NUMBER_CLI_FLAGS, number>;
 
 /**
  * Type containing the configuration flags which may be set to either string
  * or number values.
  */
-type StringNumberConfigFlags = ObjectFromKeys<typeof STRING_NUMBER_CLI_ARGS, string | number>;
+type StringNumberConfigFlags = ObjectFromKeys<typeof STRING_NUMBER_CLI_FLAGS, string | number>;
 
 /**
  * Type containing the possible LogLevel configuration flags, to be included
  * in ConfigFlags, below
  */
-type LogLevelFlags = ObjectFromKeys<typeof LOG_LEVEL_CLI_ARGS, LogLevel>;
+type LogLevelFlags = ObjectFromKeys<typeof LOG_LEVEL_CLI_FLAGS, LogLevel>;
 
 /**
  * The configuration flags which can be set by the user on the command line.
@@ -266,6 +286,7 @@ type LogLevelFlags = ObjectFromKeys<typeof LOG_LEVEL_CLI_ARGS, LogLevel>;
 export interface ConfigFlags
   extends BooleanConfigFlags,
     StringConfigFlags,
+    StringArrayConfigFlags,
     NumberConfigFlags,
     StringNumberConfigFlags,
     LogLevelFlags {

src/cli/parse-flags.ts

@@ -1,8 +1,14 @@
 import { toDashCase } from '@utils';
 
 import { LogLevel } from '../../declarations';
-import { BOOLEAN_CLI_ARGS, NUMBER_CLI_ARGS, STRING_CLI_ARGS } from '../config-flags';
-import { parseEqualsArg, parseFlags } from '../parse-flags';
+import {
+  BOOLEAN_CLI_FLAGS,
+  NUMBER_CLI_FLAGS,
+  STRING_ARRAY_CLI_FLAGS,
+  STRING_CLI_FLAGS,
+  StringArrayCLIFlag,
+} from '../config-flags';
+import { Empty, parseEqualsArg, parseFlags } from '../parse-flags';
 
 describe('parseFlags', () => {
   it('should get known and unknown args', () => {
@@ -54,7 +60,7 @@ describe('parseFlags', () => {
    * will result in a value like `[true, true]` being set on ConfigFlags, which
    * will cause these tests to start failing.
    */
-  describe.each(BOOLEAN_CLI_ARGS)('should parse boolean flag %s', (cliArg) => {
+  describe.each(BOOLEAN_CLI_FLAGS)('should parse boolean flag %s', (cliArg) => {
     it('should parse arg', () => {
       const flags = parseFlags([`--${cliArg}`]);
       expect(flags.knownArgs).toEqual([`--${cliArg}`]);
@@ -75,14 +81,14 @@ describe('parseFlags', () => {
       expect(flags[cliArg]).toBe(false);
     });
 
-    it('should set value to null if not present', () => {
+    it('should not set value if not present', () => {
       const flags = parseFlags([]);
       expect(flags.knownArgs).toEqual([]);
-      expect(flags[cliArg]).toBe(null);
+      expect(flags[cliArg]).toBe(undefined);
     });
   });
 
-  describe.each(STRING_CLI_ARGS)('should parse string flag %s', (cliArg) => {
+  describe.each(STRING_CLI_FLAGS)('should parse string flag %s', (cliArg) => {
     it(`should parse "--${cliArg} value"`, () => {
       const flags = parseFlags([`--${cliArg}`, 'test-value']);
       expect(flags.knownArgs).toEqual([`--${cliArg}`, 'test-value']);
@@ -112,7 +118,7 @@ describe('parseFlags', () => {
     });
   });
 
-  it.each(NUMBER_CLI_ARGS)('should parse number flag %s', (cliArg) => {
+  it.each(NUMBER_CLI_FLAGS)('should parse number flag %s', (cliArg) => {
     const flags = parseFlags([`--${cliArg}`, '42']);
     expect(flags.knownArgs).toEqual([`--${cliArg}`, '42']);
     expect(flags.unknownArgs).toEqual([]);
@@ -194,7 +200,7 @@ describe('parseFlags', () => {
 
     it('should not parse --max-workers', () => {
       const flags = parseFlags([]);
-      expect(flags.maxWorkers).toBe(null);
+      expect(flags.maxWorkers).toBe(undefined);
     });
   });
 
@@ -246,12 +252,136 @@ describe('parseFlags', () => {
       ['--fooBar=baz', '--fooBar', 'baz'],
       ['--foo-bar=4', '--foo-bar', '4'],
       ['--fooBar=twenty=3*4', '--fooBar', 'twenty=3*4'],
-      ['--fooBar', '--fooBar', ''],
-      ['--foo-bar', '--foo-bar', ''],
+      ['--fooBar', '--fooBar', Empty],
+      ['--foo-bar', '--foo-bar', Empty],
+      ['--foo-bar=""', '--foo-bar', '""'],
     ])('should parse %s correctly', (testArg, expectedArg, expectedValue) => {
       const [arg, value] = parseEqualsArg(testArg);
       expect(arg).toBe(expectedArg);
-      expect(value).toBe(expectedValue);
+      expect(value).toEqual(expectedValue);
+    });
+  });
+
+  describe.each(STRING_ARRAY_CLI_FLAGS)('should parse string flag %s', (cliArg: StringArrayCLIFlag) => {
+    it(`should parse single value: "--${cliArg} test-value"`, () => {
+      const flags = parseFlags([`--${cliArg}`, 'test-value']);
+      expect(flags.knownArgs).toEqual([`--${cliArg}`, 'test-value']);
+      expect(flags.unknownArgs).toEqual([]);
+      expect(flags[cliArg]).toEqual(['test-value']);
+    });
+
+    it(`should parse multiple values: "--${cliArg} test-value"`, () => {
+      const flags = parseFlags([`--${cliArg}`, 'test-value', `--${cliArg}`, 'second-test-value']);
+      expect(flags.knownArgs).toEqual([`--${cliArg}`, 'test-value', `--${cliArg}`, 'second-test-value']);
+      expect(flags.unknownArgs).toEqual([]);
+      expect(flags[cliArg]).toEqual(['test-value', 'second-test-value']);
+    });
+
+    it(`should parse "--${cliArg}=value"`, () => {
+      const flags = parseFlags([`--${cliArg}=path/to/file.js`]);
+      expect(flags.knownArgs).toEqual([`--${cliArg}`, 'path/to/file.js']);
+      expect(flags.unknownArgs).toEqual([]);
+      expect(flags[cliArg]).toEqual(['path/to/file.js']);
+    });
+
+    it(`should parse multiple values: "--${cliArg}=test-value"`, () => {
+      const flags = parseFlags([`--${cliArg}=test-value`, `--${cliArg}=second-test-value`]);
+      expect(flags.knownArgs).toEqual([`--${cliArg}`, 'test-value', `--${cliArg}`, 'second-test-value']);
+      expect(flags.unknownArgs).toEqual([]);
+      expect(flags[cliArg]).toEqual(['test-value', 'second-test-value']);
+    });
+
+    it(`should parse "--${toDashCase(cliArg)} value"`, () => {
+      const flags = parseFlags([`--${toDashCase(cliArg)}`, 'path/to/file.js']);
+      expect(flags.knownArgs).toEqual([`--${toDashCase(cliArg)}`, 'path/to/file.js']);
+      expect(flags.unknownArgs).toEqual([]);
+      expect(flags[cliArg]).toEqual(['path/to/file.js']);
+    });
+
+    it(`should parse multiple values: "--${toDashCase(cliArg)} test-value"`, () => {
+      const flags = parseFlags([
+        `--${toDashCase(cliArg)}`,
+        'test-value',
+        `--${toDashCase(cliArg)}`,
+        'second-test-value',
+      ]);
+      expect(flags.knownArgs).toEqual([
+        `--${toDashCase(cliArg)}`,
+        'test-value',
+        `--${toDashCase(cliArg)}`,
+        'second-test-value',
+      ]);
+      expect(flags.unknownArgs).toEqual([]);
+      expect(flags[cliArg]).toEqual(['test-value', 'second-test-value']);
+    });
+
+    it(`should parse "--${toDashCase(cliArg)}=value"`, () => {
+      const flags = parseFlags([`--${toDashCase(cliArg)}=path/to/file.js`]);
+      expect(flags.knownArgs).toEqual([`--${toDashCase(cliArg)}`, 'path/to/file.js']);
+      expect(flags.unknownArgs).toEqual([]);
+      expect(flags[cliArg]).toEqual(['path/to/file.js']);
+    });
+
+    it(`should parse multiple values: "--${toDashCase(cliArg)}=test-value"`, () => {
+      const flags = parseFlags([`--${toDashCase(cliArg)}=test-value`, `--${toDashCase(cliArg)}=second-test-value`]);
+      expect(flags.knownArgs).toEqual([
+        `--${toDashCase(cliArg)}`,
+        'test-value',
+        `--${toDashCase(cliArg)}`,
+        'second-test-value',
+      ]);
+      expect(flags.unknownArgs).toEqual([]);
+      expect(flags[cliArg]).toEqual(['test-value', 'second-test-value']);
+    });
+  });
+
+  describe('error reporting', () => {
+    it('should throw if you pass no argument to a string flag', () => {
+      expect(() => {
+        parseFlags(['--cacheDirectory', '--someOtherFlag']);
+      }).toThrow('when parsing CLI flag "--cacheDirectory": expected a string argument but received nothing');
+    });
+
+    it('should throw if you pass no argument to a string array flag', () => {
+      expect(() => {
+        parseFlags(['--reporters', '--someOtherFlag']);
+      }).toThrow('when parsing CLI flag "--reporters": expected a string argument but received nothing');
+    });
+
+    it('should throw if you pass no argument to a number flag', () => {
+      expect(() => {
+        parseFlags(['--port', '--someOtherFlag']);
+      }).toThrow('when parsing CLI flag "--port": expected a number argument but received nothing');
+    });
+
+    it('should throw if you pass a non-number argument to a number flag', () => {
+      expect(() => {
+        parseFlags(['--port', 'stringy']);
+      }).toThrow('when parsing CLI flag "--port": expected a number but received "stringy"');
+    });
+
+    it('should throw if you pass a bad number argument to a number flag', () => {
+      expect(() => {
+        parseFlags(['--port=NaN']);
+      }).toThrow('when parsing CLI flag "--port": expected a number but received "NaN"');
+    });
+
+    it('should throw if you pass no argument to a string/number flag', () => {
+      expect(() => {
+        parseFlags(['--maxWorkers']);
+      }).toThrow('when parsing CLI flag "--maxWorkers": expected a string or a number but received nothing');
+    });
+
+    it('should throw if you pass an invalid log level for --logLevel', () => {
+      expect(() => {
+        parseFlags(['--logLevel', 'potato']);
+      }).toThrow('when parsing CLI flag "--logLevel": expected to receive a valid log level but received "potato"');
+    });
+
+    it('should throw if you pass no argument to --logLevel', () => {
+      expect(() => {
+        parseFlags(['--logLevel']);
+      }).toThrow('when parsing CLI flag "--logLevel": expected to receive a valid log level but received nothing');
     });
   });
 });

src/cli/test/parse-flags.spec.ts

@@ -28,6 +28,17 @@ export const dashToPascalCase = (str: string): string =>
     .map((segment) => segment.charAt(0).toUpperCase() + segment.slice(1))
     .join('');
 
+/**
+ * Convert a string to 'camelCase'
+ *
+ * @param str the string to convert
+ * @returns the converted string
+ */
+export const toCamelCase = (str: string) => {
+  const pascalCase = dashToPascalCase(str);
+  return pascalCase.charAt(0).toLowerCase() + pascalCase.slice(1);
+};
+
 export const toTitleCase = (str: string) => str.charAt(0).toUpperCase() + str.slice(1);
 
 export const noop = (): any => {

src/utils/helpers.ts

@@ -1,4 +1,4 @@
-import { dashToPascalCase, isDef, isPromise, toDashCase } from '../helpers';
+import { dashToPascalCase, isDef, isPromise, toCamelCase, toDashCase } from '../helpers';
 
 describe('util helpers', () => {
   describe('isPromise', () => {
@@ -45,6 +45,16 @@ describe('util helpers', () => {
     });
   });
 
+  describe('toCamelCase', () => {
+    it.each([
+      ['my-3d-component', 'my3dComponent'],
+      ['madison-wisconsin', 'madisonWisconsin'],
+      ['wisconsin', 'wisconsin'],
+    ])('%s => %s', (input: string, exp: string) => {
+      expect(toCamelCase(input)).toBe(exp);
+    });
+  });
+
   describe('toDashCase', () => {
     it('My3dComponent => my-3d-component', () => {
       expect(toDashCase('My3dComponent')).toBe('my-3d-component');

src/utils/test/helpers.spec.ts

@@ -172,4 +172,4 @@ const SKIP_DEPS = ['@stencil/core'];
 export const readOnlyArrayHasStringMember = <T extends string>(
   readOnlyArray: ReadonlyArray<T>,
   maybeMember: T | string
-): boolean => readOnlyArray.includes(maybeMember as typeof readOnlyArray[number]);
+): maybeMember is T => readOnlyArray.includes(maybeMember as typeof readOnlyArray[number]);