-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Narrow falsy strings to '' and falsy numbers to 0 #45836
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
Narrow falsy strings to '' and falsy numbers to 0 #45836
Conversation
@typescript-bot test this |
Heya @andrewbranch, I've started to run the parallelized Definitely Typed test suite on this PR at 86e00f6. You can monitor the build here. |
Heya @andrewbranch, I've started to run the inline community code test suite on this PR at 86e00f6. You can monitor the build here. Update: The results are in! |
Heya @andrewbranch, I've started to run the extended test suite on this PR at 86e00f6. You can monitor the build here. |
Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 86e00f6. You can monitor the build here. |
Heya @andrewbranch, I've started to run the perf test suite on this PR at 86e00f6. You can monitor the build here. Update: The results are in! |
@andrewbranch |
Hey @andrewbranch, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
@andrewbranch Here they are:Comparison Report - main..45836
System
Hosts
Scenarios
Developer Information: |
The TypeScript team hasn't accepted the linked issue #45329. If you can get it accepted, this PR will have a better chance of being reviewed. |
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.
Code-wise this looks mostly fine to me, but @ahejlsberg and @RyanCavanaugh seemed to want to investigate the change more in the original issue and have yet to chime in? The last test results at least seem fine.
else if (type.flags & TypeFlags.Boolean) { | ||
return falseType; | ||
} | ||
else if (type.flags & TypeFlags.String) { |
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.
There probably needs to be a TemplateString
and StringMapping
case now, too, since both type kinds could possibly contain the empty string.
👍 I'll switch to draft pending #45329 (comment) |
The existing
filterType
function used bygetTypeWithFacts
<-narrowTypeByTruthiness
works as a filter: taking in an existing set of types and returning the ones that are allowed. This PR sets up afilterAndNarrowType
function that can also transform the types given to it into more narrow equivalents.truthyTypeFilter
is then used bynarrowTypeByTruthiness
to turns primitives into their falsy literals (e.g.number
->0
) when falsy.Sending as a draft as requested so we can try this out on real world code.Opening as a full PR since it's been a bit and none of the typescript-bot commands have shown any major red flags.Fixes #45329