Overhaul detection of JSX attributes and tag names #47096

jakebailey · 2021-12-10T04:04:38Z

Use contextToken to better figure out that we're working with a JSX attribute or tag name. The location is not enough, unfortunately.

Fixes #47090
Fixes #47280

DanielRosenwasser · 2021-12-10T22:33:12Z

@typescript-bot cherry-pick this to release-4.5

typescript-bot · 2021-12-10T22:33:15Z

Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into release-4.5 on this PR at 7ac5e59. You can monitor the build here.

typescript-bot · 2021-12-10T22:38:09Z

Hey @DanielRosenwasser, I've opened #47105 for you.

Component commits: 191aa68 Disable JSX attribute snippet if attribute is acutally the HTML tag 1786d5e Add more tests of text before and after 7ac5e59 Big comment

Component commits: 191aa68 Disable JSX attribute snippet if attribute is acutally the HTML tag 1786d5e Add more tests of text before and after 7ac5e59 Big comment Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com>

DanielRosenwasser

I already merged this in for TypeScript 4.5. Let's see if we can get more eyes before we actually ship on 4.5 on Monday.

andrewbranch · 2021-12-13T21:09:14Z

src/services/completions.ts

        const kind = SymbolDisplay.getSymbolKind(typeChecker, symbol, location);
-        if (kind === ScriptElementKind.jsxAttribute && preferences.includeCompletionsWithSnippetText && preferences.jsxAttributeCompletionStyle && preferences.jsxAttributeCompletionStyle !== "none") {
+        if (
+            kind === ScriptElementKind.jsxAttribute


Why/how is kind set to jsxAttribute when the completion is actually a tag name? Isn’t that an issue itself, which, if fixed, would make the condition correct as previously written?

When I last spoke to Jake, it seemed like this is an issue that other parts of the surrounding code handle similarly. A deeper fix might be something for a follow-up issue.

I'm super AFK, but I completely agree that the right fix is to make the parser recover this differently. I was just trying to get something quick for the VS Code recovery release before I dipped out for my vacation. There's at least one other explainer comment elsewhere (some other block that is also checking that the kind is JSX attribute; I doubt I could find it from my phone!) that documents that the first attribute might be the tag name, which is why I was maybe a little less weirded out than someone looking at this PR in a vacuum.

jakebailey · 2022-01-03T22:00:07Z

It turns out that the parser does the right thing, it's just that for purposes of the completion, location is the < token, as we are completing after it. Before, this had a display kind of jsxAttribute, hence the bug.

I pushed a change that adds a JSX tag name kind, however I still thing this isn't quite right as technically, the query is asking about the < token, which is not itself a tag name. But, before, it was an attribute, which is still wrong too, so I'm going to look harder.

jakebailey · 2022-01-03T22:04:35Z

Hm, maybe this is fine:

Returning unknown here breaks things.

jakebailey · 2022-01-03T22:07:37Z

And if I just extend the check to be location.kind === SyntaxKind.Identifier || location.kind === SyntaxKind.LessThanToken to return ScriptElementKind.memberVariableElement:

jakebailey · 2022-01-04T21:54:48Z

Okay, this should be more reasonable; PTAL.

jakebailey · 2022-01-04T21:58:09Z

Note that this PR now includes an API change, which probably makes it not worthy of being ported to 4.5 anymore; I can likely send a PR to the 4.5 branch instead which handles the nested case without changing the "tag name completion shows as an attribute" problem.

andrewbranch · 2022-01-04T22:09:16Z

src/services/symbolDisplay.ts

+                            return ScriptElementKind.memberVariableElement;
+                        case SyntaxKind.LessThanToken:
+                        case SyntaxKind.SyntaxList:
+                            return ScriptElementKind.jsxTagName;


It would be good to test these for <Foo.Bar className="" /> and <Foo<Bar> className="" />. Probably already ok due to different parents, but good to check for good measure.

Good point. Will write some tests.

The latter case doesn't work before or after at all, actually:

Whereas, omit the type param and you get the completion:

Not a regression, but I'll file a new issue once I'm finished with a test case.

Your first example doesn't work with this PR, it turns out; the joy of the location being the same for both Foo.| and Foo.Foo |.

The screenshot above is misleading in that I took it while the original code was there (when I was verifying that my test failed when run on the old code, but before I actually wrote the second test). I'll keep looking.

Filed the above as #47416, for reference.

Old version of PR :)

jakebailey · 2022-01-12T08:01:41Z

A lot closer, but the tooltip on the completion is wrong (says attribute), yet no snippet appears. They should not differ, as the call that determines the kind is the same one that determines that it's not a snippet completion.

jakebailey · 2022-01-12T20:06:26Z

Per discussion, I'm going to split this into two PRs:

Fix snippet completions appearing in the wrong place, targeting 4.5 and 4.6 (replacing my fix there)
Eliminate JSX attribute text entirely, as it's been broken forever, targeting 4.6.

)" This reverts commit 1d4ec40.

jakebailey · 2022-01-12T21:29:40Z

Closing in favor of #47412.

jakebailey requested review from RyanCavanaugh and armanio123 December 10, 2021 04:04

typescript-bot assigned jakebailey Dec 10, 2021

typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Dec 10, 2021

jakebailey mentioned this pull request Dec 10, 2021

Attribute incorrectly added for opening JSX tag #47090

Closed

typescript-bot mentioned this pull request Dec 10, 2021

🤖 Pick PR #47096 (Disable JSX attribute snippet if at...) into release-4.5 #47105

Merged

DanielRosenwasser previously approved these changes Dec 10, 2021

View reviewed changes

andrewbranch reviewed Dec 13, 2021

View reviewed changes

Hecim1984 approved these changes Dec 19, 2021

View reviewed changes

sync-by-unito bot mentioned this pull request Jan 1, 2022

build(deps-dev): bump typescript from 4.3.5 to 4.5.4 numbersprotocol/capture-lite#1150

Closed

jakebailey changed the title ~~Disable JSX attribute snippet if attribute is acutally the HTML tag~~ Disable JSX attribute snippet for HTML tag Jan 4, 2022

jakebailey changed the title ~~Disable JSX attribute snippet for HTML tag~~ Disable JSX attribute snippet for HTML tag name Jan 4, 2022

jakebailey mentioned this pull request Jan 4, 2022

Jsx Attribute Completion not always works correct when tag is like <Form.Item> #47280

Closed

andrewbranch reviewed Jan 4, 2022

View reviewed changes

andrewbranch approved these changes Jan 4, 2022

View reviewed changes

Overhaul detection of JSX attributes and tag names

5bdee45

jakebailey force-pushed the fix-47090 branch from 58ddd9e to 5bdee45 Compare January 12, 2022 07:52

jakebailey changed the title ~~Disable JSX attribute snippet for HTML tag name~~ Overhaul detection of JSX attributes and tag names Jan 12, 2022

Undo leftover change

6a7c27b

jakebailey requested a review from andrewbranch January 12, 2022 07:54

jakebailey added 2 commits January 12, 2022 10:43

Fix symbol display in other places

2d0a7a3

Completely eliminate, deprecate jsxAttribute usage

c4a7bb1

jakebailey marked this pull request as draft January 12, 2022 20:05

jakebailey added 2 commits January 12, 2022 12:18

Remove leftover

e2a958d

Remove todo

73426dd

jakebailey added a commit to jakebailey/TypeScript that referenced this pull request Jan 12, 2022

Revert "Cherry-pick PR microsoft#47096 into release-4.5 (microsoft#47105

27b29cd

)" This reverts commit 1d4ec40.

jakebailey mentioned this pull request Jan 12, 2022

Rewrite logic for JSX attribute completion detection #47412

Merged

jakebailey closed this Jan 12, 2022

jakebailey mentioned this pull request Jan 12, 2022

Deprecate ScriptElementKind.jsxAttribute #47414

Merged

sync-by-unito bot mentioned this pull request Feb 1, 2022

build(deps-dev): bump typescript from 4.3.5 to 4.5.5 numbersprotocol/capture-lite#1240

Closed

jakebailey deleted the fix-47090 branch April 27, 2022 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhaul detection of JSX attributes and tag names #47096

Overhaul detection of JSX attributes and tag names #47096

jakebailey commented Dec 10, 2021 •

edited

Loading

DanielRosenwasser commented Dec 10, 2021

typescript-bot commented Dec 10, 2021 •

edited

Loading

typescript-bot commented Dec 10, 2021

DanielRosenwasser left a comment

andrewbranch Dec 13, 2021 •

edited

Loading

DanielRosenwasser Dec 13, 2021

jakebailey Dec 16, 2021

jakebailey commented Jan 3, 2022

jakebailey commented Jan 3, 2022

jakebailey commented Jan 3, 2022

jakebailey commented Jan 4, 2022

jakebailey commented Jan 4, 2022

andrewbranch Jan 4, 2022

jakebailey Jan 4, 2022

jakebailey Jan 4, 2022

jakebailey Jan 4, 2022

jakebailey Jan 13, 2022

jakebailey commented Jan 12, 2022

jakebailey commented Jan 12, 2022

jakebailey commented Jan 12, 2022

Overhaul detection of JSX attributes and tag names #47096

Overhaul detection of JSX attributes and tag names #47096

Conversation

jakebailey commented Dec 10, 2021 • edited Loading

DanielRosenwasser commented Dec 10, 2021

typescript-bot commented Dec 10, 2021 • edited Loading

typescript-bot commented Dec 10, 2021

DanielRosenwasser left a comment

Choose a reason for hiding this comment

andrewbranch Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

DanielRosenwasser Dec 13, 2021

Choose a reason for hiding this comment

jakebailey Dec 16, 2021

Choose a reason for hiding this comment

jakebailey commented Jan 3, 2022

jakebailey commented Jan 3, 2022

jakebailey commented Jan 3, 2022

jakebailey commented Jan 4, 2022

jakebailey commented Jan 4, 2022

andrewbranch Jan 4, 2022

Choose a reason for hiding this comment

jakebailey Jan 4, 2022

Choose a reason for hiding this comment

jakebailey Jan 4, 2022

Choose a reason for hiding this comment

jakebailey Jan 4, 2022

Choose a reason for hiding this comment

jakebailey Jan 13, 2022

Choose a reason for hiding this comment

jakebailey commented Jan 12, 2022

jakebailey commented Jan 12, 2022

jakebailey commented Jan 12, 2022

jakebailey commented Dec 10, 2021 •

edited

Loading

typescript-bot commented Dec 10, 2021 •

edited

Loading

andrewbranch Dec 13, 2021 •

edited

Loading