feat: add InferredFlags type by mdonnalley · Pull Request #473 · oclif/core

mdonnalley · 2022-08-16T17:08:59Z

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
  },
})

mshanemc

approved but would love to have some assertion about it

mshanemc · 2022-08-18T00:49:51Z

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


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.

mshanemc

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

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

mshanemc · 2022-08-20T00:13:02Z

+  public flags!: MyFlags
+
+  public async run(): Promise<MyFlags> {
+    const result = await this.parse(MyCommand)


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?" ?

mshanemc · 2022-08-20T00:13:48Z

+    expectType<MyFlags>(this.flags)
+
+    expectType<string>(this.flags.requiredGlobalFlag)
+    expectNotType<undefined>(this.flags.requiredGlobalFlag)


this stuff is just way cool.

mshanemc · 2022-08-20T00:15:45Z

    "lib": ["es2020"],
-    "allowSyntheticDefaultImports": true
+    "allowSyntheticDefaultImports": true,
+    "noErrorTruncation": true,


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

mshanemc · 2022-08-20T00:17:30Z

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


feat: add InferredFlags type

0f1954b

mdonnalley self-assigned this Aug 16, 2022

mdonnalley requested a review from mshanemc August 16, 2022 17:09

mdonnalley and others added 3 commits August 16, 2022 11:11

Merge branch 'main' into mdonnalley/inferred-flags

f30fe50

Merge branch 'main' into mdonnalley/inferred-flags

c56124d

Merge branch 'main' into mdonnalley/inferred-flags

bdbc890

mshanemc previously approved these changes Aug 18, 2022

View reviewed changes

mdonnalley added 2 commits August 18, 2022 11:02

Merge branch 'main' into mdonnalley/inferred-flags

ec3f319

chore: add test for flag types

68503b7

mdonnalley dismissed mshanemc’s stale review via 68503b7 August 19, 2022 14:43

mdonnalley added 4 commits August 19, 2022 08:46

chore: more type tests

870567f

chore: clean up

5aa13aa

feat: add custom flag builder

67e69f8

chore: fix tests

39a9b50

mshanemc reviewed Aug 20, 2022

View reviewed changes

fix: custom flag builder

1b8baa2

mdonnalley mentioned this pull request Aug 23, 2022

feat: update custom flag definitions salesforcecli/sf-plugins-core#67

Merged

mdonnalley merged commit ee5ce65 into main Aug 23, 2022

mdonnalley deleted the mdonnalley/inferred-flags branch August 23, 2022 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add InferredFlags type#473

feat: add InferredFlags type#473
mdonnalley merged 11 commits intomainfrom
mdonnalley/inferred-flags

mdonnalley commented Aug 16, 2022 •

edited

Loading

Uh oh!

mshanemc left a comment

Uh oh!

mshanemc Aug 18, 2022

Uh oh!

mshanemc left a comment

Uh oh!

mshanemc Aug 20, 2022

Uh oh!

mshanemc Aug 20, 2022

Uh oh!

mshanemc Aug 20, 2022

Uh oh!

mshanemc Aug 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

mdonnalley commented Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Potential Breaks

InferredFlags

Flags.custom

Uh oh!

mshanemc left a comment

Choose a reason for hiding this comment

Uh oh!

mshanemc Aug 18, 2022

Choose a reason for hiding this comment

Uh oh!

mshanemc left a comment

Choose a reason for hiding this comment

Uh oh!

mshanemc Aug 20, 2022

Choose a reason for hiding this comment

Uh oh!

mshanemc Aug 20, 2022

Choose a reason for hiding this comment

Uh oh!

mshanemc Aug 20, 2022

Choose a reason for hiding this comment

Uh oh!

mshanemc Aug 20, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mdonnalley commented Aug 16, 2022 •

edited

Loading