Rewrite logic for JSX attribute completion detection #47412
Conversation
jakebailey
requested review from
DanielRosenwasser,
andrewbranch,
RyanCavanaugh and
armanio123
typescript-bot
added
Author: Team
For Milestone Bug
src/services/completions.ts
Outdated
| @@ -744,8 +744,46 @@ namespace ts.Completions { | |||
| } | |||
| } | |||
|
|
|||
| const kind = SymbolDisplay.getSymbolKind(typeChecker, symbol, location); | |||
| if (kind === ScriptElementKind.jsxAttribute && preferences.includeCompletionsWithSnippetText && preferences.jsxAttributeCompletionStyle && preferences.jsxAttributeCompletionStyle !== "none") { | |||
| const isJSXAttributeCompletion = contextToken && forEachAncestor(contextToken, (n) => { | |||
There was a problem hiding this comment.
createCompletionEntry is a very hot path, so we should take care to (a) make this function as efficient as possible, and more importantly (b) not run it unless we need to. I would suggest moving it to its own function so it’s ergonomic to guard on
- the necessary preferences checks right after this function
- are we even in a TSX/JSX file
Maybe there’s also a function that ran earlier that already detected that we’re in a likely JSX attribute position (after all, it had to get the props type of the component/element in order to come up with the right list of symbols, so this seems likely) and we can save that state to CompletionData for use here.
There was a problem hiding this comment.
Good points, I'll take a look. Since this is a pure function of contextToken and location, there's also no reason to calculate it more than once, either, yet I do it per entry.
There was a problem hiding this comment.
Doh, that’s the easy answer here. But it does make me think, we basically already had to know we were getting props for JSX attributes, so maybe a lot of this can go away anyway.
There was a problem hiding this comment.
I feel really silly now. This is easily achievable with two flags that are set up earlier, just not passed down. Now it's both fast and not duplicated. Pushed now.
If only I had noticed much, much sooner.
|
Is attributes completion supposed to work only for intrinsic elements (i.e. html tags) or for all components? |
|
It should work for all components if they are typed. If you're having a problem, I suggest filling a bug (this is a 10 month old closed PR). |
Fixes #47090
Fixes #47280
This is the first part of #47096, which simply fixes the snippet issue by not using
SymbolDisplay.getSymbolKind(which is broken; note the oddity of the kinds in the test cases) to detect that we are completing a JSX attribute.