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

"Automagic" conversion of number-like strings is unexpected and frustrating #963

Closed
ezzatron opened this issue Sep 27, 2017 · 16 comments
Closed

Comments

@ezzatron
Copy link

I'm building a CLI with lots of options, spread across lots of nested commands. I recently noticed (by pure luck) that an option that was accepts a phone number was being "automagically" converted to a number and losing the leading zero. After some digging, the cause is this feature:

Numbers

Every argument that looks like a number (!isNaN(Number(arg))) is converted to
one. This way you can just net.createConnection(argv.port) and you can add
numbers out of argv with + without having that mean concatenation,
which is super frustrating.

I think this feature is more detrimental than helpful. Now I have to worry about any option input that looks numeric getting coerced when it shouldn't be.

In order to mitigate the issue entirely for my use-case, I'd need to apply my own coercion function to every option in my entire interface! Wouldn't a better solution be to provide some built-in coercion functions that can be applied to options that desire this behavior on a case-by-case basis?

Another solution to this problem might be to provide an option to turn automatic coercion on or off. Even then, this behavior should probably be opt-in rather than opt-out, to avoid violating the principle of least astonishment.

@ezzatron
Copy link
Author

It also seems to be completely impossible to stop this happening for arguments, even when using a custom coercion function.

@Morishiri
Copy link
Contributor

Morishiri commented Sep 27, 2017

Hey,

actually there is really simple way to deal with it. Having script like this:

const args = yargs
    .option('number-option', {
        description: 'b',
        number: true
    })
    .option('string-option', {
        string: true
    })
    .argv;

console.log(args);

look at the option called string-option. By assigning it string: true property you are stopping yargs from parsing it like a number.

Output from

$ node index.js --string-option=15 --number-option=12

is

{ _: [],
  help: false,
  version: false,
  'string-option': '15',
  stringOption: '15',
  'number-option': 12,
  numberOption: 12,
  '$0': 'index.js' }

so as you can see 15 is string, while 12 was parsed as a number.

Similiar issue: #938

@ezzatron
Copy link
Author

Thank you! That will make the problem easier to deal with.

Do you know the same is possible for regular (non-option) arguments? The automatic number parsing seems to happen there too.

@nexdrew
Copy link
Member

nexdrew commented Sep 27, 2017

Note that you can also disable the feature entirely via yargs config in your package.json. See the parse-numbers option here: https://github.com/yargs/yargs/blob/master/docs/advanced.md#customizing

@bcoe bcoe added the triaged label Sep 27, 2017
@bcoe
Copy link
Member

bcoe commented Sep 27, 2017

@ezzatron worth noting I'm doing some work on yargs' parser's handling around number conversion, since it breaks some aspects of bringing command parsing closer to option parsing.

@ezzatron
Copy link
Author

@nexdrew That's exactly what I was looking for, thanks!

I might submit a PR to add that information to the "Tricks - Numbers" section of the docs, which is where it would have been handy to find that information.

@bcoe Thanks for the heads up :)

@bcoe
Copy link
Member

bcoe commented Sep 27, 2017

@ezzatron patches to docs very much appreciated, also if you have any thoughts about how to make this feature less shocking; I had on my mind even that I might disable this feature by default in a major release ... I was a little concerned this might break too many programs though.

Word of warning, I found some bugs with turning off number parsing entirely, which I'm in the process of fixing on a branch.

@ezzatron
Copy link
Author

My personal preference would be for it to be disabled by default, and addressed in a major release like you suggested. Less magic is good, and major releases are for breaking things.

With a good change log and/or migration guide, I don't think people will get upset. Especially if all they have to do to get the old behavior back is explicitly set parse-numbers to true in their top-level configuration.

@gregorskii
Copy link

Hi all,

I found I had to remove yargs from subprocess I was using because it appears that the auto-coercion appears to run automatically even when the value is defined as a string in the options. I was passing twitter reply to ID's which are extremely large integers. When using argv for this purpose it was incrementing the value by one for an odd reason.

In the source code is the coercion running even when the type is string? Does it automatically convert it to an int then back when its set as string?

@bcoe
Copy link
Member

bcoe commented Oct 3, 2017

@gregorskii I believe there's a bug in the coercion that does apply this logic unfortunately, I will work on getting my fixes landed soon to this behavior.

@bcoe bcoe added the bug label Oct 3, 2017
@gregorskii
Copy link

gregorskii commented Oct 3, 2017

Awesome, just switched back to regular args for this project, and it’s a pretty weird use case. But good to know a fix is coming. Thanks!

@bcoe
Copy link
Member

bcoe commented Oct 4, 2017

@gregorskii @ezzatron see yargs/yargs-parser#104, which I think is a step in the right direction.

@ogonkov
Copy link

ogonkov commented Dec 11, 2017

Webpack broke my bundle, because of this bug

@bcoe bcoe closed this as completed Jan 1, 2018
@bcoe
Copy link
Member

bcoe commented Jan 1, 2018

@ogonkov @ezzatron @Morishiri there are now a few upstream fixes in yargs-parser which address this issue:

yargs/yargs-parser#110

We don't attempt automatic conversion to numbers if the conversion would result in losing information. I will have a new version of yargs out the door today.

@eugenet8k
Copy link

This issue hit us hard when we use a default config of yargs. Please consider this scenario

./my-script.js --app-version=4.80

where 4.80 is a Git tag or branch name

When I access the arg in my-script.js:

const {argv} = require('yargs')
console.log("My version is: ", argv['app-version'])

I get:

> My version is: 4.8

And we did not notice this issue in our CI for months with our release schedule 4.71, 4.72, 4.73 ... until we hit 4.80 today.

@godspeedelbow
Copy link

@eugenet8k Opened issue for this #1758

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

No branches or pull requests

8 participants