Mingun commented 3 years ago

Problem

Adding support of config files when you have an option with a default value and you want the following priorities, is complicated.

Value from the command line
Otherwise value from the config file
Otherwise the default value

Because commander has no built-in support for config files, you can't use all its power, because options object that you get by calling opts() contains mixed default parameters (point 1 from above) and cli parameters (3) and you should manually inspect each option with default to correctly apply the config file (2) parameters. I would like if commander could do this for me.

The current workaround looks like this:

import { Command, Option } from "commander";

/// All options with defaults should be declared there
const DEFAULTS = {
  optionWithDefault: "default value",
};
let options = new Command()
  .option("--config <json>")
  .addOption(
    new Option("--option-with-default <value>")
      .default(DEFAULTS.optionWithDefault)
  )
  .parse()
  .opts();

// Remove all default options from the parsed options
for (const key of Object.keys(DEFAULTS)) {
  if (DEFAULTS[key] === options[key]) {
    delete options[key];
  }
}

const cliOptions = options;
const config = options.config;
delete options.config;

options = Object.assign({}, DEFAULTS);
if (config) {
  options = Object.assign(options, readFileAsJSON(config));
}

but it is incomplete: if I specify --option-with-default "default value" in the command line the code above will think that it is default value and override it with the config option value, which is not expected. To solve this problem I'll have to parse options manually, which effectively makes commander useless in such scenario.

Proposal

The proposed API could looks like:

import path from "path";

let options = new Command()
  .option("--option <value>")
  .addOption(
    new Option("--default <value>")
      .default("default")
  )
  .addOption(
    new Option("--config <file>")
      // Optional function should return Object with properties with values
      // from the config file
      // By default uses the specified function that could process both .json
      // and .js (as CommonJS modules, like eslint) config files
      .config(val => require(path.resolve(val)))
      // If config not specified and have a default, this file tried to read
      // using config read function from `.config()`
      .default(".config.js")
  )

The examples for the CLI definition below:

`.config.js` does not exist

Command: prog Result:
```
{ default: "default" }
```
Command: prog --default a Result:
```
{ default: "a" }
```

Command: prog --option b Result:

{ default: "default", option: "b" }

`.config.js` contains `option`

module.exports = {
option: "config",
};

Command: prog Result:

{ default: "default", option: "config" }

Command: prog --default a Result:
```
{ default: "a", option: "config" }
```

Command: prog --option b Result:

{ default: "default", option: "b" }

`.config.js` contains `default`

module.exports = {
default: "config",
};

Command: prog Result:
```
{ default: "config" }
```
Command: prog --default a Result:
```
{ default: "a" }
```
Command: prog --option b Result:
```
{ default: "config", option: "b" }
```

shadowspawn commented 3 years ago

Commander does not explicitly track whether an option has been specified on the command-line so I can see that applying the priorities for adding in values from config files is currently tricky. Some of the same issues come up with mixing in environment variables.

One work-around approach might be to not specify the default value to Commander, and instead modify the Help to display the external default value (in your example, from DEFAULTS object). Then you can hopefully merge the potential values in the desired order: from DEFAULTS, config file if specified, and command-line.

shadowspawn commented 3 years ago

A couple of thoughts.

Regarding proposed API. I think there would be support at the Command level so a startup file can be read without a corresponding option. I'm not sure at the moment if also makes sense as an Option method, or whether handled by client using the Command support.

Support for subcommands requires some thought. I think of config files as being global normally. There is probably a low chance of reuse of option names between program and subcommand, but a higher chance of potentially conflicting reuse between subcommands. So for a full solution the config options may need to be qualified by sub-command path if they can be imported globally.

shadowspawn commented 3 years ago

To implement environment variable support with the desired balance of priority for default/env/cli, I have a PR which uses a new property to add and consult the "source" of the option value. I had config file in mind as another priority level when I was writing it. (Currently private property in the PR: #1587 )

if (defaultValue !== undefined) {
   this._setOptionValueWithSource(name, defaultValue, 'default');
}

// env is second lowest priority source, above default
if (this.getOptionValue(optionKey) === undefined || this._optionValueSources[optionKey] === 'default') {
   ...
}

shadowspawn commented 3 years ago

Related old issues: #151 #328

jakobrosenberg commented 3 years ago

To whom it may concern, this is how I currently manage configs.

const config = require('./path/to/config')

const useConfig = process.argv.find((arg) => ['--help', '-h', '-d', '--use-defaults'].includes(arg));
const defaults = useConfig ? config : {}

program
  .option('-i, --install-dir <dir>'), 'location', defaults.installDir
  .option('-d, --use-defaults'), 'silent install', false
  .action(async options => {
    /** iterate over options and prompt missing fields */
  })
  ...

As for manually specifying a config file, would it be possible to do something like

const softRequire = path => {try {return require(path)} catch(e){}}

const configPath = getSingleOption(process.argv)('-c --config') || 'mylib.config.js' //made up commander helper

const config = softRequire(configPath)}
const defaults = { /** options */}

const options = { ...defaults, ...config }

program
  .option('-c --config', 'config location')  // dummy option. Only included for `--help`
  .option('-i --install-dir', 'install location', options.installDir)
  ...

EDIT: updated bottom script

alexweininger commented 3 years ago

Would love to see this feature in commander. It's currently pretty complicated to implement defaults, a config file, and the ability to override the config file via command line args.

shadowspawn commented 3 years ago

Assuming config can use a dictionary (hash) of option config values, what keys should be supported? The mapping from dash-separated to dashSeparated is a programmer detail, so I am thinking at least the flags themselves should be supported since these are what end-users work with.

program.option('-p, --port-number <number>');

// possible config objects
{ '-p': 8080 }
{ '--port-number': 9000 }

For keys that do not start with a dash, Commander could perhaps look for a long option flag that matches (e.g. port-number), or look for the JavaScript style name (e.g. portNumber) for programmers configuring their own programs.

{ 'port-number': 8080 } // ????
{ 'portNumber': 9000 } // ????

alexweininger commented 3 years ago

I think it would be best to only allow the JavaScript style names of options for keys of the config file. This would mitigate conflicts, and users are familiar with JavaScript style property names in JSON.

I bet it would be possible for commander to generate a JSON Schema file for the config file.

Mingun commented 3 years ago

Yes, I support @alexweininger -- supporting only JavaScript style will be enough, at least in the beginning

jakobrosenberg commented 3 years ago

What would the equivalent of this be with a builtin config path option?

// softRequire is like require, but doesn't throw error on missing files
const softRequire = path => {try {return require(path)} catch(e){}}

// made up commander helper
const configPath = getSingleOption(process.argv)('-c --config') || 'mylib.config.js' 

const options = softRequire(configPath)} || {}

program
  .option('-c --config', 'config location')  // dummy option. Only included for `--help`
  .option('-i --install-dir', 'install location', options.installDir)
  ...

I'm a little on the fence about a native configs feature.

It adds complexity to the codebase
It adds more documentation
Config resolution becomes magic
Don't expand the API and documentation to solve something that could adequately be solved with the existing API and an example. It conceals the basics and edge cases that could have been solved with knowledge of the basics are either not possible or require further API expansions...
This sounds like something that could easily be solved with a plugin or tiny helper library.

My preferred solution would be a loadConfig helper which takes the option as the first parameter and a the default path as the second parameter.

const { loadConfig, program } = require('commander')

const options = loadConfig('-c --config', './my-default-config-path.js')

program
  .option('-c --config', 'config location')  // dummy option. Only included for `--help`
  .option('-i --install-dir', 'install location', options.installDir)
  ...

shadowspawn commented 3 years ago

What would the equivalent of this be with a builtin config path option?

Actually, I'm still thinking about lower-level details and don't have a builtin config path option in my head! So this code is a bit different, but where my thinking is up to:

program
  .option('-c --config <file>', 'config location')
  .option('-i --install-dir <file>', 'install location');
program.parse();

if (program.opts().config) {
  const configHash = require(program.opts().config);
  program.assignConfig(configHash); // does not override CLI values
}

console.log(program.opts().installDir); // may come from config or command-line

alexweininger commented 3 years ago

If there was an easier way to discern if an option has been passed in by the user or not, implementing a config file while using commander becomes a lot easier. I think solving this problem first might be more of a priority than to create a fully integrated config file feature.

Quoting from https://github.com/tj/commander.js/issues/1584#issue-970994630

but it is incomplete: if I specify --option-with-default "default value" in the command line the code above will think that it is default value and override it with the config option value, which is not expected. To solve this problem I'll have to parse options manually, which effectively makes commander useless in such scenario.

shadowspawn commented 3 years ago

Hopefully solved that problem internally while implementing .env(): https://github.com/tj/commander.js/issues/1584#issuecomment-904220002

jakobrosenberg commented 3 years ago

@shadowspawn I like your example, but how would it handle this scenario?

//mycli.js
const { loadConfig, program } = require('commander')

// specify default config
const env = process.env.NODE_ENV || 'development'
const defaultConfigPath = `./config/${env}.config.js`

// use user provided config or fallback to defaultConfigPath
const options = loadConfig('-c --config', defaultConfigPath )

program
  .option('-c --config', 'config location')  // dummy option. Only included for `--help`
  .option('-i --install-dir', 'install location', options.installDir)
  .option('-a --architecture <arch>', 'system architecture', 'hardcoded-value')
  ...

Expectations:

If I run mycli, it should correctly use the values specified in the config corresponding to the environment.
If I run mycli --help it should print the correct defaults that corresponds to the environment.
If I run mycli --help --config myconfig.js it should print the defaults that I provided in my custom config.

Note for anyone who thinks configs shouldn't be treated as defaults: Please consider that these are the default values that options will default to. Showing any other value would be misleading.

hildjj commented 3 years ago

Just so we're looking at the whole problem at once, here is a list of all of the places that config information might come from in a full-featured CLI, in order of increasing precedence. Everywhere I say FILE, i mean myCli.js, myCli.cjs, myCli.mjs, or myCli.json, and there might be a built-in mechanism to extend configs contained in other packages:

Built-in defaults, which might be system-dependent (e.g. the NODE_ENV example above)
$XDG_CONFIG_HOME/myCli/FILE no error on file not existing, error if not readable, can't be parsed, or invalid option
../FILE (etc. Keep going until root directory, or you find one). Maybe allow for a root = true like eslint. Files closer to pwd take precedence. Some use cases might not want the searching.
--config FILE on command line (this might turn off directory searching). Error if file does not exist.
$MYCLI_[var] environment variables
Command line

For each bit of config found, it needs to be carefully combined with the existing set, depending on type. Simple types like bool, string, and choices overwrite. Counts are specified as numbers in files, then overwrite. Variadic options get concatenated. Coerced options run through the (value, previous) => handler. There are probably a few other edge cases.

Note that a program might want to opt out of or into any of these (except the command line, I guess. :) ).

shadowspawn commented 3 years ago

https://github.com/tj/commander.js/issues/1584#issuecomment-919864200

I was not imagining an overlap between defaults and a configuration file in that way. Interesting though, and clear outline of scenario.

I think of the configuration file(s) being outside the help, with perhaps a way to view the details:

npm config list
git config --global --list

Are there some (well-known) programs or frameworks that behave the way you describe?

If I run mycli --help --config myconfig.js it should print the defaults that I provided in my custom config.

Two issues:

the help and version options are handled as soon as they are reached (left to right)
I imagine the config value being a different value than the default, and not appearing in the help

shadowspawn commented 3 years ago

https://github.com/tj/commander.js/issues/1584#issuecomment-920304453

Wide coverage, thanks.

here is a list of all of the places that config information might come from

Another custom place is a key of package.json.

Everywhere I say FILE, i mean myCli.js, myCli.cjs, myCli.mjs, or myCli.json

yaml format has come up in previous discussions (#151), probably outside familiar javascript tooling examples (like eslint). If Commander handles the file opening, there could be a custom parse routine.

Variadic options get concatenated.

I did see recently that mocha does this with options which can safely be repeated: https://mochajs.org/#merging

With environment variables (.env()) I had decided it was a simple model to have environment variables override default values, whatever the type of the option.

hildjj commented 3 years ago

+1 to package.json. I think it goes between 2 & 3.

+1 to yaml being possible. Allowing for custom parsers is better than taking a dependency to do yaml out of the box, imo.

I don't think that env variables should always change the help to show a different default. If you go out of your way like @jakobrosenberg did in his example, you can work around that somewhat. Also, in that example, if your config file is .js, you can do the NODE_ENV check in there and just have one file. I get that only solves that one particular case, but it's something to remember; we don't have to provide ultimate flexibility in places where the user can use JS to customize.

jakobrosenberg commented 3 years ago

One thing to keep in mind through all this is separation of responsibilities so we don't end up with a Frankenstein's monster or Homer's car.

There are numerous config resolvers that could easily be used with Commander. All you need is just the path of the config file, which could be solved with a simple helper that returns the value of a single argument, eg. getOption('-c --config').

Adding a small helper vs a full-blown config resolver would add

less complexity to the code base
less API surface + documentation
more flexibility
fewer potential dependencies
clearer separation of responsibility
easier to reason about code

alexweininger commented 3 years ago

Hopefully solved that problem internally while implementing .env(): #1584 (comment)

https://github.com/tj/commander.js/blob/950f742e60ae6b30a3d4e945db6d4322919628fd/lib/command.js#L573

https://github.com/tj/commander.js/blob/950f742e60ae6b30a3d4e945db6d4322919628fd/lib/command.js#L1460

If we could expose this so that we can consume this in the same way you did when implementing .env() I think that would solve the issues and allow people to implement config files on their own. I think this is better than providing some standard way to implement a config file through commander.

shadowspawn commented 3 years ago

Lots of good comments. Selected highlights:

@Mingun https://github.com/tj/commander.js/issues/1584#issue-970994630

Adding support of config files when you have an option with a default value and you want the following priorities, is complicated.

@alexweininger https://github.com/tj/commander.js/issues/1584#issuecomment-916597848

It's currently pretty complicated to implement defaults, a config file, and the ability to override the config file via command line args.

@alexweininger https://github.com/tj/commander.js/issues/1584#issuecomment-917535941

If there was an easier way to discern if an option has been passed in by the user or not, implementing a config file while using commander becomes a lot easier.

@hildjj https://github.com/tj/commander.js/issues/1584#issuecomment-920304453

For each bit of config found, it needs to be carefully combined with the existing set, depending on type.

@hildjj https://github.com/tj/commander.js/issues/1584#issuecomment-921061280

+1 to yaml being possible. Allowing for custom parsers is better than taking a dependency to do yaml out of the box, imo.

@jakobrosenberg https://github.com/tj/commander.js/issues/1584#issuecomment-921095033

There are numerous config resolvers that could easily be used with Commander.

The two things I am currently thinking about are:

low level: expose option value source (default/env/cli) for custom implementations
medium level: add routine to do a careful merge of config object into option values, taking into account priorities, and custom option processing

For now, I am thinking of leaving it up to user to locate desired implicit config file (or package.json property), and parsing config file. There are so many possibilities and patterns used by utilities, and I don't want to constrain user into particular pattern unless it will cover wide range of real world use. But it needs to be easy to put together.

alexweininger commented 3 years ago

@shadowspawn

The two things I am currently thinking about are:

low level: expose option value source (default/env/cli) for custom implementations

medium level: add routine to do a careful merge of config object into option values, taking into account priorities, and custom option processing

Both of these sound great to me. And I totally agree with:

leaving it up to user to locate desired implicit config file (or package.json property), and parsing config file

shadowspawn commented 3 years ago

@jakobrosenberg

My preferred solution would be a loadConfig helper which takes the option as the first parameter and a the default path as the second parameter.

// made up commander helper [combined from examples]
const options = loadConfig('-c --config', './my-default-config-path.js', process.argv)

(This is to find config file from arguments, before doing full parse and help.)

I see the attraction for you to allow using the config as defaults so they are displayed in help, as in your example.

The full parse does recursive processing of subcommands with a lot of complexity, both for processing valid syntax and for showing good error messages. My instinct is that while it may be possible to robustly extract an option value for a single program where you know the syntax and configuration you are using, it would be hard to do robustly for a general solution.

jakobrosenberg commented 3 years ago

@shadowspawn for Commander to imports configs, that would be my favorite, yes. But I would still prefer that Commander handles arguments parsing and leaves module importing to native tools like require/import or dedicated libraries.

const configPath = getOption('-c --config')
const config = require(configPath)

Having a getOption would improve the argument parsing capabilities of Commander and I can think of numerous scenarios that could benefit from it. For instance, I often use prompt or enquirer with Commander, but I always add a --silent option that lets me enable/disable defaults. With getOption you could do something like this:

// we want to include the defaults if we're doing a silent install or if we're printing the help page
const useDefaults = getOption('-s --silent') || getOption('-h --help')
const defaults = useDefaults ? require('./config.js') : {}

program
  .option('-i, --install-dir <dir>'), 'location', defaults.installDir
  .option('-s --silent', 'install using default values')
  .action( async options => {
    // if we didn't use `--install-dir` or `--silent`, installDir is undefined and we will prompt the user
    options.installDir = options.installDir || await promptInstallDir()
  })

You could even combine the above example with config import. That would allow us to obtain config values by CLI arguments -> user config -> program defaults (with silent option) -> prompt

example

```javascript // we want to include the defaults if we're doing a silent install or if we're printing the help page const useDefaults = getOption('-s --silent') || getOption('-h --help') const defaults = useDefaults ? require('./config.js') : {} // load config const configPath = getOption('-c --config') const config = require(configPath) Object.assign(defaults, config) program .option('-i, --install-dir '), 'location', defaults.installDir .option('-s --silent', 'install using default values') .action( async options => { // if we didn't use `--install-dir` or `--silent`, installDir is undefined and we will prompt the user options.installDir = options.installDir || await promptInstallDir() }) ```

shadowspawn commented 3 years ago

It was more complicated enforcing the syntax than I expected, but I have some proof of concept code working. Example usage:

const { program, Option } = require('commander');

program
  .option('-d, --debug')
  .option('-c, --color')
  .option('-C, --no-color')
  .option('-o, --optional [value]')
  .option('-r, --required <value>', 'string', 'default string')
  .option('-f, --float <number>', 'number', parseFloat)
  .option('-f', 'number', parseFloat)
  .option('-l, --list <values...>')
  .addOption(new Option('-p, --port <number>').env('PORT'))
  .option('--use-config')
program.parse();

if (program.opts().useConfig) {
  const config = {
    debug: true,
    color: true,        // same as CLI --color
    color: false,       // same as CLI --no-color
    '--no-color': true, // same as CLI --no-color
    '-o': true,         // same as CLI -o
    'optional': 'config string',
    'required': 'config string',
    'float': '3.4',
    'list': ['apple', 'pear'],
    'port': '443'
  };
  program.mergeConfig(config);
}

console.log(program.opts());

$ node merge.js                                
{ required: 'default string' }

$ node merge.js --use-config                   
{
  required: 'config string',
  useConfig: true,
  debug: true,
  color: false,
  optional: 'config string',
  float: 3.4,
  list: [ 'apple', 'pear' ],
  port: '443'
}

$ PORT=9000 node merge.js -r RRR --use-config -o OOO            
{
  required: 'RRR',
  useConfig: true,
  optional: 'OOO',
  port: '9000',
  debug: true,
  color: false,
  float: 3.4,
  list: [ 'apple', 'pear' ]
}

(Edit: fixed priority order of env and config)

shadowspawn commented 3 years ago

Is it best to display a merge error if a config key does not match a declared option?

Displaying an error is helpful for picking up mistakes early, but prevents directly using a flat configuration file across multiple subcommands with mixture of unique options (by ignoring unknowns).

Mingun commented 3 years ago

Is it best to display a merge error if a config key does not match a declared option?

I think, you can return the error from mergeConfig and allow the user to decide if he/she wants to display that error message.

'--no_color': true, // same as CLI --no-color
Wouldn't it be better to name key as --no-color to match the CLI option?

shadowspawn commented 3 years ago

--no_color was a typo, thanks. I'll fix that! (You can tell I have not implemented the error for unrecognised option key...)

shadowspawn commented 3 years ago

FYI @jakobrosenberg I was thinking about partial parsing again after seeing this argparse python code: https://stackoverflow.com/questions/3609852/which-is-the-best-way-to-allow-configuration-options-be-overridden-at-the-comman/5826167#5826167

And came up with something along the lines you were suggesting:

function getSingleOption(flags, name, args) {
  const findOne = new Command;
  findOne
    .allowUnknownOption()
    .helpOption(false)
    .option(flags);
  findOne.parse(args);
  return findOne.opts()[name];
}

console.log(getSingleOption('-c, --config <value>', 'config', process.argv));
console.log(getSingleOption('--use-config', 'useConfig', process.argv));

% node partial.js                                                         
undefined
undefined

% node partial.js --help --before --config foo --use-config --after --help
foo
true

shadowspawn commented 3 years ago

I was not sure my mergeConfig approach in #1607 was going to solve enough config related use cases, and switched focus to providing the low-level routines to allow clients to work with priorities and option sources: #1613

jakobrosenberg commented 3 years ago

@shadowspawn , sorry completely missed your previous message.

Love the example. - Though the second parameter (name) should probably be derived from flags.

function getSingleOption(flags, args) {
  // flagToCamelCase for lack of better words ^^
  const name = flagToCamelCase(flags) 
  ...
}

Regarding the PR, I can't fully wrap my head around it, but I assume it abstracts the commented logic.

/** resolve defaults, configs and env */
...

const defaults = { ...myDefaults, ...myConfig, ...myEnv }

program
  .option('--cheese [type]', 'cheese type', defaults.cheese)

CheshireSwift commented 3 years ago

I just wanted to voice support for a lighter touch approach, rather than Commander being responsible for locating/opening/parsing config files itself.

There's already fantastic tools that make this easy (e.g. cosmiconfig). If Commander exposes a way to detect an option's source (or even a way to merge an arbitrary object into the options), then those existing tools can be leveraged easily, while still leaving the door open to whatever more niche approaches a given project might need.

shadowspawn commented 3 years ago

Did a deep dive on config files, but for now just added .getOptionValueSource() and .setOptionValueWithSource() in Commander v8.3.0. Lighter touch approach for people rolling their own solutions!

jakobrosenberg commented 3 years ago

@shadowspawn Sorry for resurrecting this, but I have a question about how to accomplish the following. Is it possible to get the value of install <dir> before running the program?


// pseudo function that grabs the dir value
const installDir = getValue(['install', 'dir'])

// if app is already installed, use existing config as defaults
const defaults = require(installDir + '/config.js') || getDefaults()

program
  .command('install <dir>')
  .option('--secure', 'set to secure', defaults.secure)
  .action((dir, options) => { ... })

shadowspawn commented 3 years ago

I don't see an easy way to get a subcommand argument without parsing.

A different approach might be to read the local configuration in a .hook('preAction', ...).

shadowspawn commented 7 months ago

A couple more related approaches I saw recently:

1) @jpilgrim published cmdsettings.

Mentioned in: https://github.com/tj/commander.js/issues/1748#issuecomment-2095631625

2) Mocha has a PR to add MOCHA_OPTIONS which can contain a space-separated list of command-line options, inspired by NODE_OPTIONS.

https://github.com/mochajs/mocha/pull/4835#issuecomment-2082835625

tj / commander.js

Add support for config files #1584

Problem

Proposal

`.config.js` does not exist

`.config.js` contains `option`

`.config.js` contains `default`

tj / commander.js

Add support for config files #1584

Problem

Proposal

.config.js does not exist

.config.js contains option

.config.js contains default

`.config.js` does not exist

`.config.js` contains `option`

`.config.js` contains `default`