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

fix(cli): refactor CLI argument parser #3765

Merged
merged 1 commit into from
Nov 7, 2022
Merged

fix(cli): refactor CLI argument parser #3765

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 44 additions & 23 deletions src/cli/config-flags.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a bit of a 🤔 and thought these arrays should be arrays of flags rather than args, so I renamed...if we decide the new name isn't an improvement happy to back the change out!

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't comment on this the first time around, but I do think this is an improvement & am in favor of keeping the change 👍

'build',
'cache',
'checkVersion',
Expand Down Expand Up @@ -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',
Expand All @@ -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',
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reversed this object to make it easier to get the full config flag name from the alias.

};

/**
* 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}$`);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we could programmatically grab the keys from CLI_FLAG_ALIASES here so that we don't have to update this regex when we update the object above. Would something like this work?

Suggested change
export const CLI_FLAG_REGEX = new RegExp(`^-[chpv]{1}$`);
export const CLI_FLAG_REGEX = new RegExp(`^-[${Object.keys(CLI_FLAG_ALIASES).join('')}]{1}$`);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually had basically exactly that originally but I ended up having to change it because it caused a tree shaking issue, I think because then CLI_FLAG_REGEX had a dependency on CLI_FLAG_ALIASES so the treeshaking check that we do for the build verification broke 😢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

which does introduce a bit of an unfortunate situation where we can't automatically keep the regex in sync with the defined aliases. If we have some precedent for adding an exception for the tree-shaking build verification thingy I'd be open to doing that

Copy link
Contributor

Choose a reason for hiding this comment

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

Gahh that's shame :-( We don't (have a precedent). I feel like we should put something on the books to document the treeshaking check a bit better. Sometimes I think about it as a black box that we always abide by. Stubbed out STENCIL-627 to do just that, but I don't think that should block this effort


/**
* 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`.
Expand All @@ -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.
Expand All @@ -266,6 +286,7 @@ type LogLevelFlags = ObjectFromKeys<typeof LOG_LEVEL_CLI_ARGS, LogLevel>;
export interface ConfigFlags
extends BooleanConfigFlags,
StringConfigFlags,
StringArrayConfigFlags,
NumberConfigFlags,
StringNumberConfigFlags,
LogLevelFlags {
Expand Down
Loading