FIX string parsing issue of number format

[furkankose]

While I was using convict in a project, I realized that number format validation does not work properly. When you set a string value (other than the values that can be parse to number) to a number field, convict should throw a validation error but it does not. That's why, I added an isNumber function to fix this issue.

By the way, I am not totally sure about the scope of number format. That's why I estimated NaN and Infinity values as truthy values because of their data types. If NaN and Infinity values are not valid options for number format, I can update the function by taking that into consideration.

You can reproduce the issue by using the code below

const convict = require('convict')

const config = convict({
  number: {
    format: Number,
    default: 53,
  },
})

config.load({number: 'convict'})
config.validate({strict: true})

console.log('number: ', config.get('number')) // number: NaN

[coveralls]

Coverage decreased (-1.8%) to 91.599% when pulling fb7711dbaac5bc5aa7f8783b2733f342310f88ad on furkankose:master into e3770a6768d7832ed77d3609071e3226fd3288e8 on mozilla:master.

[A-312]

Be careful the behavior will change in the next version of convict

[A-312]

Madarche works on my PR, my code was like that https://github.com/A-312/node-blueconfig/blob/dc67c0de4dccbc2d04039f9a43f5cb91d08de340/packages/blueconfig/lib/index.js#L475

[furkankose]

Be careful the behavior will change in the next version of convict

Oops, I was not aware of this. Should I close the PR in this situation?

[A-312]

@madarche Maybe you should keep a PR open "Convict next version".

[kon14]

Any updates on this?

[BenceSzalai]

Shouldn't the same changes be applied to int type as well? parseInt is also susceptible of returning NaN for random strings.

case 'int': v = parseInt(v, 10); break`

[madarche]

Yes, both Number and int are affected by the same problem.

Here are the definitions of what the int and nat formats are expected to be and those definitions have no problems: https://github.com/mozilla/node-convict/blob/master/packages/convict/src/main.js#L56-L61

So a fix should try to leverage those definitions instead of adding a new way, a new function, to validate what a number is.

Also a fix would really be warmly welcomed if it could contain a matching test and not lower the test coverage.

Thank you

[BenceSzalai]

Ah, thanks for the response. I think i can look into this, doesn't seem to be a huge thing.

It is off-topic, but don't know a better place to ask: generally speaking I find this repo a bit confusing, as there are issues and open PRs with little activity for years suggesting it may be abandoned, also mentions of "next version" / v7 further discouraging involvement, and then I got this unexpectedly quick response. So not sure what should I think of this project. Is there a status page/roadmap or similar to educate myself before engaging with the project?

[madarche]

You have said it all @BenceSzalai: very little activity almost abandoned, but because the project is stable, used and useful, it's being maintained to a minimum to keep the applications running and the users safe. Also small additions that improve this package without making it more complex are welcome.