-
Notifications
You must be signed in to change notification settings - Fork 199
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
Start Razor Language Server when new Razor LSP editor opens. #1487
Conversation
- Included the LanguageServer core dlls as part of the VSIX install. To do this I had to expose the language server's output path via a target and then dynamically include its outputs in the VSIX output. - Given how VSIX's are built I could not privately reference our language server to ensure that it's built when the VSIX is built. To work around this limitation I added a `_BuildLanguageServer` target that calls MSBuild directly on the language server as part of build. - For now I'm dynamically booting the currently built language server that's deployed with the extension. I haven't given a lot of thought to what flavor of OS (x64 vs. x86) the server can run on; or if we need to ensure dotnet.exe is available. - Found that the VS LSP client and the O# language server library that we use don't inter-operate as we'd expect. The server ends up telling the client that it doesn't care about text document change events (we do). I've created a separate PR in O# to fix this: OmniSharp/csharp-language-server-protocol#199 dotnet/aspnetcore#17789
@dotnet/aspnet-build This PR doesn't do anything special with dependencies and the one it's based off of already passed. Any idea why signing would be exploding all of sudden? |
@dotnet/aspnet-build Never mind, I see why it's complaining in this PR given that I'm lifting bits into the VSIX now. |
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.
I have some questions but suspect I just don't understand the context here. Feel free to merge once they're answered.
public IEnumerable<string> FilesToWatch => null; | ||
|
||
public event AsyncEventHandler<EventArgs> StartAsync; | ||
public event AsyncEventHandler<EventArgs> StopAsync |
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.
Why is the add/remove explicitly defined here but implicitly defined for StartAsync
?
Also, I don't see anywhere that StopAsync
is actually called? I suspect I misunderstand the workflow of ILanguageClient. Could you go into the expected flow a bit?
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.
StartAsync
is called when we actually start the language server but we don't actually take control of stopping the language server currently. We rely on the ILanguageClient infrastructure to stop us. The gist here is that our ILanguageClient is very "dumb" currently. It's just the boiler plate to start a basic language server and then I plan to fill it out later as we flesh it all out more.
var extensionDirectory = Path.GetDirectoryName(currentAssemblyLocation); | ||
var languageServerPath = Path.Combine(extensionDirectory, "LanguageServer", "rzls.exe"); | ||
var info = new ProcessStartInfo(); | ||
info.FileName = languageServerPath; |
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.
Possible nit:
var info = new ProcessStartInfo{
info.FileName = languageServerPath,
// ...
}
|
||
public Task OnServerInitializeFailedAsync(Exception e) | ||
{ | ||
return Task.CompletedTask; |
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.
All three of these functions strike me as in need of diagnostic logging (but especially OnServerInitializeFailedAsync
). Would it be possible to add a constructor that takes a logger so it can be used here, or will the DI system not enable that?
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.
You're definitely right. We'll expand on the implementation of this class in the coming months. For now it's just basic boiler plate to get the ball rolling 😄
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.
My concerns are explained, .
- Included the LanguageServer core dlls as part of the VSIX install. To do this I had to expose the language server's output path via a target and then dynamically include its outputs in the VSIX output. - Given how VSIX's are built I could not privately reference our language server to ensure that it's built when the VSIX is built. To work around this limitation I added a `_BuildLanguageServer` target that calls MSBuild directly on the language server as part of build. - For now I'm dynamically booting the currently built language server that's deployed with the extension. I haven't given a lot of thought to what flavor of OS (x64 vs. x86) the server can run on; or if we need to ensure dotnet.exe is available. - Found that the VS LSP client and the O# language server library that we use don't inter-operate as we'd expect. The server ends up telling the client that it doesn't care about text document change events (we do). I've created a separate PR in O# to fix this: OmniSharp/csharp-language-server-protocol#199 - Add OmniSharp 3rd party library to signing information for VSIX insertions. dotnet/aspnetcore#17789
- Included the LanguageServer core dlls as part of the VSIX install. To do this I had to expose the language server's output path via a target and then dynamically include its outputs in the VSIX output. - Given how VSIX's are built I could not privately reference our language server to ensure that it's built when the VSIX is built. To work around this limitation I added a `_BuildLanguageServer` target that calls MSBuild directly on the language server as part of build. - For now I'm dynamically booting the currently built language server that's deployed with the extension. I haven't given a lot of thought to what flavor of OS (x64 vs. x86) the server can run on; or if we need to ensure dotnet.exe is available. - Found that the VS LSP client and the O# language server library that we use don't inter-operate as we'd expect. The server ends up telling the client that it doesn't care about text document change events (we do). I've created a separate PR in O# to fix this: OmniSharp/csharp-language-server-protocol#199 - Add OmniSharp 3rd party library to signing information for VSIX insertions. dotnet/aspnetcore#17789
_BuildLanguageServer
target that calls MSBuild directly on the language server as part of build.dotnet/aspnetcore#17789