Created OnAutoInsertFeature to support dynamic capability registration #7033
Conversation
|
FYI @dibarbet |
| import * as Is from 'vscode-languageclient/lib/common/utils/is'; | ||
| import * as UUID from 'vscode-languageclient/lib/common/utils/uuid'; | ||
|
|
||
| export class OnAutoInsertFeature implements DynamicFeature<RoslynProtocol.OnAutoInsertRegistrationOptions> { |
There was a problem hiding this comment.
I did not know this could be extended in this way. This is super useful!
| } | ||
| } | ||
| dispose(): void { | ||
| this._registrations.clear(); |
There was a problem hiding this comment.
I assume VSCode calls this when the instance shuts down? We don't need to manually do anything right?
There was a problem hiding this comment.
Documentation says this:
/**
* Called when the client is stopped to dispose this feature. Usually a feature
* un-registers listeners registered hooked up with the VS Code extension host.
*/That said, it didnt' actually get called on shutdown. Perhaps there's a different way to trigger this.
| return undefined; | ||
| } | ||
|
|
||
| private *getDocumentSelectors(): IterableIterator<VDocumentSelector> { |
There was a problem hiding this comment.
I'm learning so many new things in this PR...
There was a problem hiding this comment.
Me too. I haven't really written any TypeScript before. :)
| return undefined; | ||
| } | ||
| return ( | ||
| Is.boolean(capability) && capability === true |
There was a problem hiding this comment.
I don't think the capability is ever a boolean, right? https://github.com/dotnet/roslyn/blob/b67df4c5e39ac1a104b2e573542211d4d6b1cd1d/src/Features/LanguageServer/Protocol/Protocol/Internal/VSInternalServerCapabilities.cs#L51
There was a problem hiding this comment.
Doubtful but TextDocumentLanguageFeature does it like this so I kept it.
There was a problem hiding this comment.
I think that is because there are a lot of capabilities that are either booleans or an object - https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#serverCapabilities
Since we control this end to end (and its never a boolean), I believe we should remove the boolean check
| } | ||
| } | ||
| const registrations = count > 0; | ||
| return { kind: 'document', id: this.registrationType.method, registrations, matches: false }; |
There was a problem hiding this comment.
Wondering - could this implement DynamicDocumentFeature instead? Looks like it might already have this state management code
https://github.com/microsoft/vscode-languageserver-node/blob/54dd4abae40251a18fbe263285e6e97f27457e62/client/src/common/features.ts#L177
This is also probably outside ofthe scope of this PR, but it seems like most vscode features implement TextDocumentLanguageFeature, and then also have the implementation of the feature inside that type.
Wondering if that is something we can do here (instead of having a separate place where the implementation vs the capabilities happen).
e.g. https://github.com/microsoft/vscode-languageserver-node/blob/54dd4abae40251a18fbe263285e6e97f27457e62/client/src/common/hover.ts#L28
There was a problem hiding this comment.
I did go down that road initially. However, I wasn't able to get all the way there because it started making hardcoded assumptions about the list of language features. For example, it requires a client that implements FeatureClient<Middleware, LanguageClientOptions> but MiddleWare is hardcoded to this: https://github.com/microsoft/vscode-languageserver-node/blob/54dd4abae40251a18fbe263285e6e97f27457e62/client/src/common/client.ts#L338
|
@dibarbet Would you mind re-running the checks? Looks like some package downloads failed. Network issues? |
rerunning - this doesn't need the server side change to go in first, right? |
|
No, the server side change is really just for XAML. C# continues to use static registration, which still works with this change. |
Fixes #11822 Thanks to the awesome work of @mgoertz-msft and the sheer mind-blowing PR that is dotnet/vscode-csharp#7033 Roslyn has fully poly-filled OnAutoInsert support, so this PR is super easy. I feel like I should buy a lottery ticket!
2 participants
Created OnAutoInsertFeature which supports static capability registration for C# and dynamic capability registration for XAML.
Fixes #7030 Support for dynamic OnAutoInsert capability registration