Skip to content
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

Overhaul detection of JSX attributes and tag names #47096

Closed
wants to merge 6 commits into from

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Dec 10, 2021

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
Copy link
Member

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 10, 2021

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
Copy link
Collaborator

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

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Dec 10, 2021
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
DanielRosenwasser pushed a commit that referenced this pull request Dec 10, 2021
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>
Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

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
Copy link
Member Author

Hm, maybe this is fine:

image

Returning unknown here breaks things.

@jakebailey
Copy link
Member Author

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

image

@jakebailey 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 jakebailey changed the title Disable JSX attribute snippet for HTML tag Disable JSX attribute snippet for HTML tag name Jan 4, 2022
@jakebailey
Copy link
Member Author

Okay, this should be more reasonable; PTAL.

@jakebailey
Copy link
Member Author

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.

return ScriptElementKind.memberVariableElement;
case SyntaxKind.LessThanToken:
case SyntaxKind.SyntaxList:
return ScriptElementKind.jsxTagName;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Will write some tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

image

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

image

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed the above as #47416, for reference.

@jakebailey jakebailey changed the title Disable JSX attribute snippet for HTML tag name Overhaul detection of JSX attributes and tag names Jan 12, 2022
@jakebailey
Copy link
Member Author

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 jakebailey marked this pull request as draft January 12, 2022 20:05
@jakebailey
Copy link
Member Author

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.

jakebailey added a commit to jakebailey/TypeScript that referenced this pull request Jan 12, 2022
@jakebailey
Copy link
Member Author

Closing in favor of #47412.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
5 participants