-
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
restore type inference & type safety to ts defs #98
Conversation
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.
Looks good to me, granted I'm not a TS user. Would it be possible to create a small runnable TS file in example/
that exercises the changes here? It'd make it a lot easier to verify any regressions in the typings moving forward.
// returned environment object can have properties other than the ones we've | ||
// validated. these are not parsed or processed, and thus are always of type | ||
// `string`. If you need better type safety and a fully-inferred environment, | ||
// use `cleanEnv` in strict mode. |
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 this comment! Makes me wonder if strict checking should have its own top-level cleanEnvStrict()
so the typings could me more exact though
@@ -67,7 +65,7 @@ interface CleanOptions { | |||
/** | |||
* A function used to transform the cleaned environment object before it is returned from cleanEnv. | |||
*/ | |||
transformer?: (env: CleanEnv) => CleanEnv | |||
transformer?: (env: unknown) => unknown |
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.
Just read up on the new unknown
type, pretty cool! I'm fine with bumping up the required typescript version with this change, probably means the next release should be a major since TS is becoming so widely used
@af done! i also amended the unfinished sentence in the existing example 😄 re: verifying regressions, there's also the existing typescript test file, which i've already adjusted for this PR. but as i noted in #83 (comment), it's difficult (and maybe ultimately undesirable) to make an example of invalid typescript code that tests exactly the properties we want to verify. for what it's worth, feel free to ping me on further typescript PRs if you'd like my input :) |
Thanks for adding the example, it really helps to be able to easily try and verify the inference and type-checking. This looks good and works well, merged! Do you think this should require a major version bump, due to the type definition changes and increased TS version requirement? |
yeah, i think so! even without the increased TS version requirement, this restores strict type checking for |
@af incidentally, any chance you'd be up for cutting a new release sometime soon? :) |
Yes, will do, just wanted to take a little time to see if there were any other changes to make before cutting a major. Regardless, planning on releasing that in the next few days, sorry for the delay! |
v5.0.0 released! Let me know if you run into any issues |
thanks so much ❤️ |
* restore type inference & safety to ts defs * widen return type of non-strict cleanenv * add typescript example
This PR reverts the problematic changes in #83, as discussed there.
Instead of changing the type of
env
back toany
, I changed it to the more recently introducedunknown
type, which has somewhat better properties for type safety when used as a return value. The only caveat to this is that it raises the minimum required Typescript version for consuming these definitions to 3.0, which may be acceptable.