Skip to content

Update Typescript VS interface#60635

Merged
dibarbet merged 4 commits intodotnet:release/dev17.2from
MariaSolOs:fix-ts-completion-duplicates
Apr 9, 2022
Merged

Update Typescript VS interface#60635
dibarbet merged 4 commits intodotnet:release/dev17.2from
MariaSolOs:fix-ts-completion-duplicates

Conversation

@MariaSolOs
Copy link
Contributor

CompletionOptions aren't correctly passed to TypeScript in Visual Studio, causing duplications in the completion list. This PR updates the relevant interfaces so that the CompletionProviders are correctly invoked.

@MariaSolOs MariaSolOs requested a review from a team as a code owner April 7, 2022 21:23
@ghost ghost added Community The pull request was submitted by a contributor who is not a Microsoft employee. Area-IDE labels Apr 7, 2022
Copy link
Member

@dibarbet dibarbet left a comment

Choose a reason for hiding this comment

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

looks fine to me assuming it fixes the issue - we will have to do a dual insertion for this so please don't merge yet (we're fighting other RPS issues).

@tmat should probably also take a look


internal sealed override bool ShouldTriggerCompletion(HostLanguageServices languageServices, SourceText text, int caretPosition, CompletionTrigger trigger, CompletionOptions options, OptionSet passThroughOptions)
=> ShouldTriggerCompletionImpl(text, caretPosition, trigger, options.TriggerOnTypingLetters);
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only need to trigger the completion of non-expanded items.

Copy link
Contributor Author

@MariaSolOs MariaSolOs Apr 8, 2022

Choose a reason for hiding this comment

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

Turns out that we didn't need to make any changes here and just verify that the correct GetCompletions method is invoked. Functioning locally and doesn't require any changes in the TypeScript side.

@MariaSolOs MariaSolOs changed the base branch from main to release/dev17.2 April 7, 2022 23:03
@dnfadmin
Copy link

dnfadmin commented Apr 8, 2022

CLA assistant check
All CLA requirements met.

return base.GetBetterItem(item, existingItem);
}

internal override Task<CompletionList> GetCompletionsAsync(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing this method since it's now overridden by the parent class.

@dibarbet dibarbet merged commit 9a34593 into dotnet:release/dev17.2 Apr 9, 2022
@MariaSolOs MariaSolOs deleted the fix-ts-completion-duplicates branch April 9, 2022 02:02
dibarbet added a commit that referenced this pull request Apr 13, 2022
* Update versions for preview 4

* Update Typescript VS interface (#60635)

* Update Typescript VS interface

* Restore method signatures in completion provider

* Move completion override to completion service with providers

* Remove duplicate override

Co-authored-by: Maria Solano <mariasolano@microsoft.com>

Co-authored-by: David Barbet <dabarbet@microsoft.com>
Co-authored-by: Joey Robichaud <jorobich@microsoft.com>
Co-authored-by: Maria José Solano <majosolano99@gmail.com>
Co-authored-by: Maria Solano <mariasolano@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Approved to merge Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants