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

Using $0 (a default command) as an alias breaks the usage output for nested commands #2291

Closed
bglimepoint opened this issue Jan 16, 2023 · 9 comments · Fixed by #2303
Closed
Labels

Comments

@bglimepoint
Copy link

bglimepoint commented Jan 16, 2023

We've run into an issue with yargs when using an alias for the default command.

Rather than outputting the correct usage for the subcommand when there's a typo, it instead is outputting the usage for the top level commands, but like they're nested under the subcommand.

We've create a little reproduction - https://github.com/bglimepoint/yargs-alias-help-text-bug-example.

The expected output would be:

$ node index.js test testingtypo

index.js test

parent - test description

Commands:
  index.js test testing   testing description
  index.js test testing2  testing description
  index.js test testing3  testing description

Options:
  --version  Show version number  [boolean]
  --help     Show help  [boolean]

Unknown argument: testingtypo

Which is what we get when we remove the aliases: ['$0'] from one of our subcommands; but when using the default command we get the following bug (afaict) instead:

$ node index.js test testingtypo
index.js test testing

testing description

Commands:
  index.js test test        parent - test description
  index.js test command2    parent - command2 description
  index.js test completion  generate completion definition script

Options:
  --version  Show version number  [boolean]
  --help     Show help  [boolean]

Unknown argument: testingtypo

Notice that the usage there is for the top level things (completion, and then the two fake top level commands), not the usage for the test subcommand.

The repro repo has the buggy version on the master branch, and the "working" version on the no-alias branch (due to the removal of the alias).

Hopefully we're not just doing something silly :-D

Thanks for your time and efforts with yargs :-)

@Tobbe
Copy link

Tobbe commented Jan 23, 2023

I just spent more than two hours trying to figure out what I was doing wrong, and then trying to create a small reproduction to post as an issue. Just to find that the very first issue in the list is this one, describing my exact scenario 😆

Only thing I can add is that I'm seeing the same behavior even without the alias, but with just a regular default command

.command('$0', false, () => {}, (argv) => {
  console.log('argv', argv)
  if (argv.list) {
    console.log('This is the full list')
  }
})

@Tobbe
Copy link

Tobbe commented Jan 23, 2023

@bglimepoint Thanks for your reproduction. I simplified it even more.
This single file example still shows the weird/buggy behavior

const yargs = require("yargs");

yargs
  .command({
    command: "test [subcommand]",
    describe: "parent - test description",
    builder(yargs) {
      yargs.command("$0", "testing description", () =>
        console.log("nested default")
      );
      yargs.command("testing2", "testing description", () =>
        console.log("nested")
      );
      yargs.command("testing3", "testing description", () =>
        console.log("nested")
      );
    },
  })
  .command({
    command: "command2",
    describe: "parent - command2 description",
    builder(yargs) {
      yargs.command("testing", "testing description", () =>
        console.log("nested")
      );
    },
    handler() {
      console.log("handler test");
    },
  })
  .strict()
  .help()
  .parse();

Looking through older issues I think this is a duplicate of #2247

@Tobbe
Copy link

Tobbe commented Jan 23, 2023

One workaround is to remove .strict() and handle that yourself

const yargs = require("yargs");

yargs
  .command({
    command: "test [subcommand]",
    describe: "parent - test description",
    builder(yargs) {
      yargs.command(
        "$0",
        "testing description",
        () => {},
        (argv) => {
          if (
            argv._.length > 2 ||
            (argv._.length === 2 &&
              argv._[1] !== "testing2" &&
              argv._[1] !== "testing3")
          ) {
            yargs.showHelp();
            return;
          }

          console.log("nested custom", argv);
        }
      );
      yargs.command("testing2", "testing description", () =>
        console.log("nested")
      );
      yargs.command("testing3", "testing description", () =>
        console.log("nested")
      );
    },
  })
  .command({
    command: "command2",
    describe: "parent - command2 description",
    builder(yargs) {
      yargs.command("testing", "testing description", () =>
        console.log("nested")
      );
    },
    handler() {
      console.log("handler test");
    },
  })
  // .strict()
  .help()
  .parse();

@bcoe
Copy link
Member

bcoe commented Feb 13, 2023

@Tobbe thanks for the reproduction and workaround.

I'm unlikely to have much time for open source in the coming months (work has been taking much of my time, and I'm doing the minimal work on yargs necessary to make sure critical security bugs and regressions are addressed).

I would happily give the maintainer access to some folks on the RedwoodJS team, if you want to dig into any of the annoying bugs biting you.

@Tobbe
Copy link

Tobbe commented Feb 13, 2023

Hi Ben. I know the struggle. I try to balance a full-time job and contributing to RW on my spare time 🙂
I mentioned your message to the rest of the RW team and they all said I should take you up on your offer. But I'm a bit hesitant because I don't feel like I know yargs well enough to do any meaningful contributions. We'll all keep it in mind though if we run into any showstoppers in the future.

Thanks for all the work you've done so far!

@shadowspawn
Copy link
Member

shadowspawn commented Feb 20, 2023

The reproduction example linked in the original report (thanks!) has the default command an extra level down from the root. The error handling "unfreezes" the usage, but this throws the context right back to the root (as per the comment) instead of just one level up.

yargs/lib/command.ts

Lines 311 to 315 in 663c1b6

// A null command indicates we are running the default command,
// if this is the case, we should show the root usage instructions
// rather than the usage instructions for the nested default command:
if (isDefaultCommand)
innerYargs.getInternalMethods().getUsageInstance().unfreeze(true);

I suspect this scenario might be fixed by calling freeze each time an explicit command is processed, but not sure if this will break other cases. More checking to see whether more freezing is the answer...

bcoe added a commit that referenced this issue Feb 21, 2023
freeze() was not being called prior to reset(), resulting in display
issues for default sub-commands.

Fixes #2291
@bcoe
Copy link
Member

bcoe commented Feb 21, 2023

@bglimepoint @Tobbe, a fix for this should now be available in yargs@ 17.7.1.

@Tobbe, if you find yourself taking on weird workarounds in Redwood, feel free to reach out to me in bugs like this (or out of band on Twitter). I have minimal time for open source these days, and want to prioritize the major platforms using yargs (like Redwood).

@Tobbe
Copy link

Tobbe commented Feb 21, 2023

Thank you @bcoe, that's reassuring to hear

@bglimepoint
Copy link
Author

@bcoe Huge thanks for the fix, it's working well for us :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants
@Tobbe @bcoe @shadowspawn @bglimepoint and others