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

Recommended way to override parse in custom command #613

Closed
Arkham opened this issue Feb 7, 2023 · 4 comments
Closed

Recommended way to override parse in custom command #613

Arkham opened this issue Feb 7, 2023 · 4 comments

Comments

@Arkham
Copy link

Arkham commented Feb 7, 2023

Hello there! Congrats for releasing version 2.0 :)

We're happy users of oclif at shopify/cli and I was going through the migration guide from v1. We currently have created our own base command which overrides the default implementation of parse.

Currently we import these types from Interfaces in order to do so:

  • FlagInput
  • FlagOutput
  • OutputArgs
  • Input
  • ParserOutput

But in v2 these types are no longer exposed from the Interfaces module.

I was wondering what is your recommendation in upgrading our code to v2. I'm happy to put together a PR to re-expose these types, but I wanted to hear if you had some ideas.

We currently override the parse logic because we want to load some configuration from disk and want our CLI to receive those customisations as flags, just if as the user had passed them in.

I always felt a bit iffy in how we're overriding the parse function, so maybe there is a nicer solution. Thank you!

@mdonnalley
Copy link
Contributor

mdonnalley commented Feb 7, 2023

@Arkham Thanks for reaching out!

We stoped exposing those types (as well as many others) because it was becoming difficult to make necessary changes without potentially introducing a breaking change to the types. You can, however, still access them like this:

import {FlagInput, FlagOutput, OutputArgs, Input, ParserOutput} from '@oclif/core/lib/interfaces/parser'

That being said we're open to a PR to re-expose those types if you feel like that would be best for you.

I do have one suggestion. Instead of overriding parse in your base command class, you could use the init method to add additional flags to the parse result. You can see an example of this in the Custom Base Class docs. For your purposes, the init method would look something like this:

  public async init(): Promise<void> {
    await super.init()
    // parse flags and args 
    const {args, flags} = await this.parse({
      flags: this.ctor.flags,
      baseFlags: (super.ctor as typeof BaseCommand).baseFlags,
      args: this.ctor.args,
      strict: this.ctor.strict,
    })
    // custom logic here
    const additionalFlags = await readFromDiskAndMakeFlags()

    // assign flags and args as class properties
    this.flags = {...flags, ...additionalFlags} as Flags<T>
    this.args = args as Args<T>
  }

@Arkham
Copy link
Author

Arkham commented Feb 7, 2023

Hey Mike, thank you for your swift reply. That looks much cleaner than our approach :) I'll check it out next week and report back.

@amcaplan
Copy link
Contributor

Hi @mdonnalley, I'm just picking this up after some time. (Always lots to do...)

I think some additional context might be helpful.

The flags we're adding aren't in addition to what the user passed in; instead, it's a set of defaults defined by the user in a file. So for example, imagine a user has an environments file that looks like this:

{
  "dev": {
    "domain": "dev.myproject.dev",
    "buildMode": "development"
  },
  "staging": {
    "domain": "staging.myproject.dev",
    "buildMode": "development"
  },
  "prod": {
    "domain": "prod.myproject.com",
    "buildMode": "production"
  }
}

Then we allow them to specify values in the command or from the file:

# These are equivalent
$ my-cli deploy --env dev
$ my-cli deploy --build-mode development --domain dev.myproject.dev

The rules are:

  1. Explicit values from the command line take precedence. If unspecified, and an environment has been specified, use the values from the environment. If there is no value there, use the default value in the flag defaults.
  2. Invalid values in the file are error conditions (e.g. "domain": 5 or "domain": "myaddress@myproject.com" or an unknown key) and are validated exactly as though they'd been passed in.
  3. We are able to report to the user which values have been activated from their environment:
    The following values were used from environment `dev`:
    • domain: "dev.myproject.dev"
    • buildMode: "development"
    

So we need to know which values in the ultimately chosen flags were specified on the command line, which were specified in the file, and which are populated by defaults.

The way we've done this to date is unfortunately an extremely convoluted 3-pass strategy, but it's the best I could come up with:

  1. Parse naively, just to see which is the environment selected
  2. Convert the environment keys/values to command-line strings and parse again (note I previously tried using the env values as defaults, but defaults aren't validated by oclif, so it didn't work)
  3. Remove all defaults from the flags and parse again

The output of 2 is the actual flags, and the diff to the outputs of 2 and 3 is the effect of applying the environment.

I recognize this is wildly complicated, but the main point is that the types of our additional flags are actually dependent on the types we've described, because they're just sorta-defaults of the flags that are defined in the static flags object.

You can see the actual code here.

For now we've imported from the types' new home as you suggested, but any suggestions for improvement are certainly welcome.

@amcaplan
Copy link
Contributor

By the way, the documentation for the actual feature is here in case it helps to understand why this UX is valuable.

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

No branches or pull requests

3 participants