Fixed JSX attributes discriminating based on optional children #53980
Conversation
typescript-bot
added
the
For Milestone Bug
| @@ -29336,14 +29336,17 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| } | |||
|
|
|||
| function discriminateContextualTypeByJSXAttributes(node: JsxAttributes, contextualType: UnionType) { | |||
| const jsxChildrenPropertyName = getJsxElementChildrenPropertyName(getJsxNamespaceAt(node)); | |||
| return discriminateTypeByDiscriminableItems(contextualType, | |||
| concatenate( | |||
| map( | |||
| filter(node.properties, p => !!p.symbol && p.kind === SyntaxKind.JsxAttribute && isDiscriminantProperty(contextualType, p.symbol.escapedName) && (!p.initializer || isPossiblyDiscriminantValue(p.initializer))), | |||
There was a problem hiding this comment.
I came to the conclusion that this "branch" doesn't need any immediate adjustments because JSX expressions can't be easily used as value-based discriminants.
JsxTextis always typed asstringType(no string literal types there)- more than 1 children also can't be a discriminant value because arrays~ can't be such
In theory, it's actually possible to have things like:
type Props = {
children: "a"
foo: string
} | {
children: "b"
foo: number
}
;<Comp foo="bar">{"a"}</Comp>So maybe this should be improved further, for this very specific use case - just to be consistent with:
;<Comp foo="bar" children={"a"} />I think though that this can be done in a separate PR (if requested). It would be somewhat weird though that those 2 wouldn't behave the same, so perhaps further adjustments (beyond this function here) should be introduced in such a PR:
;<Comp foo="bar">{"a"}</Comp>
;<Comp foo="bar">a</Comp>Note that this doesn't work in 5.0 anyway so likely I didn't regress this in #53502: TS playground 5.0
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at 862f863. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 862f863. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at 862f863. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 862f863. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 862f863. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 862f863. You can monitor the build here. Update: The results are in! |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running |
|
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
@jakebailey Here they are:Comparison Report - main..53980
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
|
Hey @jakebailey, the results of running the DT tests are ready. Main only errors:Package: wordpress__components |
|
@typescript-bot pack this |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 862f863. You can monitor the build here. |
|
Hey @jakebailey, 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 |
fixes #53941