Skip to content

Design footgun. If you don't call parse() the CLI silently ignores all flags #1375

@Veetaha

Description

@Veetaha

The PR title pretty much describes it, so I'd rather tell the full story of how I discovered it and why I think it's a design footgun 🦵 🔫

I've written a command like this that takes zero arguments and zero flags. It just performs a single action without any CLI-level parameterization:

import { Command } from "@oclif/core";

export default class Destroy extends Command {
    static override description = "Destroys the application stack";

    async run() {
        // Destroy goes brr....
        console.log("Destroy completed!");
    }
}

And I've been happy with it for a while because everything seemed to work fine. But then after some time, I noticed something weird. I've been writing commands like deploy --foo bar and destroy interchangeably until I realized that I wrote destroy --foo bar mistakenly and it silently worked! I could write any gibberish like destroy --asd- wew rdf- and the CLI ate it just fine.

I've been scratching my head for some time with this trying to find existing issues in oclif about the bugs in ignoring unknown/extra flags until I actually realized what I did wrong. I just forgot to add this into my run() method:

await this.parse(Destroy);

This is something I would never think about because I didn't need to take any input for my command so I thought I didn't need to parse anything but it was wrong. Parsing not only reads the input but also validates for extra parameters in the CLI which isn't something one would think about first, because people tend to think about the successful cases first, and relying on the CLI library to handle the syntax errors for them as granted.

Having this ceremony of calling this.parse() even if you don't need to take any arguments is something I think is wrong by design. The feature of parsing the CLI input must be provided by default and not opt-in. Instead, there can be an option to opt-out.

I'd imagine an API where we don't need to call parse() at all, and instead the run() method accepts the args/flags as its parameter. I know there may be some other nuances with this and this breaks the compatibility, but I just want to point out that this design footgun exists so people should keep it in mind. At the very least a warning can be added to the docs to try to prevent this problem

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions