Add option to configure automatic optional chain completions #34552
Conversation
DanielRosenwasser
force-pushed
the
includeAutomaticOptionalChainCompletions
branch
from
b871ef4
to
4eca13e
Compare
|
Hey @typescript-bot where the heck are you? You're supposed to yell at @minestarks @amcasey and @mjbvz that there's been a protocol change. |
|
Oh, uh, I know the issue. |
src/services/completions.ts
Outdated
| const insertQuestionDot = origin && originIsNullableMember(origin); | ||
| if (insertQuestionDot && preferences === false) { |
There was a problem hiding this comment.
I'm guessing this was supposed to be preferences.includeAutomaticOptionalChainCompletions === false.
There was a problem hiding this comment.
Wow, comparability strikes - UserPreferences is all-optional and weak type checks don't get applied in comparisons.
There was a problem hiding this comment.
Out of curiosity, why is any object type considered comparable to a primitive type like boolean? I don't think the result of any such comparison could ever be meaningful, since objects are compared by reference at runtime (i.e. it would always compare false).
There was a problem hiding this comment.
@fatcerberus an object type doesn't imply an object value. "hello world" is a valid { toString(): string }
There was a problem hiding this comment.
No, I get why it’s assignable. I’m confused why it’s comparible, as the comparison is never meaningful.
Does assignability always imply comparability?
There was a problem hiding this comment.
Assignability should imply comparability - at least, that was always the intent. Ideally the hierarchy is
comparable to ≥ assignable to ≥ a subtype of ≥ identical to
DanielRosenwasser
force-pushed
the
includeAutomaticOptionalChainCompletions
branch
from
eb2c4d2
to
3d7451c
Compare
That's correct! |
DanielRosenwasser
changed the title
|
@typescript-bot pack this |
|
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 3d7451c. You can monitor the build here. It should now contribute to this PR's status checks. |
src/services/completions.ts
Outdated
| const insertQuestionDot = origin && originIsNullableMember(origin); | ||
| if (insertQuestionDot && preferences.includeAutomaticOptionalChainCompletions === false) { |
There was a problem hiding this comment.
This condition should be in addSymbolOriginInfo where we add symbols to the origin map in the first place.
There was a problem hiding this comment.
I mean in addPropertySymbol
There was a problem hiding this comment.
Oops, I guess I should clarify that I signed off that this change seems sensible for VS and not that the implementation looks correct.
If something can be turned off, someone will want to do it. We'll take it :) |
src/services/completions.ts
Outdated
| @@ -1119,6 +1120,9 @@ namespace ts.Completions { | |||
| insertQuestionDot = isRightOfDot && !isRightOfQuestionDot; | |||
| type = type.getNonNullableType(); | |||
| } | |||
| if (insertQuestionDot && preferences.includeAutomaticOptionalChainCompletions === false) { | |||
There was a problem hiding this comment.
Is this mistake? You don't want to return here.. Instead you want to add this check in addPropertySymbol where insertQuestionDot is checked
There was a problem hiding this comment.
Didn't realize it was a closure function and other checks were happening in there. Thanks!
There was a problem hiding this comment.
Okay, should be fixed now
DanielRosenwasser
deleted the
includeAutomaticOptionalChainCompletions
branch
Fixes #34542