Conversation
af
left a comment
There was a problem hiding this comment.
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.
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
| * 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.
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
envback toany, I changed it to the more recently introducedunknowntype, 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.