refactor!: restructure configuration to take options bag

[aaronccasanova]

Per the discussion in #45 this PR restructures the current options API where each option is configured in three separate list and instead allows options to be configured in a single object.

The goal being to make the API more intuitive for configuring options (e.g. short, withValue, and multiples) while creating a path forward for introducing more configuration options in the future (e.g. default).

Before:

parseArgs(process.argv.slice(2), {
  withValue: ['foo'],
  multiples: ['foo'],
  shorts: {
    f: 'foo',
  },
})

After:

parseArgs({
  args: process.argv.slice(2),
  options: {
    foo: {
      short: 'f',
      type: 'string', // defaults to 'boolean'
      multiple: true,
      // default: 'bar' - Future iteration
    }
  },
})

While the above mentioned discussion left off with a tentative interest in moving forward with the suggestion, I wanted to at least make an official PR with the proposed changes and have an implementation we could reference.

Note: All tests are passing and I've introduced a few new validation helpers to check the new structure

TODO: Assuming we decide to move forward with this approach

• [x] Update README examples

[bcoe]

I'm at least partially sold on this approach, especially if it would help us reach consensus on some other features .. such as my thinking that we could figure out a better API surface for strict if we configure as an options bag.

[bcoe]

@aaronccasanova I'd be interested in seeing the updated README if you have the time.

One open question for me, if an argument is a flag, would you just set withValue: false? is this confusing (@ljharb, @shadowspawn). I like that we could support strict with this approach, without adding an additional configuration -- I just wonder if withValue is the right variable name.

[ljharb]

It is a bit confusing; I’d prefer an enum of “value” or “flag” if that were possible.

[aaronccasanova]

@aaronccasanova I'd be interested in seeing the updated README if you have the time.

Will do 👍

if an argument is a flag, would you just set withValue: false? is this confusing

I agree this is a bit confusing. I was assuming users would omit withValue and leverage the zero config behavior where options without withValue are flags.

One alternative solution is to replace withValue with type (e.g. type: string | number | boolean). Not only does this allow users to explicitly define the option type, but provides more meaningful metadata to the parser. For example, If option was set to type: 'number' the parser could confidently capture negative numbers (-100) instead of expanding them as a short options group (-1 -0 -0) as mentioned here.

[bcoe]

One alternative solution is to replace withValue with type (e.g. type: string | number | boolean)

I don't hate that idea, but for MVP think starting with string and boolean would be best, in the future we could decide whether we should add additional types, or leave this to implementers.

[aaronccasanova]

for MVP think starting with string and boolean would be best

Agreed, I went ahead and replaced the withValue option with type, added validation checks for string|boolean, and updated the tests: https://github.com/pkgjs/parseargs/pull/63/commits/082bec5a5a667ee4d1d7725e4b8fcfa2199d0b4c

[aaronccasanova]

@bcoe Updated the README to reflect the most recent implementation (with the type option): https://github.com/pkgjs/parseargs/pull/63/commits/09090081a42c5eea90a338e6cd1a2d2bfe1ec978

[bnb]

Added the Merge into Node.js milestone, not because this needs to be merged by then but because IMO we should definitely figure this out - merge or close - before that IMO.

[isaacs]

I like this a lot. It also leaves the door open for things like description, options, conflictsWith, validate: () => Boolean, etc. And it's much more declarative/readable.

[bcoe]

I will make an effort to review this tomorrow ❤️

[bcoe]

Proposing we land this early in the week, if we don't see any arguments against here or in #45.

It feels like doing so would sure up some important remaining work, and could actually put us on track for being in Node 18.

[shadowspawn]

Non-blocking suggestion: property could be args rather than argv?

const result = parseArgs({ args: process.mainArgs, strict:false });

[ljharb]

but process.argv is a thing, and process.mainArgs isn't?

[shadowspawn]

Yes, I did consider real process.argv vs proposed process.mainArgs. (And wondering about asking where mainArgs was headed!) But that wasn't the only reason I suggested it.

Factors in favour of args:

1) Part of the point of parseArgs is to get away from the arcane conventions of argv.

2) If I have some custom arguments to parse in my own code, I would rather call them args than argv.

const result = parseArgs({ args: myArgs, strict:false });

const args = getTestArgs();
const result = parseArgs({ args, strict:false });

3) The routine is called parseArgs not parseArgv

4) If I did want to parse process.argv, I don't need to specify a property at all. (Edit: oh yes I do, because I need to call slice! See (1).)

[ljharb]

I’m not sure how arcane it is; it’s still the terminology in the environment the arguments are coming from :-)

[shadowspawn]

Yeah, nah, I am still liking args. Some scenarios.

1) I want the arguments from the Node.js environment my application was launched with.

a) leave it up to parseArgs (normal)

const result = parseArgs({ strict:false });

b) specify it myself (perhaps after preprocessing, say). Examples with both naming.

const result = parseArgs({ argv: process.argv, strict:false }); // [sic] wrong, argv is not argv
const result = parseArgs({ args: process.argv.slice(2), strict:false }); // get the args from argv

2) I have my own arguments.

const args = getTestArgs();
const result = parseArgs({ args, strict:false });

[shadowspawn]

Note that @aaronccasanova made exactly the argv: process.argv mistake in his description of usage! (Sorry Aaron, but useful example. 😄 )

https://github.com/pkgjs/parseargs/pull/63#issue-1127954190

[bakkot]

The thing named argv in almost every language includes the name of the program itself (except for some reason in Ruby). My understanding is that this doesn't.

Since process.argv is not the intended/default value, +1 to not naming this argv.

[aaronccasanova]

Note that @aaronccasanova made exactly the argv: process.argv mistake in his description of usage! (Sorry Aaron, but useful example. 😄)

Lol no worries! I actually intentionally used process.argv instead of process.argv.slice(2) to keep the example concise and focused on the options API. In regards to args vs argv, my initial implementation used args and I updated it to argv to reduce the number of changes introduced in this PR. Although, I also prefer args for two reasons:

• The default/input args will likely always be processed and not representative of the standard argv shape
• The utility is called parseArgs and thus args is a more intuitive name for the inputs (imo)

[shadowspawn]

Weak suggestion: multiple rather than multiples. (The plural in the original was because it was an array of options.)

[shadowspawn]

Light comment: in terms of shim correctness, I am guessing the new validation helpers need to be outside validators.js which is a Node.js internal file?

Non-blocking. Interested in whether this is the case, but can be remedied in a separate PR.

[shadowspawn]

I'll go through and edit in some non-blocking explanations in some of my suggestions, as most do not affect client, are an existing shortcoming that can be remedied in a small PR, etc.

Edit: Aaron on top of things before I made edits, might not get to that!

I am keen on changing argv to args as it is widespread and client facing, and have submitted a PR for that.

Edit: landed

[aaronccasanova]

Thanks everyone for the feedback and helping improve the robustness of the implementation!! I think the PR is in a really good spot and has laid a great foundation for future iterations. Here are some notable changes that have recently been introduced since the initial reviews:

• The argv parser config has been renamed to args
• The multiples option config has been renamed to multiple
• Internal usages of the name option have been updated to longOption and shortOption respectively
• Added validation to ensure a provided short option config is a single character (and test case)