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

.pkgConf(), .normalize() and .array() not working well together #37

Closed
Josan-Coba opened this issue Jul 4, 2016 · 2 comments · Fixed by #45
Closed

.pkgConf(), .normalize() and .array() not working well together #37

Josan-Coba opened this issue Jul 4, 2016 · 2 comments · Fixed by #45
Labels

Comments

@Josan-Coba
Copy link

(Originally notified in yargs/yargs#536)

const argv = require('yargs')
      .pkgConf('opts')
      .array('a')
      .normalize('a')
      .argv;

The above code paired with a package.json containing

"opts": {
  "a": ["bin/../a.txt", "bin/../b.txt"]
}

crashes throwing TypeError: Path must be a string. Received [ 'bin/../a.txt', 'bin/../b.txt' ].

@Josan-Coba
Copy link
Author

Josan-Coba commented Jul 4, 2016

After @bcoe's answer in the previous issue I've run a couple of additional experiments and found that if the options are passed through cli instead of .pkgConf() then .normalize() works pretty much as expected even being an array:

$ ./test.js -a "bin/../a.txt" "bin/../b.txt"
[ 'a.txt', 'b.txt' ]

I've searched through the code looking for some possible causes. The error pointed to line 344, in the function setArg():

// Set normalized value when key is in 'normalize' and in 'arrays'
if (checkAllAliases(key, flags.normalize) && checkAllAliases(key, flags.arrays)) {
  value = path.normalize(val)
}

I haven't thoroughly examined the source and I'm not familiar with the whole parsing process, but a quick peek tells that when a config object is used, the function setConfigObject() passes the whole object (in this case, the array in package.json) directly to setArg(), and hence path.normalize crashes.
On the other case, the options are processed by eatArray(), who, instead of passing around the whole array after being parsed, calls setArg() once for every value in the array, and so path.normalize receives a string and happily deals with it.

I guess possible fixes would be for setArg() to check if it's dealing with an array:

// Set normalized value when key is in 'normalize' and in 'arrays'
if (checkAllAliases(key, flags.normalize) && checkAllAliases(key, flags.arrays)) {
  if (Array.isArray(val)) value = val.map(path.normalize)
  else value = path.normalize(val)
}

Or forcing setConfigObject() to behave like eatArray(), passing every element of a given array one by one to setArg().

@Josan-Coba
Copy link
Author

Ok, a bit of issue-digging later it looks like .normalize() works with arrays because it was already adressed in yargs/yargs#430.
So, am I right if I think of this as a bug instead of an enhancement?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants