Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using an array as an object key #9

Closed
nhz-io opened this issue Oct 18, 2022 · 8 comments
Closed

Using an array as an object key #9

nhz-io opened this issue Oct 18, 2022 · 8 comments
Labels
bug Something isn't working help wanted pull requests welcome

Comments

@nhz-io
Copy link
Contributor

nhz-io commented Oct 18, 2022

Consider this block:

minimist/index.js

Lines 52 to 66 in 62fde7d

Object.keys(opts.alias || {}).forEach(function (key) {
aliases[key] = [].concat(opts.alias[key]);
aliases[key].forEach(function (x) {
aliases[x] = [key].concat(aliases[key].filter(function (y) {
return x !== y;
}));
});
});
[].concat(opts.string).filter(Boolean).forEach(function (key) {
flags.strings[key] = true;
if (aliases[key]) {
flags.strings[aliases[key]] = true;
}
});

Here it builds an array of aliased args:

minimist/index.js

Lines 55 to 57 in 62fde7d

aliases[x] = [key].concat(aliases[key].filter(function (y) {
return x !== y;
}));

And here it uses this array as a key for strings:

flags.strings[aliases[key]] = true;

Which will just result in:

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

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

@nhz-io nhz-io changed the title Using an array as object key Using an array as an object key Oct 18, 2022
@ljharb
Copy link
Member

ljharb commented Oct 18, 2022

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

@nhz-io
Copy link
Contributor Author

nhz-io commented Oct 18, 2022

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
Copy link
Member

ljharb commented Oct 18, 2022

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

@nhz-io
Copy link
Contributor Author

nhz-io commented Oct 18, 2022

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
Copy link
Contributor Author

nhz-io commented Oct 18, 2022

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
Copy link
Member

ljharb commented Oct 18, 2022

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.

@ljharb ljharb added help wanted pull requests welcome bug Something isn't working labels Oct 18, 2022
@shadowspawn
Copy link
Collaborator

shadowspawn commented Oct 18, 2022

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 added a commit to nhz-io/node-minimist that referenced this issue Oct 18, 2022
nhz-io added a commit to nhz-io/node-minimist that referenced this issue Oct 18, 2022
@shadowspawn shadowspawn mentioned this issue Oct 18, 2022
@nhz-io
Copy link
Contributor Author

nhz-io commented Oct 18, 2022

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

Well, aliases cannot be flat without breaking this:

minimist/index.js

Lines 125 to 127 in 62fde7d

(aliases[key] || []).forEach(function (x) {
setKey(argv, x.split('.'), value);
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted pull requests welcome
Projects
None yet
Development

No branches or pull requests

3 participants