-
Notifications
You must be signed in to change notification settings - Fork 760
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
Fix Intellisense and other features of Bicep in Visual Studio that were broken by VS 17.10 #14532
Conversation
3e12375
to
2a1238e
Compare
2a1238e
to
8246b91
Compare
Test this change out locally with the following install scripts (Action run 9965886474) VSCode
Azure CLI
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have tests/manual validation workflow to let us know early if this breaks in the future? I'm also curious if there is a set of partner tests/a process we can use so folks can reach out to us before these are broken. Even if that is us subscribed to the right breaking change notifications, that would be helpful as well.
src/vs-bicep/Bicep.VSLanguageServerClient.Vsix/Bicep.VSLanguageServerClient.Vsix.csproj
Outdated
Show resolved
Hide resolved
@AbhitejJohn The manual weekly testing will easily catch this. Right now the section on completion notes that it's broken. I'll be updating and also adding a few more simple scenarios. Hopefully I can get the automated testing back online, soon, too. |
eb30d2e
to
bfa4473
Compare
src/Bicep.LangServer.UnitTests/Handlers/BicepCodeActionHandlerTests.cs
Outdated
Show resolved
Hide resolved
src/vs-bicep/Bicep.VSLanguageServerClient/DidChangeWatchedFilesNotifier.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Fixes #14316
VS 17.10 added support for language server capabilities. This means that it started firing notifications only for events that were explicitly requested by the client (which is normal and works in vscode). Unfortunately, their LSP implementation lacks support for document selectors based on language ID (they only support file patterns, language/scheme are NYI). This means it didn't recognize any of our registrations and VS was giving us virtually no notifications, e.g. for file changes. The fix is to use file patterns for VS, language/scheme for any other hosts including vscode.
I fixed this such that it should only affect the behavior in VS, not VsCode or other hosts, out of an abundance of caution (it would be possible to return language/scheme as well as file patterns, but it does cause minor behavior changes in vscode and would also mean VS behavior would change in minor ways again after this fix this problem - better to just make the behavior explicit for both hosts).
The actual fix is in DocumentSelectorFactory, almost all of the rest is just passing along the dependencies it needs to know when to apply the fix.