-
Notifications
You must be signed in to change notification settings - Fork 13k
Fix JSX attribute completion for union types containing string-like t… #62242
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
Open
IAmArthurAdams
wants to merge
4
commits into
microsoft:main
Choose a base branch
from
IAmArthurAdams:fix-jsx-completion-union-types
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+82
−1
Open
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
52 changes: 52 additions & 0 deletions
52
tests/cases/fourslash/jsxAttributeCompletionStyleAutoSignalish.ts
This file contains hidden or 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
/// <reference path="fourslash.ts" /> | ||
|
||
// @Filename: foo.tsx | ||
//// declare namespace JSX { | ||
//// interface Element { } | ||
//// interface SignalLike<T> { | ||
//// value: T; | ||
//// peek(): T; | ||
//// subscribe(fn: (value: T) => void): () => void; | ||
//// } | ||
//// type Signalish<T> = T | SignalLike<T>; | ||
//// interface IntrinsicElements { | ||
//// div: { | ||
//// class?: Signalish<string | undefined>; | ||
//// id?: Signalish<string | undefined>; | ||
//// title?: Signalish<string | undefined>; | ||
//// disabled?: Signalish<boolean | undefined>; | ||
//// 'data-testid'?: Signalish<string | undefined>; | ||
//// role?: Signalish<string | undefined>; | ||
//// // For comparison - pure string type should still work | ||
//// pureString?: string; | ||
//// // Boolean-like should not get quotes | ||
//// booleanProp?: boolean; | ||
//// } | ||
//// } | ||
//// } | ||
//// | ||
//// <div [|prop_/**/|] /> | ||
|
||
// Test that string-like Signalish types prefer quotes over braces | ||
verify.completions({ | ||
marker: "", | ||
includes: [ | ||
{ | ||
name: "class", | ||
insertText: "class=\"$1\"", | ||
isSnippet: true, | ||
sortText: completion.SortText.OptionalMember, | ||
}, | ||
{ | ||
name: "id", | ||
insertText: "id=\"$1\"", | ||
isSnippet: true, | ||
sortText: completion.SortText.OptionalMember, | ||
}, | ||
], | ||
preferences: { | ||
jsxAttributeCompletionStyle: "auto", | ||
includeCompletionsWithSnippetText: true, | ||
includeCompletionsWithInsertText: true, | ||
} | ||
}); |
Oops, something went wrong.
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.
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.
I'm confused by "has non object types"; is this not actually reporting whether or not some member is
string
orundefined
ornull
?Can you go into detail somewhere (PR description, comment, something) about what specific case the repro is hitting and what special casing is required?
Uh oh!
There was an error while loading. Please reload this page.
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.
Thank you for the feedback!
I’ve renamed
hasNonObjectTypes
tohasPrimitiveTypes
and updated the comment to better reflect what the check is actually doing.The logic specifically checks whether the union contains both string-like types and primitive types (
string
,undefined
,null
). This distinction matters in cases like Preact’sSignalish<string | undefined>
(which expands tostring
|undefined
|SignalLike<string | undefined>
):Without ensuring a string-like type is present, TypeScript might apply this rule to unions like
null | undefined
, where quotes wouldn’t make sense. With this special-casing, we only switch to quoted completions ("$1") when the union actually includes string primitives.Uh oh!
There was an error while loading. Please reload this page.
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.
But
number
is primitive too? (We have a definition of Primitive in this codebase and it is different, so terminology is important.)I'm just finding this PR strange because the effect is "any union that contains a
string
andundefined
ornull
gets completions with quotes"). That meaning is obscured in your code because to get there the code first checks if all elements are strings/undefined, which the second branch will of course match as well.Basically, the code here looks complicated, but is effectively equivalent to swapping
every
tosome
+ only checkingTypeFlags.String
in the original code, which indeed passes the same tests.And that's the core problem;
Signalish<string | undefined>
isstring | undefined | SignalLike<string>
, andSignalike<string>
is an object, and so our old code rejected it because you might want to write a non-string. So this isn't a bugfix, this is a design change we'd need to weigh.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 the thorough feedback!
I agree the term “Primitive” doesn’t fit well with the repo’s existing definitions, so I could rename that variable to something clearer like
hasStringishOrNullableTypes
to avoid confusion.I understand that this change is more of a design choice than a bugfix. For union types like
Signalish<string | undefined>
, users generally expect quoted string completions rather than brace completions, even though the union includes object-like members. The original code’s strict “all types must be string-like or undefined” check prevented that and led to a less intuitive experience.While the new logic looks similar to swapping every for some, the goal was to cover these practical cases where quoted completions make more sense despite object members being present.
If this behavior shift feels too broad for this PR, I’m happy to revise or work on a more thorough approach based on your guidance. I’m also fine keeping this PR open whilst it is determined if this change is needed at all. 😊
Uh oh!
There was an error while loading. Please reload this page.
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.
If anything, I would prefer if the PR were just changed to
some
+ checking justTypeFlags.StringLike
, since that's the actual change and what we'd accept if we wanted to change this.And no offense, but I can tell that this is all LLM generated code and comment replies; the explanation ("While the new logic looks similar to swapping every for some, the goal was to cover these practical cases where quoted completions make more sense despite object members being present.") doesn't make much sense in context of what I said. It's a bit distracting.