Uh oh!
There was an error while loading. Please reload this page.
add jsx fragments to callLikeExpression - #59933
Conversation
TypeScript Bot (typescript-bot)
commented
Sep 10, 2024
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure Daniel Rosenwasser (@DanielRosenwasser) and Ryan Cavanaugh (@RyanCavanaugh) are aware of the changes, just as a heads up. |
Isabel Duan (iisaduan)
commented
Sep 10, 2024
TypeScript Bot (@typescript-bot) test it |
TypeScript Bot (typescript-bot)
commented
Sep 10, 2024
Hey Isabel Duan (@iisaduan), the results of running the DT tests are ready. Everything looks the same! |
TypeScript Bot (typescript-bot)
commented
Sep 10, 2024
Isabel Duan (@iisaduan) Here they are:tscComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
TypeScript Bot (typescript-bot)
commented
Sep 10, 2024
Isabel Duan (@iisaduan) Here are the results of running the user tests with tsc comparing Everything looks good! |
| export type JsxCallLike = | ||
| | JsxOpeningLikeElement | ||
| | JsxOpeningFragment; |
There was a problem hiding this comment.
wouldnt it make sense to include JsxOpeningFragment as part of JsxOpeningLikeElement?
There was a problem hiding this comment.
Isabel Duan (@iisaduan) can you address that suggestion?
There was a problem hiding this comment.
Apologies for the delay in response. It's a good question and I wanted something detailed for PR description.
TLDR keeping a type that has only opening element and self closing element, is still beneficial, and adding JsxOpeningFragment to JsxOpeningLikeElement requires a much bigger change than is needed to add this fragment type check. The biggest reason is because this add causes crashes in expression checking. We would either need to rework what fragments are, in a way that is making them almost identical to an JsxOpeningElement, or we would need to update the majority of places JsxOpeningLikeElement is referenced to ignore fragments anyway (and if we don't want to ignore them, we'd need to handle fragments specially, which could cause us to do redundant/unnecessary work).
In my first iteration of this PR, I did try to add fragments to openingLike, and the biggest issue was that we run into crashes during expression checking (probably due to several series of no longer correct assertions). Since the fragment check is now fully added, I tried again to fix these crashes over the last day or so, but I still ran into layers of them, so it just ends up being much more work than is necessary for this type check.
Also regarding the use of JsxOpeningLikeElement-- they have many more checks than what we need to check fragments, so adding fragments would require us to then ignore fragments in many uses of openinglike. For example, after this PR, just in checker, there are ~30 functions that would still error because of fragment incompatibilty with what JsxOpeningLikeElement is currently. These functions would almost always be updated to ignore fragments anyways, since ~20 are related to checking tagName or attributes. While the rest may have checks applicable to fragments, they aren't performing any different checks than what we already do (so we should probably ignore fragments because we don't need to do the work for them again). This is without taking into account other parts of the compiler/services, and anyone else using JsxOpeningLikeElement
Uh oh!
There was an error while loading. Please reload this page.
TypeScript Bot (typescript-bot)
commented
Sep 10, 2024
Isabel Duan (@iisaduan) Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
Uh oh!
There was an error while loading. Please reload this page.
| const errorNode = isJsxOpeningFragment(node) ? node : node.tagName; | ||
| const errorEntityName = isJsxOpeningFragment(node) ? "Fragment" : entityNameToString(node.tagName); | ||
| const diag = createDiagnosticForNode(errorNode, Diagnostics.Tag_0_expects_at_least_1_arguments_but_the_JSX_factory_2_provides_at_most_3, errorEntityName, absoluteMinArgCount, entityNameToString(factory), maxParamCount); | ||
| const tagNameDeclaration = isJsxOpeningFragment(node) ? getJSXFragmentType(node).symbol.valueDeclaration : getSymbolAtLocation(node.tagName)?.valueDeclaration; |
There was a problem hiding this comment.
Rather than writing Tag 'Fragment' expects at least..., should we specialize the message to just Fragments expect at least... for the fragment case? Since they're really not named tags, per sey.
There was a problem hiding this comment.
I ended up reverting this change. I did some testing, and even with proper checking of fragments in the rest of checkTagNameDoesNotExpectTooManyArguments, it doesn't seem possible to hit this error message with a fragment. Hitting this error message requires attributes to be passed in through the tag (example <TagName x={/*stuff*/}>). For just an opening element, if there are no attributes in the tag (<TagName>), we won't ever hit this error because we issue missing property or not assignable errors instead. So even if Fragment is defined to always take multiple arguments, we won't ever get this error
Typescript 5.7 will fail unless usage of RaxFragment is explicit, with an import of it and a `/** @jsxFrag RaxFragment */` pragma at the top of the file. This PR adds that. RaxFragment also needs to have an exported value. I wasn't sure what to use here so I copied preact, since preact is used in the Typescript handbook as an example: ```ts export const RaxFragment: FunctionComponent<{}> ``` Notably, though, the preact source has a comment indicating that *they're* not sure of the correct type either. Here is the Typescript 5.7 PR that made this error appear: microsoft/TypeScript#59933
It now requires an explicit import of React in order to use JSX fragments. Here is the Typescript 5.7 PR that introduced this error: microsoft/TypeScript#59933
It now requires an explicit import of React in order to use JSX fragments. Here is the Typescript 5.7 PR that introduced this error: microsoft/TypeScript#59933
This PR adds type checking to the children of JSX fragments. Fixes#50429.
JsxOpeningFragmenttoCallLikeExpressionby definingJsxCallLike.JsxOpeningFragmentis not added toJsxOpenlingLikeFragmentbecause it requires many more changes than necessary for this typecheck, and causes crashes in expression checking. More details here.NodeLinks:jsxFragmentTypeto store the type of the fragment at theSourceFilelevel, since a different fragment factory can be referenced per file.resolvedJsxElementAllAttributesTypeandresolvedIndexInfo(since both have been unused since at least 2018??)isCallLikeExpressionto account forInstanceOfExpressionTS2876: Using JSX fragments requires fragment factory {0} to be in scope, but it could not be found.We previously did not issue an error if a fragment factory was set through a pragma or compiler option, and the specified factory could not be resolved.