feat(45010): handle unclosed fragment in `getJsxClosingTagAtPosition` #45532

hi-ogawa · 2021-08-21T10:47:43Z

This PR attempts to handle unclosed jsx fragment in language service so that it will auto-complete on vscode.
I used NodeFlags.ThisNodeHasError directly to detect the parse error related to the closing fragment, which might not be the most reliable way to check unclosed fragment, but at least for the test cases I added (mostly the copy of existing autoCloseTag.ts), it seems to be working.
Please let me know if this is a right approach or if there's a better way to do it.
I would appreciate any feedback, thanks a lot!

ghost · 2021-08-21T10:47:55Z

All CLA requirements met.

hi-ogawa · 2021-08-22T01:29:45Z

src/services/services.ts

@@ -2088,10 +2088,15 @@ namespace ts {
            const token = findPrecedingToken(position, sourceFile);
            if (!token) return undefined;
            const element = token.kind === SyntaxKind.GreaterThanToken && isJsxOpeningElement(token.parent) ? token.parent.parent
-                : isJsxText(token) ? token.parent : undefined;
+                : isJsxText(token) && isJsxElement(token.parent) ? token.parent : undefined;


To comment on why this change is necessary, I found that JsxText.parent is not necessarily JsxElement but it also can be JsxFragment, even though typing-wise it's supposed to be JsxElement according to

TypeScript/src/compiler/types.ts

Lines 2601 to 2605 in 8362a0f

export interface JsxText extends LiteralLikeNode {

readonly kind: SyntaxKind.JsxText;

readonly parent: JsxElement;

readonly containsOnlyTriviaWhiteSpaces: boolean;

}

Without this change, isUnclosedTag({ openingElement, ... }) below will lead to runtime error since openingElement becomes undefined, for example, in the test case const x = <> foo/*1*/ </> (see @Filename: /1.tsx in autoCloseFragment.ts).

Probably, a legitimate fix would be something like defining type JsxParent = JsxElement | JsxFragment and replacing existing member types parent: JsxElement with parent: JsxParant, but I'm not so sure yet that is what's supposed to be since I'm very new to the code base.
I would appreciate if someone familar with these parts can comment on this.

As far as I can tell, I think this is a good way to fix this problem. I think maybe the original code just didn't consider JsxFragments when it was written.

@hi-ogawa did you try changing the type of parent here to be JsxElement | JsxFragment? I agree we should really fix that type. Would you mind trying it and seeing if you can resolve any other errors that come up?

Thanks for the feedback!
Well, I didn't try anything yet, but I'll see what I can do and get back to you with my findings.

@andrewbranch Following your request, I addressed the issue with this commit 90a35e0.
There wasn't any type error related to JsxText.parent so it looks alright.
I also changed the type of JsxExpression.parent to include JsxFragment since I believe that's what it should be, then there was one type error in src/services/completions.ts, so I did a trivial fix for that.
Overall, I think there aren't much use JsxText.parent or JsxExpression.parent in the code base currently, so hopefully the type change won't have much impact.
But, one thing I worry about is that it's going to change public API (as you can see from baselines/references/api), so the library users will be also affected by this change.

melisarichmomnd

ShuiRuTian · 2021-08-22T06:16:50Z

Could I ask an code-unrelated question?
How did you debug this? I tried to fix this issue too, but found it seems this feature is not done through tsserver.
And even now I could not trigger the breakpoint.
Here is how I tried to debug:

run powershell command under typescript repo $env:TSS_DEBUG = 5859; code --user-data-dir ../vscode-debug/
set tsserver path of opened vscode to built tsserver.js of typescript reposiotry
restart tsserver of opened vscode.
choose "Attach to VS Code TS Server via Port" in typescript repo vscode debug menu.
choose 5859 port
set breakpoint and make changes on opened vscode.

And there is no related info on tsserver log, which you could see by run command Open ts server log when you press "ctrl+shift+p" on opened vscode.

gabritto

Looks very good, thanks! :)

gabritto · 2021-09-03T23:29:20Z

src/services/services.ts

@@ -2088,10 +2088,15 @@ namespace ts {
            const token = findPrecedingToken(position, sourceFile);
            if (!token) return undefined;
            const element = token.kind === SyntaxKind.GreaterThanToken && isJsxOpeningElement(token.parent) ? token.parent.parent
-                : isJsxText(token) ? token.parent : undefined;
+                : isJsxText(token) && isJsxElement(token.parent) ? token.parent : undefined;


As far as I can tell, I think this is a good way to fix this problem. I think maybe the original code just didn't consider JsxFragments when it was written.

andrewbranch · 2021-09-07T18:09:52Z

src/services/services.ts

@@ -2088,10 +2088,15 @@ namespace ts {
            const token = findPrecedingToken(position, sourceFile);
            if (!token) return undefined;
            const element = token.kind === SyntaxKind.GreaterThanToken && isJsxOpeningElement(token.parent) ? token.parent.parent
-                : isJsxText(token) ? token.parent : undefined;
+                : isJsxText(token) && isJsxElement(token.parent) ? token.parent : undefined;


@hi-ogawa did you try changing the type of parent here to be JsxElement | JsxFragment? I agree we should really fix that type. Would you mind trying it and seeing if you can resolve any other errors that come up?

andrewbranch · 2021-09-07T18:13:46Z

@ShuiRuTian I’m not sure, but the debug issue you were seeing might be solved by typescript.tsserver.useSeparateSyntaxServer: false. (You might also be interested in https://marketplace.visualstudio.com/items?itemName=andrewbranch.vscode-tsserver-debug for convenience 🙂)

andrewbranch

Thanks! I’m not concerned about the public API change—the API was wrong, and you’re fixing it. Consumers might have type errors, but it should help them catch bugs in their own code.

hi-ogawa · 2021-09-08T15:35:34Z

Awesome! Thank you two for the code review and triaging my first PR!

feat(45010): handle unclosed fragment in getJsxClosingTagAtPosition

08333d8

typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Aug 21, 2021

hi-ogawa commented Aug 22, 2021

View reviewed changes

Update tests

60498dd

melisarichmomnd suggested changes Aug 22, 2021

View reviewed changes

sandersn requested review from andrewbranch and gabritto September 3, 2021 13:36

sandersn assigned gabritto Sep 3, 2021

gabritto approved these changes Sep 3, 2021

View reviewed changes

andrewbranch requested changes Sep 7, 2021

View reviewed changes

Fix types of JsxText.parent and JsxExpression.parent

90a35e0

hi-ogawa force-pushed the 45010-auto-complete-jsx-fragment branch from 19cc4a3 to 90a35e0 Compare September 8, 2021 15:21

andrewbranch approved these changes Sep 8, 2021

View reviewed changes

andrewbranch merged commit 617251f into microsoft:main Sep 8, 2021

hi-ogawa deleted the 45010-auto-complete-jsx-fragment branch September 8, 2021 21:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(45010): handle unclosed fragment in `getJsxClosingTagAtPosition` #45532

feat(45010): handle unclosed fragment in `getJsxClosingTagAtPosition` #45532

Uh oh!

hi-ogawa commented Aug 21, 2021 •

edited

Loading

Uh oh!

ghost commented Aug 21, 2021 •

edited by ghost

Loading

Uh oh!

hi-ogawa Aug 22, 2021 •

edited

Loading

Uh oh!

gabritto Sep 3, 2021

Uh oh!

andrewbranch Sep 7, 2021

Uh oh!

hi-ogawa Sep 8, 2021

Uh oh!

hi-ogawa Sep 8, 2021 •

edited

Loading

Uh oh!

melisarichmomnd left a comment

Uh oh!

ShuiRuTian commented Aug 22, 2021 •

edited

Loading

Uh oh!

gabritto left a comment

Uh oh!

gabritto Sep 3, 2021

Uh oh!

andrewbranch Sep 7, 2021

Uh oh!

andrewbranch commented Sep 7, 2021

Uh oh!

andrewbranch left a comment

Uh oh!

hi-ogawa commented Sep 8, 2021

Uh oh!

Uh oh!

	export interface JsxText extends LiteralLikeNode {
	readonly kind: SyntaxKind.JsxText;
	readonly parent: JsxElement;
	readonly containsOnlyTriviaWhiteSpaces: boolean;
	}

feat(45010): handle unclosed fragment in getJsxClosingTagAtPosition #45532

feat(45010): handle unclosed fragment in getJsxClosingTagAtPosition #45532

Uh oh!

Conversation

hi-ogawa commented Aug 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Aug 21, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hi-ogawa Aug 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabritto Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

andrewbranch Sep 7, 2021

Choose a reason for hiding this comment

Uh oh!

hi-ogawa Sep 8, 2021

Choose a reason for hiding this comment

Uh oh!

hi-ogawa Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melisarichmomnd left a comment

Choose a reason for hiding this comment

Uh oh!

ShuiRuTian commented Aug 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gabritto left a comment

Choose a reason for hiding this comment

Uh oh!

gabritto Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

andrewbranch Sep 7, 2021

Choose a reason for hiding this comment

Uh oh!

andrewbranch commented Sep 7, 2021

Uh oh!

andrewbranch left a comment

Choose a reason for hiding this comment

Uh oh!

hi-ogawa commented Sep 8, 2021

Uh oh!

Uh oh!

feat(45010): handle unclosed fragment in `getJsxClosingTagAtPosition` #45532

feat(45010): handle unclosed fragment in `getJsxClosingTagAtPosition` #45532

hi-ogawa commented Aug 21, 2021 •

edited

Loading

ghost commented Aug 21, 2021 •

edited by ghost

Loading

hi-ogawa Aug 22, 2021 •

edited

Loading

hi-ogawa Sep 8, 2021 •

edited

Loading

ShuiRuTian commented Aug 22, 2021 •

edited

Loading