-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
add jsx fragments to callLikeExpression #59933
Merged
Merged
Changes from 3 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
0009efc
test case (before)
iisaduan 41b93dd
fix bug in test case
iisaduan 423ee81
fix implicit import
iisaduan 48187e8
clean up tests and comments
iisaduan 4c7543a
Merge branch 'main' of https://github.com/iisaduan/TypeScript into ty…
iisaduan fda397f
cache jsxfragment type in nodelinks so we will only error on the firs…
iisaduan e9578d4
fix isCallLikeExpression
iisaduan 0d4c3fb
Merge branch 'main' of https://github.com/microsoft/TypeScript into t…
iisaduan 9a83697
update baselines after merge
iisaduan 36bb8e7
rename test
iisaduan fb8defa
rename test
iisaduan 3765230
create type for empty fragment attributes only once
iisaduan bea0cba
revert error message change
iisaduan 6a20cc6
update error messages
iisaduan eb498b4
Update diagnostic message again
iisaduan 0250e44
number typo
iisaduan af56332
remove unused nodelinks property
iisaduan b7c272c
Merge branch 'main' of https://github.com/microsoft/TypeScript into t…
iisaduan b850aac
error number merge conflcit
iisaduan 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
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains 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
This file contains 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
iisaduan marked this conversation as resolved.
Show resolved
Hide resolved
|
This file contains 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
This file contains 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
This file contains 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
49 changes: 49 additions & 0 deletions
49
tests/baselines/reference/inlineJsxAndJsxFragPragmaOverridesCompilerOptions.errors.txt
This file contains 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,49 @@ | ||
| snabbdomy.tsx(6,1): error TS2304: Cannot find name 'null'. | ||
|
|
||
|
|
||
| ==== react.d.ts (0 errors) ==== | ||
| declare global { | ||
| namespace JSX { | ||
| interface IntrinsicElements { | ||
| [e: string]: any; | ||
| } | ||
| } | ||
| } | ||
| export function createElement(): void; | ||
| export function Fragment(): void; | ||
|
|
||
| ==== preact.d.ts (0 errors) ==== | ||
| export function h(): void; | ||
| export function Frag(): void; | ||
|
|
||
| ==== snabbdom.d.ts (0 errors) ==== | ||
| export function h(): void; | ||
|
|
||
| ==== reacty.tsx (0 errors) ==== | ||
| import {createElement, Fragment} from "./react"; | ||
| <><span></span></> | ||
|
|
||
| ==== preacty.tsx (0 errors) ==== | ||
| /** | ||
| * @jsx h | ||
| * @jsxFrag Frag | ||
| */ | ||
| import {h, Frag} from "./preact"; | ||
| <><div></div></> | ||
|
|
||
| ==== snabbdomy.tsx (1 errors) ==== | ||
| /** | ||
| * @jsx h | ||
| * @jsxfrag null | ||
| */ | ||
| import {h} from "./snabbdom"; | ||
| <><div></div></> | ||
| ~~ | ||
| !!! error TS2304: Cannot find name 'null'. | ||
|
|
||
| ==== mix-n-match.tsx (0 errors) ==== | ||
| /* @jsx h */ | ||
| /* @jsxFrag Fragment */ | ||
| import {h} from "./preact"; | ||
| import {Fragment} from "./react"; | ||
| <><span></span></> |
This file contains 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
25 changes: 25 additions & 0 deletions
25
tests/baselines/reference/jsxChildWrongFragment.errors.txt
This file contains 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,25 @@ | ||
| index.tsx(7,28): error TS2322: Type '{ children: () => string; }' is not assignable to type '{ children?: ReactNode; }'. | ||
| Types of property 'children' are incompatible. | ||
| Type '() => string' is not assignable to type 'ReactNode'. | ||
| index.tsx(10,47): error TS2322: Type '() => string' is not assignable to type 'ReactNode'. | ||
|
|
||
|
|
||
| ==== index.tsx (2 errors) ==== | ||
| /// <reference path="/.lib/react18/react18.d.ts" /> | ||
| /// <reference path="/.lib/react18/global.d.ts" /> | ||
|
|
||
| const test = () => "asd"; | ||
|
|
||
| // No Errors | ||
| const jsxWithJsxFragment = <>{test}</>; | ||
| ~~ | ||
| !!! error TS2322: Type '{ children: () => string; }' is not assignable to type '{ children?: ReactNode; }'. | ||
| !!! error TS2322: Types of property 'children' are incompatible. | ||
| !!! error TS2322: Type '() => string' is not assignable to type 'ReactNode'. | ||
|
|
||
| // Type '() => string' is not assignable to type 'ReactNode'. | ||
| const jsxWithReactFragment = <React.Fragment>{test}</React.Fragment>; | ||
| ~~~~ | ||
| !!! error TS2322: Type '() => string' is not assignable to type 'ReactNode'. | ||
| !!! related TS6212 index.tsx:10:47: Did you mean to call this expression? | ||
|
|
This file contains 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,24 @@ | ||
| //// [tests/cases/compiler/jsxChildWrongFragment.tsx] //// | ||
|
|
||
| //// [index.tsx] | ||
| /// <reference path="/.lib/react18/react18.d.ts" /> | ||
| /// <reference path="/.lib/react18/global.d.ts" /> | ||
|
|
||
| const test = () => "asd"; | ||
|
|
||
| // No Errors | ||
| const jsxWithJsxFragment = <>{test}</>; | ||
|
|
||
| // Type '() => string' is not assignable to type 'ReactNode'. | ||
| const jsxWithReactFragment = <React.Fragment>{test}</React.Fragment>; | ||
|
|
||
|
|
||
| //// [index.js] | ||
| "use strict"; | ||
| /// <reference path="react18/react18.d.ts" /> | ||
| /// <reference path="react18/global.d.ts" /> | ||
| const test = () => "asd"; | ||
| // No Errors | ||
| const jsxWithJsxFragment = React.createElement(React.Fragment, null, test); | ||
| // Type '() => string' is not assignable to type 'ReactNode'. | ||
| const jsxWithReactFragment = React.createElement(React.Fragment, null, test); |
This file contains 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,25 @@ | ||
| //// [tests/cases/compiler/jsxChildWrongFragment.tsx] //// | ||
|
|
||
| === index.tsx === | ||
| /// <reference path="react18/react18.d.ts" /> | ||
| /// <reference path="react18/global.d.ts" /> | ||
|
|
||
| const test = () => "asd"; | ||
| >test : Symbol(test, Decl(index.tsx, 3, 5)) | ||
|
|
||
| // No Errors | ||
| const jsxWithJsxFragment = <>{test}</>; | ||
| >jsxWithJsxFragment : Symbol(jsxWithJsxFragment, Decl(index.tsx, 6, 5)) | ||
| >test : Symbol(test, Decl(index.tsx, 3, 5)) | ||
|
|
||
| // Type '() => string' is not assignable to type 'ReactNode'. | ||
| const jsxWithReactFragment = <React.Fragment>{test}</React.Fragment>; | ||
| >jsxWithReactFragment : Symbol(jsxWithReactFragment, Decl(index.tsx, 9, 5)) | ||
| >React.Fragment : Symbol(React.Fragment, Decl(react18.d.ts, 390, 9)) | ||
| >React : Symbol(React, Decl(react18.d.ts, 62, 15)) | ||
| >Fragment : Symbol(React.Fragment, Decl(react18.d.ts, 390, 9)) | ||
| >test : Symbol(test, Decl(index.tsx, 3, 5)) | ||
| >React.Fragment : Symbol(React.Fragment, Decl(react18.d.ts, 390, 9)) | ||
| >React : Symbol(React, Decl(react18.d.ts, 62, 15)) | ||
| >Fragment : Symbol(React.Fragment, Decl(react18.d.ts, 390, 9)) | ||
|
|
This file contains 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,47 @@ | ||
| //// [tests/cases/compiler/jsxChildWrongFragment.tsx] //// | ||
|
|
||
| === Performance Stats === | ||
| Type Count: 1,000 | ||
|
|
||
| === index.tsx === | ||
| /// <reference path="react18/react18.d.ts" /> | ||
| /// <reference path="react18/global.d.ts" /> | ||
|
|
||
| const test = () => "asd"; | ||
| >test : () => string | ||
| > : ^^^^^^^^^^^^ | ||
| >() => "asd" : () => string | ||
| > : ^^^^^^^^^^^^ | ||
| >"asd" : "asd" | ||
| > : ^^^^^ | ||
|
|
||
| // No Errors | ||
| const jsxWithJsxFragment = <>{test}</>; | ||
| >jsxWithJsxFragment : JSX.Element | ||
| > : ^^^^^^^^^^^ | ||
| ><>{test}</> : JSX.Element | ||
| > : ^^^^^^^^^^^ | ||
| >test : () => string | ||
| > : ^^^^^^^^^^^^ | ||
|
|
||
| // Type '() => string' is not assignable to type 'ReactNode'. | ||
| const jsxWithReactFragment = <React.Fragment>{test}</React.Fragment>; | ||
| >jsxWithReactFragment : JSX.Element | ||
| > : ^^^^^^^^^^^ | ||
| ><React.Fragment>{test}</React.Fragment> : JSX.Element | ||
| > : ^^^^^^^^^^^ | ||
| >React.Fragment : React.ExoticComponent<{ children?: React.ReactNode | undefined; }> | ||
| > : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^ | ||
| >React : typeof React | ||
| > : ^^^^^^^^^^^^ | ||
| >Fragment : React.ExoticComponent<{ children?: React.ReactNode | undefined; }> | ||
| > : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^ | ||
| >test : () => string | ||
| > : ^^^^^^^^^^^^ | ||
| >React.Fragment : React.ExoticComponent<{ children?: React.ReactNode | undefined; }> | ||
| > : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^ | ||
| >React : typeof React | ||
| > : ^^^^^^^^^^^^ | ||
| >Fragment : React.ExoticComponent<{ children?: React.ReactNode | undefined; }> | ||
| > : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^ | ||
|
|
16 changes: 16 additions & 0 deletions
16
tests/baselines/reference/jsxFactoryAndJsxFragmentFactoryNull.errors.txt
This file contains 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,16 @@ | ||
| jsxFactoryAndJsxFragmentFactoryNull.tsx(3,1): error TS2304: Cannot find name 'null'. | ||
| jsxFactoryAndJsxFragmentFactoryNull.tsx(4,1): error TS2304: Cannot find name 'null'. | ||
| jsxFactoryAndJsxFragmentFactoryNull.tsx(4,17): error TS2304: Cannot find name 'null'. | ||
|
|
||
|
|
||
| ==== jsxFactoryAndJsxFragmentFactoryNull.tsx (3 errors) ==== | ||
| declare var h: any; | ||
|
|
||
| <></>; | ||
| ~~ | ||
| !!! error TS2304: Cannot find name 'null'. | ||
| <><span>1</span><><span>2.1</span><span>2.2</span></></>; | ||
| ~~ | ||
| !!! error TS2304: Cannot find name 'null'. | ||
| ~~ | ||
| !!! error TS2304: Cannot find name 'null'. |
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.
wouldnt it make sense to include
JsxOpeningFragmentas part ofJsxOpeningLikeElement?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.
@iisaduan can you address that suggestion?
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.
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 elementandself closing element, is still beneficial, and addingJsxOpeningFragmenttoJsxOpeningLikeElementrequires 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 anJsxOpeningElement, or we would need to update the majority of placesJsxOpeningLikeElementis 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 inchecker, there are ~30 functions that would still error because of fragment incompatibilty with whatJsxOpeningLikeElementis currently. These functions would almost always be updated to ignore fragments anyways, since ~20 are related to checkingtagNameorattributes. 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 usingJsxOpeningLikeElement