Demote priority of JS completions #49716
Conversation
Fixes #48498 Unchecked JS files gather identifier-based completions. Currently, this search happens instead of `getCompletionEntriesFromSymbols` for TS/checked JS files. However, identifier-based completions are much lower quality and can be ignored by some editors. Identifier-based completions should be gathered last, after gathering other completions. That's what this PR does.
typescript-bot
added
Author: Team
For Milestone Bug
|
Ah, I think your code is basically right but your description is off a bit. There’s a big chunk of completions that already does run first and doesn’t discriminate much between checked and unchecked files: |
src/services/completions.ts
Outdated
| } | ||
|
|
||
| if (!isChecked) { | ||
| const uniqueNames = getCompletionEntriesFromSymbols( |
There was a problem hiding this comment.
So this actually changes the order for unchecked files from
- symbols
- identifiers
- keywords
to
- keywords
- symbols
- identifiers
and I think we still want symbols first.
The (pre-existing) two huge calls to getCompletionEntriesFromSymbols are identical—can we lift and deduplicate that from the if/else? Then, let’s maintain uniqueNames from the result of that throughout the rest of this function so we don’t have to repeatedly regenerate it from entries.
There was a problem hiding this comment.
Ah, that makes sense. When I was moving the code around, I missed that entries was side-effected by getCompletionEntriesFromSymbols, and I assumed that the two calls were different since there would be no good reason to duplicate calls like that.
The code is much more elegant now, although reading it raises more questions than before. But those were questions that were hidden by the previous structure, in my opinion.
There was a problem hiding this comment.
Yeah, the important work is done by mutation and the return value is bonus info... not the best
There was a problem hiding this comment.
Yeah, the important work is done by mutation and the return value is bonus info... not the best
|
@typescript-bot run dt (Trying to fix the on-demand DT run) |
|
@typescript-bot run dt trying again |
src/services/completions.ts
Outdated
| uniqueNames: UniqueNameSet, | ||
| target: ScriptTarget, | ||
| entries: SortedArray<CompletionEntry>): void { | ||
| const entryNames = new Set(entries.map(e => e.name)); |
There was a problem hiding this comment.
GitHub isn’t showing me where this function gets called, but is this one redundant with uniqueNames too?
There was a problem hiding this comment.
Yep! I forgot to update that one, but it's the JS identifier-based completion path that I moved to the end of the function.
(So technically it's not required to add items to uniqueNames, but I'm going to, in order to avoid future surprises.)
Fixes #48498
Unchecked JS files gather identifier-based completions. Currently, this search happens instead of
getCompletionEntriesFromSymbolsfor TS/checked JS files. However, identifier-based completions are much lower quality and can be ignored by some editors.Identifier-based completions should be gathered last, after gathering other completions. That's what this PR does.