minimistjs / minimist

parse argument options
MIT License
530 stars 30 forks source link

Using an array as an object key #9

Closed nhz-io closed 1 year ago

nhz-io commented 1 year ago

Consider this block:

https://github.com/minimistjs/minimist/blob/62fde7d935f83417fb046741531a9e2346a36976/index.js#L52-L66

Here it builds an array of aliased args: https://github.com/minimistjs/minimist/blob/62fde7d935f83417fb046741531a9e2346a36976/index.js#L55-L57

And here it uses this array as a key for strings: https://github.com/minimistjs/minimist/blob/62fde7d935f83417fb046741531a9e2346a36976/index.js#L64

Which will just result in:

{
  strings: {
    "[object Object]": true
  }
}

It should either build flat aliases or iterate over the nested array

ljharb commented 1 year ago

Is there a test case you can suggest that indicates the problem?

nhz-io commented 1 year ago

Actually, the array will be turned into string joined with comma. But regardless, the logic still seems broken to me. I will update later if i find a use case where this breaks the functionalilty.

ljharb commented 1 year ago

In which browser is an array used as an object key not toStringed such that it auto-joins?

nhz-io commented 1 year ago

In which browser is an array used as an object key not toStringed such that it auto-joins?

My mistake. It something i messed up it seems

nhz-io commented 1 year ago

Found the case where it breaks.

minimist = require('minimist')

console.log(minimist(["-f123"], {
    alias: {
        foo: ["f"]
    },
    string: ["foo"]
}))

/* Outputs:
  { _: [], f: '123', foo: '123' }
  Works as intended because flags.strings key will be: {"foo": true, "f": true}
  */

console.log(minimist(["-f123"], {
    alias: {
        foo: ["f", "g"]
    },
    string: ["foo"]
}))
/* Outputs:
  { _: [], f: 123, foo: 123, g: 123 }
  Broken because flags.strings will be: {"foo": true, "f,g": true}
*/

Its an edge-case, but its still breaks the convention

ljharb commented 1 year ago

The readme indeed says "an object mapping string names to strings or arrays of string argument names to use as aliases", so this seems like a bug.

shadowspawn commented 1 year ago

Took me a while to make sense of it, but I agree it is a bug. An alias with multiple items does not play well with the opts.string configuration.

(I suspect there are some holes in the combination of aliases with opts.boolean too, but aliasIsBoolean() means the edge cases are quite different. Separate issue.)

nhz-io commented 1 year ago

It should either build flat aliases or iterate over the nested array

Well, aliases cannot be flat without breaking this: https://github.com/minimistjs/minimist/blob/62fde7d935f83417fb046741531a9e2346a36976/index.js#L125-L127