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

feat: add InferredFlags type #473

Merged
merged 11 commits into from Aug 23, 2022
Merged

feat: add InferredFlags type #473

merged 11 commits into from Aug 23, 2022

Conversation

mdonnalley
Copy link
Contributor

@mdonnalley mdonnalley commented Aug 16, 2022

  • Adds InferredFlags to Interfaces export. See details below
  • Adds Flags.custom to replace Flags.build. See details below
  • Adds type tests for flags using tsd
  • Fixes the default property always expecting a single item even if multiple is set to true

Potential Breaks

In order to switch the expected type of default based on multiple the Interfaces.OptionFlag no longer has statically known members. Downstream, this means that any interface that extends Interfaces.OptionFlag will fail to compile. To fix this, you just need to switch to using a type:

Before

export interface IdFlagConfig extends Partial<Interfaces.OptionFlag<string>> {
  length?: 15 | 18;
  startsWith?: string;
}

After

export type IdFlagConfig = Partial<Interfaces.OptionFlag<string>>  & {
  length?: 15 | 18;
  startsWith?: string;
};

Additionally, the same change may cause custom flags that use build to fail compilation if you purposely override the default. For example:

export function durationFlag(durationConfig: DurationFlagConfig): Interfaces.OptionFlag<Duration> {
  const { defaultValue, min, max, unit, ...baseProps } = durationConfig;
  return Flags.build({
    ...baseProps,
    parse: async (input: string) => validate(input, { min, max, unit }),
    default: defaultValue ? toDuration(defaultValue, unit) : undefined,
  })();
}

This fails because the default type doesn't know what the value of baseProps.multiple is. In order to fix this you need to add an if/else like so:

export function durationFlag(
  durationConfig: DurationFlagConfig
): Interfaces.OptionFlag<Duration> | Interfaces.OptionFlag<Duration | undefined> {
  const { defaultValue, min, max, unit, multiple, ...baseProps } = durationConfig;
  if (multiple) {
    return Flags.build({
      ...baseProps,
      parse: async (input: string) => validate(input, { min, max, unit }),
      multiple: true,
      default: defaultValue ? async () => [toDuration(defaultValue, unit)] : undefined,
    })();
  } else {
    return Flags.build({
      ...baseProps,
      parse: async (input: string) => validate(input, { min, max, unit }),
      multiple: false,
      default: defaultValue ? async () => toDuration(defaultValue, unit) : undefined,
    })();
  }
}

InferredFlags

Infer the flags that are returned by Command.parse. This is useful for when you want to assign the flags as a class property.

export type StatusFlags = Interfaces.InferredFlags<typeof Status.flags & typeof Status.globalFlags>

export abstract class BaseCommand extends Command {
  static enableJsonFlag = true
  static globalFlags = {
    config: Flags.string({
      description: 'specify config file',
    }),
  }
}

export default class Status extends BaseCommand {
  static flags = {
    force: Flags.boolean({char: 'f', description: 'a flag'}),
  }
  public flags!: StatusFlags
  public async run(): Promise<StatusFlags> {
    const result = await this.parse(Status)
    this.flags = result.flags
    return result.flags
  }
}

Flags.custom

Custom flags built using Flags.build were not properly typed unless you specified a variety of function overloads (example).

To replace that, we've introduced Flags.custom which allows us to define a custom with correct typing and no function overloading required:

Before

export function integer(opts: Partial<OptionFlag<number>> & {min?: number; max?: number } & {multiple: true} & ({required: true} | { default: Default<number> })): OptionFlag<number[]>
export function integer(opts: Partial<OptionFlag<number>> & {min?: number; max?: number } & {multiple: true}): OptionFlag<number[] | undefined>
export function integer(opts: Partial<OptionFlag<number>> & {min?: number; max?: number } & ({required: true} | { default: Default<number> })): OptionFlag<number>
export function integer(opts?: Partial<OptionFlag<number>> & {min?: number; max?: number }): OptionFlag<number | undefined>
export function integer(opts: Partial<OptionFlag<number>> & {min?: number; max?: number } = {}): OptionFlag<number> | OptionFlag<number[]> | OptionFlag<number | undefined> | OptionFlag<number[] | undefined> {
  return build({
    ...opts,
    parse: async input => {
      if (!/^-?\d+$/.test(input))
        throw new Error(`Expected an integer but received: ${input}`)
      const num = Number.parseInt(input, 10)
      if (opts.min !== undefined && num < opts.min)
        throw new Error(`Expected an integer greater than or equal to ${opts.min} but received: ${input}`)
      if (opts.max !== undefined && num > opts.max)
        throw new Error(`Expected an integer less than or equal to ${opts.max} but received: ${input}`)
      return opts.parse ? opts.parse(input, 1) : num
    },
  })()
}

After

export const integer = custom<number, {min?: number; max?: number;}>({
  parse: async (input, ctx, opts) => {
    if (!/^-?\d+$/.test(input))
      throw new Error(`Expected an integer but received: ${input}`)
    const num = Number.parseInt(input, 10)
    if (opts.min !== undefined && num < opts.min)
      throw new Error(`Expected an integer greater than or equal to ${opts.min} but received: ${input}`)
    if (opts.max !== undefined && num > opts.max)
      throw new Error(`Expected an integer less than or equal to ${opts.max} but received: ${input}`)
    return num
  },
})

@mdonnalley mdonnalley self-assigned this Aug 16, 2022
mshanemc
mshanemc previously approved these changes Aug 18, 2022
Copy link
Member

@mshanemc mshanemc left a comment

Choose a reason for hiding this comment

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

approved but would love to have some assertion about it

* }
* }
*/
export type InferredFlags<T> = T extends FlagInput<infer F> ? F & { json: boolean | undefined; } : unknown
Copy link
Member

Choose a reason for hiding this comment

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

is there any way to test this? Maybe by building a test class and having a flags property like your example, and then asserting things about its type?

What I'd like to do is prevent some future flag changes/refactoring/type fixing from breaking this feature.

Copy link
Member

@mshanemc mshanemc left a comment

Choose a reason for hiding this comment

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

lgtm. Will approve once the sf-plugins-core stuff is updated.

Do you want to do the updates on sf-plugins-core or me?

Comment on lines +128 to +131
public flags!: MyFlags

public async run(): Promise<MyFlags> {
const result = await this.parse(MyCommand)
Copy link
Member

Choose a reason for hiding this comment

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

I understand why the ! is there in here and in your example. It's kinda true...as long as people make
const {flags} = await this.parse(MyCommand) (or whatever) their very first step in run.

Is that worth adding to our linter? Are there situations where someone wouldn't want to parse the flags first (ex: checking some env, etc?) or not at all?

Otherwise, if they do the public flags!: MyFlags but forget to parse (or try to use this.flags before parsing) the compiler won't warn them.

Or maybe the rule is "don't refer to the instance flags prop if you haven't called the parse and assigned it?" ?

expectType<MyFlags>(this.flags)

expectType<string>(this.flags.requiredGlobalFlag)
expectNotType<undefined>(this.flags.requiredGlobalFlag)
Copy link
Member

Choose a reason for hiding this comment

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

this stuff is just way cool.

@@ -11,7 +11,8 @@
"strict": true,
"target": "es2020",
"lib": ["es2020"],
"allowSyntheticDefaultImports": true
"allowSyntheticDefaultImports": true,
"noErrorTruncation": true,
Copy link
Member

Choose a reason for hiding this comment

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

oh, nice. How would you feel about this being in dev-config?

export type CustomOptionFlag<T, P = any> = FlagBase<T, string, P> & OptionFlagProps & {
defaultHelp?: DefaultHelp<T>;
input: string[];
} & ({
Copy link
Member

Choose a reason for hiding this comment

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

very nice.

@mdonnalley mdonnalley merged commit ee5ce65 into main Aug 23, 2022
@mdonnalley mdonnalley deleted the mdonnalley/inferred-flags branch August 23, 2022 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants