when in singleton mode, calling parse() multiple times results in undefined behavior

[btruhand]

So my situation is that I want to use a required positional argument with some non-required options. To my surprise the positional argument is not picked up as I expected. Here is what the command look like

$ > node index.js stress-test restock
index.js stress-test <notification>

Some description

Positionals:
  notification  notification type to stress test
                                        [string] [required] [choices: "restock"]

Options:
  --version             Show version number                            [boolean]
  --help                Show help                                      [boolean]
  --amount, -n          amount of notifications to create  [number] [default: 1]
  --environment, --env  environment to conduct stress test in
                                 [choices: "dev", "stg", "pro"] [default: "stg"]

Missing required argument: notification

If I try to give an argument that is not part of the argument choice this is what I get:

$ > node index.js stress-test test
index.js stress-test <notification>

Some description

Positionals:
  notification  notification type to stress test
                                        [string] [required] [choices: "restock"]

Options:
  --version             Show version number                            [boolean]
  --help                Show help                                      [boolean]
  --amount, -n          amount of notifications to create  [number] [default: 1]
  --environment, --env  environment to conduct stress test in
                                 [choices: "dev", "stg", "pro"] [default: "stg"]

Invalid values:
  Argument: notification, Given: "test", Choices: "restock"

So obviously the argument is properly picked up. Not sure what is going on here. This is what the command option looks like:

   {
      command: 'stress-test <notification>',
      describe: 'Some description',
      builder: (yargs) => {
        return yargs.positional('notification', {
          describe: 'notification type to stress test',
          choices: ['restock'],
          type: 'string'
        }).options({
          amount: {
            default: 1,
            requiresArg: true,
            describe: 'amount of notifications to create',
            alias: 'n',
            number: true
          },
          environment: {
            default: 'stg',
            requresArg: true,
            choices: ['dev','stg','pro'],
            describe: 'environment to conduct stress test in',
            alias: 'env'
          }
        });
      }
   }

[rmlevangelio]

Any solution for this question?

[btruhand]

@rmlevangelio I don't know if anyone is working on this or not but I never managed to make things working for this case. In the end I just went with options arguments instead

[varl]

Seeing the same issue in command modules. Any solutions a year later?

[bcoe]

@varl odd, out of curiosity does it work with the chaining API, when you're not using command modules?

I tend to use the chaining API and have never bumped into this.

[mleguen]

I am unable to reproduce the issue with @btruhand 's command (either with 13.2.4 or 13.3.0) in a command module:

• node index.js stress-test test is refused
• node index.js stress-test restock is accepted

@varl could you please give some more details on your command module?

[varl]

@mleguen @bcoe Thanks for looking into this. At the end of the day it looks like we are just plain old "doing it wrong" internally. 😅

We inadvertently wound up calling yargs.parse() twice: once in our makeEntryPoint library function (repo 1) we use to generate command modules, and then again when the entry point was actually executed (repo 3,4,5, etc..).

The first time the required argument is parsed, it is correctly picked up, but the second time parse runs it is missing and causes the "missing required argument: name" message. Calling parse twice on the same instance seems incorrect; at least it is not idempotent.

./cli-broken.js test foobar               
should print the name: foobar
cli-broken.js test <name>

Delete tracked branches gone stale for remotes

Positionals:
  name  a name                           [string] [required] [choices: "foobar"]

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

Missing required argument: name

I've condensed the implementation which spans multiple repositories and tools into a single one here: https://github.com/varl/yargs-1137-reproduction

Is there something I can do to help the devs who implement command modules for our CLI tool avoid inadvertently calling parse twice?

[mleguen]

@varl I suggest you use yargs's parsed property value before calling parse: if false, it's safe to call parse(), otherwise not.

But as commented in #1144 do not check the parsed property of the "object" returned by require("yargs), but of the real yargs object returned by any chained method. In your example, the object return by your your builder function would do the job.

[varl]

@mleguen Thanks for taking the time to check it out and for pointing out the gotcha! I will change it in accordance to your suggestion. 👍

[bcoe]

@mleguen thanks for helping dig into this 👍

@varl if I understand correctly, was the behavior different if you called:

argv = yargs.parse()
argv = yargs.parse()

i.e., calling parse twice gave different results ... this I don't love, and should be fixed (even if calling it once is a good workaround).

[varl]

@bcoe Yes, the behavior was different.

A minimal example

#!/usr/bin/env node

const yargs = require('yargs')

let argv = yargs
    .command(
        'test <name>',
        'prints foobar if foobar is passed',
        yargs => yargs.positional('name', {
            aliases: 'n',
            describe: 'a name',
            choices: ['foobar'],
            type: 'string',
        }),
        argv => console.log(`should print the name: ${argv.name}`)
    )

argv = yargs.parse()
argv = yargs.parse()

The result from a run

./cli-double.js test foobar
should print the name: foobar
cli-double.js test <name>

Delete tracked branches gone stale for remotes

Positionals:
  name  a name                           [string] [required] [choices: "foobar"]

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

Missing required argument: name

First it prints the correct line: "should print the name: foobar" and when argv.parse() is called a second time, I'm unsure what happens, but the name argument is gone, so it prints the help text.

It behaves as if the args have already been consumed and aren't available the second time parse() is called on yargs. I don't expect the handler to run more than once on multiple parse() calls, as that could lead to other bugs.

[mleguen]

Issue reproduced with @varl 's example.

[mleguen]

The problem comes from command.runCommand using yargs.reset and not restoring yargs afterwards.

So in the second call to yargs.parse(), yargs is no longer waiting for a "test" command, but directly for a "name" positional it does not find.

We have to find a way to "unreset" yargs before leaving runCommand.

[mleguen]

I suggest to extend the parse(args, cb) freeze/unfreeze "hack" to every run of _parseArgs:

• freeze when entering _parseArgs
• unfreeze before leaving (to be done before every return)
• handle a freezes stack to allow nested freezes

The alternative is to copy yargs instead of resetting it to handle nested commands parsing, but would require much more work.

@bcoe what do you think of this?

[mleguen]

Forget it. It breaks dozens of tests.

In fact, I understand we are indeed expecting yargs.parse() not to be idempotent in several places in the code, for example to allow yargs.parse() to be called in the builder function, as in this test:

yargs/test/command.js

Lines 824 to 831 in 266ed91

	it('allows builder function to return parsed argv', () => {
	const argv = yargs('yo Leslie')
	.command('yo <someone>', 'Send someone a yo', yargs => yargs.parse(), (argv) => {
	argv.should.have.property('someone').and.equal('Leslie')
	})
	.parse()
	argv.should.have.property('someone').and.equal('Leslie')
	})

So I frankly do not see how to allow calling parse() twice without breaking some other things we already allow.

Does someone have another idea?

[mleguen]

I finally found a way not to break anything by freezing/unfreezing directly in parse, as it was already done when calling parse with arguments. I should be able to submit a PR soon.

[bcoe]

@btruhand if you npm i yargs@next you should now be able to run .parse() multiple times.

convince you to try this out on your original bug, and see if it does the trick for you?

[btruhand]

@bcoe thanks for everyone's dedicated support. Am not using it anymore but I would try it out tomorrow out of curiosity