feat(typescript-estree): computed members discriminated unions #1349
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the PR, @bradzacher! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day. |
bradzacher
changed the title
bradzacher
commented
Dec 18, 2019
bradzacher
force-pushed
the
prop-name-discriminated-unions
branch
from
f8cc808 to
811ad0a
Compare
armano2
suggested changes
Dec 18, 2019
packages/typescript-estree/tests/ast-alignment/fixtures-to-test.ts
Outdated
Show resolved
Hide resolved
property names are either `computed: false` with `key: Identifier | StringLiteral | NumberLiteral`, or they are `computed: true` with `key: Expression`.
the previous typings took the simple approach of just having a single type, but it made things a bit more cumbersome.
This change creates two types for each, using the `computed` value as the key for a discriminated union.
This means that if you check `node.computed === true`, then TS will narrow the `key` type appropriately.
I also noticed a minor bug in the `TSEnumMember` handling, as the types didn't previously support the fact that it's syntactically valid to use an expression (note it's semantically invalid - TS error 1164).
It's also semantically and syntactically valid to do something like `enum Foo { ['key'] }`, which really should be handled differently to `enum Foo { key }`, even if they mean the same thing.
So I also added a `computed` prop to `TSEnumMember`, and gave it the same discriminated union treatment.
bradzacher
force-pushed
the
prop-name-discriminated-unions
branch
from
b026aad to
e91a7fe
Compare
Codecov Report
@@ Coverage Diff @@
## master #1349 +/- ##
=========================================
- Coverage 94.01% 94% -0.02%
=========================================
Files 131 131
Lines 5867 5867
Branches 1662 1663 +1
=========================================
- Hits 5516 5515 -1
Misses 186 186
- Partials 165 166 +1
|
armano2
previously approved these changes
Dec 19, 2019
armano2
previously approved these changes
Dec 19, 2019
armano2
approved these changes
Dec 19, 2019
bradzacher
changed the title
This was referenced Dec 20, 2019
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1345
Property names are either
computed: falsewithkey: Identifier | StringLiteral | NumberLiteral, or they arecomputed: truewithkey: Expression.the previous typings took the simple approach of just having a single type, but it made things a bit cumbersome to use.
This change creates two types for each, using the
computedvalue as the key for a discriminated union.This means that if you check
node.computed === true, then TS will narrow thekeytype appropriately.I also noticed a minor bug in the
TSEnumMemberhandling, as the types didn't previously support the fact that it's syntactically valid to use an expression (note it's semantically invalid - TS error 1164).It's also semantically and syntactically valid to do something like
enum Foo { ['key'] }, which really should be handled differently toenum Foo { key }, even if they mean the same thing.So I also added a
computedprop toTSEnumMember, and gave it the same discriminated union treatment.