Fix JSX attribute completion for union types containing string-like t… #62242
Conversation
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
|
@microsoft-github-policy-service agree |
src/services/completions.ts
Outdated
| // Check if the union contains string-like types that users would typically provide as strings | ||
| // This handles cases like Preact's Signalish<string | undefined> = string | undefined | SignalLike<string | undefined> | ||
| const hasStringLikeTypes = some(unionType.types, type => !!(type.flags & TypeFlags.StringLike)); | ||
| const hasNonObjectTypes = some(unionType.types, type => !!(type.flags & (TypeFlags.StringLike | TypeFlags.Undefined | TypeFlags.Null))); |
There was a problem hiding this comment.
I'm confused by "has non object types"; is this not actually reporting whether or not some member is string or undefined or null?
Can you go into detail somewhere (PR description, comment, something) about what specific case the repro is hitting and what special casing is required?
There was a problem hiding this comment.
Thank you for the feedback!
I’ve renamed hasNonObjectTypes to hasPrimitiveTypes 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’s Signalish<string | undefined> (which expands to string | 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.
There was a problem hiding this comment.
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 and undefined or null 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 to some in the original code, which indeed passes the same tests.
And that's the core problem; Signalish<string | undefined> is string | undefined | SignalLike<string>, and Signalike<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.
…ypes
Fixes #62237