-
Notifications
You must be signed in to change notification settings - Fork 65
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
Enhanced typings with optional environment variables #194
Conversation
af877e1
to
1443777
Compare
This comment was marked as resolved.
This comment was marked as resolved.
a0f5e31
to
1c32939
Compare
@af should be good! |
Thanks for this and sorry for the delay in looking at it! Hoping to review and test it out this weekend 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some initial feedback, mostly about the exposed API and docs.
First of all, the JSON inference is super nice! And I really appreciate that you tidied up a lot of the kludges I still had in place from migrating this thing to TS.
Some of the types are pretty complex so I'm still digesting them. Overall looking very good though!
README.md
Outdated
|
||
- `makeBaseValidator<BaseT>` when you want the output to be narrowed-down to a subtype of `BaseT` (e.g. `str`). | ||
- `makeExactValidator<T>` when you want the output to be widened to `T` (e.g. `bool`). | ||
- `makeMarkupValidator` for input which can produce arbitrary output types (e.g. `json`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if it makes sense to expose makeMarkupValidator
in the public API. What other use cases can you think of for it? If we keep it private for now we can always expose it later. Given the domain of "parsing env var strings" I think json() would be the dominant case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking query parameters, e.g. "option1=true&option2=false&token=XXX"
; or POSIX like flags e.g. -b -c --long-flag=/bin/xxx
, /etc/fstab
, XML, YAML... JSON5; honestly I don't know much about backend developers practices , but I had imagined exposing this one would give plenty of flexibility for consumers of this library, freeing them from the hassle of typings these validators correctly.
PS1: didn't test but I think we should also forbid specs with choices in markup validators, correct?
PS2: I went back and forth between naming this validator markupValidator
and structuredValidator
; English is not my native language and if you have better naming ideas I'm all for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think structuredValidator
would be a better name for sure, when most people hear "markup" they probably think of a specific type of format like HTML/XML/etc.
Also re choices
, a nice side effect of removing structuredValidator
from the public API is we don't have to worry about that interaction :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also re choices, a nice side effect of removing structuredValidator from the public API is we don't have to worry about that interaction :)
Well, with one important exception: json
validator. In any case, I did implement this specific logic here:
328e056
Yet, I'm happy to not export makeStructuredValidator
if you don't want to.
README.md
Outdated
depending on your use case: | ||
|
||
- `makeBaseValidator<BaseT>` when you want the output to be narrowed-down to a subtype of `BaseT` (e.g. `str`). | ||
- `makeExactValidator<T>` when you want the output to be widened to `T` (e.g. `bool`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could probably use examples of each case (makeValidator vs makeExactValidator) here as I don't think it'll be clear to most readers without diving into the source
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if that's enough: 9727612
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't find the comment about formatting again; but fixed it here: ce1007f
Hey @af ! It's cool if this PR proves useful. Although it was hard at times, I had a lot of fun with it. I don't know if it will help, but perhaps a trick to read those validator types is following: // Such validator only works for subtypes of BaseT.
export interface BaseValidator<BaseT> {
// These overloads enable nuanced type inferences for optimal DX
// This will prevent specifying "default" alone from narrowing down output type
(spec: RequiredChoicelessSpecWithType<BaseT>): RequiredValidatorSpec<BaseT>
<T extends BaseT>(spec?: RequiredSpec<T>): RequiredValidatorSpec<T>
<T extends BaseT>(spec: OptionalSpec<T>): OptionalValidatorSpec<T>
} Think of it as function overloads. For instance, this: export const num = makeValidator<number>((input: string) => {
const coerced = parseFloat(input)
if (Number.isNaN(coerced)) throw new EnvError(`Invalid number input: "${input}"`)
return coerced
}) is strictly equivalent to this: export function num(spec: RequiredChoicelessSpecWithType<number>): ValidatorSpec<number>
export function num<T extends number>(spec?: RequiredSpec<T>): RequiredValidatorSpec<T>
export function num<T extends number>(spec: OptionalSpec<T>): OptionalValidatorSpec<T>
export function num(spec?: Spec<number>): ValidatorSpec<number> {
return internalMakeValidator((input: string) => {
const coerced = parseFloat(input)
if (Number.isNaN(coerced)) throw new EnvError(`Invalid number input: "${input}"`)
return coerced
})
} In the call site, TypeScript will simply try each of these signatures, starting at the topmost, and stop at the one matching the combination of type parameters and arguments. E.g.: const MY_NUM_VAR = num<1|2|3>(); In this instance, TypeScript picks the second overload from the top because:
|
ee01de1
to
328e056
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few requested changes but looking good. Almost ready to merge!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the revisions! Latest CI run failed but once that's resolved I'll merge 👍
@af Should be good now! |
@af Please excuse the disturbance; I have a small request: would you be OK to release an alpha/beta with these changes? We use this package extensively in multiple projects, so I'll be able to beta-test this with reasonable coverage. |
Thanks for the nudge, I'd meant to do that but it got lost in the shuffle. Just published |
Great to hear, thanks! I will take it for a spin on a few of my projects as well, then after some long-overdue docs updates I'll push out a beta unless anything comes up |
This PR offers three major improvements:
default
toundefined
cleanEnv
.Remark Should probably considered breaking for TS users and shipped within a major release.
Changes
Supports optional presence of environment variables
Output type (VScode)
Notice a few things:
STR_OPT
property is of typestring | undefined
STR_CHOICES
is narrowed down to'foo' | 'bar'
STR_REQ
is widened to typestring
whileSTR_DEFAULT_CHOICES
is narrowed-down to'foo' | 'bar'
Better JSON support
JSON output type can now be inferred from usage (
default
):Output type (VScode)
Make Validator Wrappers
There are now three types of validators which can be created with distinct functions. These functions are identical runtime-wise, but provide fine-grained typings to cover different scenarios:
makeBaseValidator<BaseT>
when you want the output to be narrowed-down to a subtype ofBaseT
(this is used instr
and all string-based internal validators).makeExactValidator<T>
when you want the output to be widened toT
(this is used inbool
validator).makeMarkupValidator
for input which can produce arbitrary output types (this is used injson
validator).makeValidator
is still there, but is typed likemakeBaseValidator
.Guarantee types stability with thorough types testing
Thanks to
expect-type
, we can test all scenarios and maintain high types quality.envalid/tests/types.test.ts
Lines 1 to 299 in a0f5e31