feat(laconia-config): Parse integers and floats from LACONIA_CONFIG_ env variables

[geoffdutton]

This adds an integer converter and float converter when parking LACONIA_CONFIG_ environment variables (#542).

• Add LACONIA_CONFIG_PORT: integer:8080 to env to be interpreted by laconia-config as parseInt(8080);
• Add LACONIA_CONFIG_TAX: float:80.80 to env to be interpreted by laconia-config as parseFloat(80.08);
• Update documentation and examples with new features laconiajs/website/issues/8

Note: There was a .forEach with an async callback in the acceptance test that I changed to a for..of loop in order to properly await the promises.

[hugosenari]

Anything is ok, thanks 👍

Only missed tests for NaN, to makes it clear what we should expect when string isn't float/integer.

[geoffdutton]

Yeah I was thinking how we handle that. Should we throw an error if either is NaN, or just return NaN?

[hugosenari]

Yeah I was thinking how we handle that. Should we throw an error if either is NaN, or just return NaN?

NaN is good enough 😉

[ceilfors]

I kind of prefer throwing, over returning NaN. As a user, I'll discover my configuration error faster if Laconia is throwing that error for me, instead of me discovering that I'm passing NaN into our app code. I take the error as a configuration issue as, if I configure float:not-float-string, that's my fault as a user, and I'd like to know that. Throwing error is also the behaviour that we currently have under SsmConfigConverter.js, so consistency is a plus.

Thoughts?

[geoffdutton]

Ah yeah if we're throwing in another one with an unexpected value, I'd say we throw here too, or at the very least return null or something so you could do something like const dbPort = env.dbPort || 3306.
@hugosenari What do you think about throwing an error if the value is NaN?

[hugosenari]

Some days ago I was thinking that if null invention was billion dollar.

JavaScript has trillion dollars mistake (null, undefined and NaN) :-)

NaN: user could write defensive code;
Exception: fail fast.

Both ok, stay with exception for consistency.

[geoffdutton]

Sounds good. One more case I thought of, what should happen if I pass integer:1.23?

[hugosenari]

parseInt ignores:

• parseInt('1.2.2.000000.222,2222', 10) === 1
• parseInt('1avocado', 10) === 1

[geoffdutton]

I updated the tests for NaN to throw an error if the value is not a valid integer or float.

We could probably combine validateAndParseInt and validateAndParseFloat to a more generic function, but I figured it'd be best to be explicit that the value is not a float or not a integer depending on the config key.

[ceilfors]

parseInt('1avocado', 10) === 1 🤯

Super optional, but maybe it's worth adding a unit test to show this behaviour (Or maybe I'm just not used to it), or document that we're just using parseInt so we'll be able to parse 1avocado

[geoffdutton]

Ha yeah I was thinking about that too when mixing numbers and letters/other chars. We could do something like:

if (inputVal !== inputVal.replace(/\D/g,"")) {
  throw Error("passed string contains invalid characters");
}

Otherwise I'd say just call it out in the docs. Any preference?

[hugosenari]

source

Maybe just comment in the docs 😅

[ceilfors]

@geoffdutton It seems like @hugosenari is genuinely waiting for this one, haha. I prefer to throw an error as you have made it look super simple to implement, but I'm happy to merge and release to make it available with this one! Let me know!

[geoffdutton]

I implemented those checks as well, but I'll still call it out int he docs. Should be good to go.

[hugosenari]

Current regex fail for negative inputs?

Any reason to not use match instead of replace?

if (isNaN(parsedVal) || val.macth(floatRegex))

edit: found this related next JS iteration has _ as Numeric Separator

[geoffdutton]

Good call on the negative numbers and using .match, I've updated to address those.

I think for now we ignore the _ as a Numeric Separator since:

parseInt("99_99", 10 ) === 99;
parseFloat("80_8") === 80;

I will update the documentation to reflect when errors are thrown now. Do one of you want to merge the PR per the "don't merge your own PRs" guideline?