Custom parsers are typed to receive strings, but spec requires default values to match the type

[novemberborn]

const hex = envalid.makeValidator<Buffer>(value => Buffer.from(value, 'hex'))

envalid.cleanEnv({
  VALUE: 'decafbad',
}, {
  VALUE: hex({ default: 'ff' }), // TypeScript expects a Buffer or undefined
})

The parser is typed as receiving a string value:

envalid/src/envalid.d.ts

Line 116 in ad2f612

parser: (input: string) => any,

However the validator specs infer the validated type and mandate it for the default and devDefault values. That's nice for numbers and booleans but I suspect it'll trip up the json validator. It works for my hex validator above but only because Node.js doesn't throw an error for the unnecessary encoding argument.

Perhaps the default values should always be strings, and be typed as such?

[af]

This is a good point, I don't think the type of default/devDefault was ever formally specified.

Looking at the implementation, rawValue is assigned to the default/devDefault values so it should be safe to type them as strings. However it might be a bit counterintuitive that choices is typed as T instead– it doesn't look like that can be changed.

I'll try and get to this change in the next week unless anyone raises a reasonable objection. PRs welcome in the meantime

[lostfictions]

Huh, the behaviour of default/devDefault is actually kind of surprising to me here. I would have assumed that the defaults are already validated values, not ones that are substituted for rawValue and passed into the validator.

After all, defaults are (I hope) known values -- they shouldn't require additional validation, so if rawVar is undefined but there's a default I'd have expected to early out and just return it. That aligns with the notion of choices, which are known values to compare to the result.

So in @novemberborn's case I'd suggest the typings aren't wrong, but that this should work:

envalid.cleanEnv({
  VALUE: 'decafbad',
}, {
  VALUE: hex({ default: Buffer.from('ff', 'hex') })
})

Playing around in the node repl, it looks like it actually does, but only because Buffer.from(Buffer.from('ff', 'hex'), 'hex') happens to work.

Personally I'd be in favour of directly returning a default value if it's available and rawValue is undefined; to me that would be the less surprising behaviour. (Technically this would comprise a breaking change in behaviour, but it's very likely there's other folks like me who have already been using defaults in this way, especially since that's how the typings work!)

We could even add an overload to allow default/devDefault to also be a value-returning function (ie. { default: () => Buffer.from('ff', 'hex') }) for lazy initialization if it's expensive or side-effectful (like building a buffer or reading a file). What do folks think?

[novemberborn]

What's nice about specifying strings is that it's easier to see how to use those values in .env files or Kubernetes deployments or whatever.

For example I have a validator built on top of ms and it's nicer to be able to write '2h' rather than 7_200_000.

[lostfictions]

In your particular case with ms, couldn't you just write { default: ms('2h') }?

I thought about it some more and I do think you have a good point, though -- Envalid is meant to work on env vars; that is, it's mostly operating on string inputs anyway. And what I said before about validated values might be backwards -- choices might actually be the odd one out for how it works on (potentially transformed) validated values. (choices only makes sense for string or number anyway, since any non-primitive -- say, a Buffer -- is not going to be found by choices.includes(value).)

Unfortunately, the other side of the coin is that for non-string primitive validators (boolean, number) it's a bit more awkward and brittle to define default/devDefault/choices as strings. That is, it's maybe surprising to require users to type something like this:

{ LAUNCH_STAGE: num({ devDefault: "1", choices: ["1", "12", "36"] })

or: { IS_READY: bool({ devDefault: "true" })

rather than just letting them type the literals true or 36, as it currently allowed. This does mean there's technically a mistake in the typings, though -- when a user writes bool({ devDefault: true }) with the literal, that literal gets passed into the parser verbatim even though it's supposed to only take strings.

[lostfictions]

So going forward, how about this?

1. For any Spec<T>, devDefault and default both have type string | T. At runtime, during validation, if we don't have a rawValue and typeof default === 'string' (or devDefault ofc) we pass that default through the validator (same as current behaviour). However, if the default is not a string, we assume it's an already-validated/transformed value and return it directly.

2. Deprecate Spec.choices in favour of a new helper, choices, which takes string or number arguments and returns a validator, eg.

   cleanEnv(
       process.env,
       {
           RATING: choices('good', 'bad', 'ok')({
               default: 'good'
           }),
           STARS: choices(1, 2, 3, 4, 5)()
       }
   )

   This simplifies validator logic, fixes the case where validators that return anything other than string or number don't work with choices, and the return value of the validator can be typed to exactly the values provided (which would accomplish what @SimenB wanted to achieve in feat: add support for generics in built-in validators #113 but without the footguns).

[novemberborn]

@lostfictions that sounds great, I like it!

[StevenLangbroek]

Hey folks! Just ran into this issue, anything I can do to help implement this?

[StevenLangbroek]

Is it maybe a simpler change to have defaults always be of type string, and parse them like any values actually read from env?