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

Fix issue #139, validator return type is too broad #146

Merged
merged 2 commits into from
Oct 31, 2021

Conversation

jkomyno
Copy link
Contributor

@jkomyno jkomyno commented Apr 27, 2021

Hi, I think I have solved issue #139.
I have used type-coercion to the generic type T.
This approach is safe because all these generic types T extend the default return type (e.g. string for str()).

I've also added a small unit test, whose whole purpose is to type-check.

@af
Copy link
Owner

af commented Jun 25, 2021

Thanks for the PR (and sorry for the very slow response). Personally I think type coercion like this should be a last resort, so I'd like to see if we could solve the issue without it.

Just curious, did you investigate the suggested approach referenced here, and were there any issues with it? Because I think changing the API slightly for choices is preferable to adding a bunch of type coercion

@jkomyno
Copy link
Contributor Author

jkomyno commented Jun 25, 2021

Hi @af, thank you for the feedback.
Actually, TypeScript's as doesn't perform type coercion, it only performs a type assertion.
It helps the TypeScript type inference engine figuring out that the type returned by the inner function in the closure actually type-checks correctly with the generic (but bounded) T type.

I'm not sure I completely understood why adding other parameters in the API could be helpful in this case, so feel free to expand on that :)

@AlexRex
Copy link

AlexRex commented Oct 19, 2021

Hi @af, did you have any time to look at this? I think it will improve a lot the type checking for enums.

@af
Copy link
Owner

af commented Oct 31, 2021

Sorry for the long delay here, after finally playing around with it I think this is quite a nice addition despite my initial hesitation.

Edit: the following bit is wrong, you can omit as const and TypeScript still infers the union type correctly 🎉 which is surprising to me as I thought it would infer string[] by default with the choices there

One really cool thing about this approach is the following example:

const env = envalid.cleanEnv(process.env, {
  FOO: envalid.str({ choices: ['bar', 'baz'] as const })
});

const foo = env.FOO

Using as const for choices allows the type of foo to actually be inferred as 'bar' | 'baz', even without explicitly providing the type parameter to envalid.str()! That's a pretty big win in my books.

Going to merge this and then make some small additions to the new tests in a separate commit. Thanks again!

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

Successfully merging this pull request may close these issues.

3 participants