Configuration parser called twice

[codedot]

$ cat test.js
function load(path)
{
        console.warn("warning: %s not found", path);
        return {};
}

require("yargs")
        .config("config", load)
        .global("config")
        .command("command")
        .argv;
$ node test --config /tmp/missing/file command
warning: /tmp/missing/file not found
warning: /tmp/missing/file not found
$ 

[joelpinheiro]

Hi. I made a pull request with my proposed solution.

[nexdrew]

This is a problem concerning the relationship between argv parsing (via yargs-parser) and "lazy" command execution (via yargs), as described below:

1. argv parsing is executed with configured options
2. yargs determines if any parsed args represent a command
3. if so, that command's "builder" is run to configure the options for that command
4. repeat steps 1-3 until no more (nested) commands are found

Since command configuration is lazy/dynamic in this approach, it is necessary to do argv parsing more than once (N+1 times, in fact, where N represents the number of nested subcommands given on the command line). And since the config file loading/parsing is handled by the argv parsing logic in yargs-parser (which has no concept of commands), it is executed each time argv parsing occurs (assuming the config option is valid for that command). When the config option is marked as global, it becomes valid for every command and every level of parsing/execution.

The short-term workaround is to not use a global config option, but rather define it as an option only at the "innermost" commands. For the example above, it would look more like this:

function load (path) {
  console.warn("warning: %s not found", path)
  return { hello: 'world' }
}
require('yargs')
  .command('command', 'Description', yargs => yargs.config('config', load), argv => {
    // by this time, load has been called and its result has been applied to argv
    console.log(argv.config) // path given on command line
    console.log(argv.hello) // value from load function
  })
  .argv

I'm not quite sure what the best long-term solution is, but I think we need to add some concept of commands down to the yargs-parser level, which would allow us to parse positional args there (giving them 1st-class citizen status) as well as defer/cache config file loading/parsing as necessary. Perhaps we also need to take a harder look at changing the command API to better support a fully declarative DAG instead of a lazily-built one.

[codedot]

I think the whole approach of global-vs-local options is a bad idea. It is very annoying that I have to mark every single option as global, and work around issues that exist just to support some unnecessary abstraction nobody needs. When someone starts to call a bug a feature, it is a good sign it is time to leave. So, I am thinking of using some other library instead.

[bcoe]

@codedot in yargs@7.x we're switching the default behavior to having options applied globally; we'll probably introduce a helper that allows for the opposite behavior, .e.g., .global(false), but everything will default to true.

[davewasmer]

FWIW, the lazy invocation of the builder() method here is crucial for me (in fact, didn't realize it was lazy until I stumbled on this issue, which is exactly what I needed). I've got a couple commands that do some expensive on-the-fly subcommand discovery in their builder() methods. By lazily invoking the builder() method, I don't have to worry that the user will have to wait for that expensive subcommand discovery for all the other commands that don't need it.

Just thought I'd mention the use case given @bcoe's comment about investigating a fully declarative command API vs. the current lazy invocation approach.

[bcoe]

@codedot @davewasmer we've just released a beta of yargs 7.x, which now defaults most options to being applied globally. Would love your help testing the next major release, before we promote it to a wider audience:

npm cache clear; npm i yargs@next

[codedot]

This bug is still reproduced for yargs@7.0.2.