Initial cohost server implementation #9674
Conversation
davidwengier
force-pushed
the
RoslynCohost
branch
4 times, most recently
from
153da8a
to
16da9b8
Compare
We need these for now, so that our DocumentVersionCache and DocumentContextFactory etc. work as expected. In the long run these might be unnecessary, as we get everything from Roslyn.
davidwengier
force-pushed
the
RoslynCohost
branch
from
16da9b8
to
9797f66
Compare
Forgot this one
|
Integration test run passed: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=8830201 |
|
|
||
| namespace Microsoft.AspNetCore.Razor.LanguageServer.DocumentColor; | ||
|
|
||
| internal interface IDocumentColorService : ICapabilitiesProvider |
There was a problem hiding this comment.
Probably for another time, but this feels a bit awkward. Should we just move to a central location for capabilities?
There was a problem hiding this comment.
I personally prefer the "each endpoint does its own capabilities" model, but I agree the document service doing it is weird. Ideally the ApplyCapabilities impl would return back to the endpoint, once there is only one.
|
|
||
| var serverCapabilities = new VSInternalServerCapabilities(); | ||
|
|
||
| foreach (var provider in _cohostCapabilitiesProviders) |
There was a problem hiding this comment.
Our capabilities are not any difference right? This is just needed so we can run both cohosting and our current lsp server together?
There was a problem hiding this comment.
No, the capabilities are different: We don't offer the same capability in cohosting and non. They sum together to the full set we support.
This is the place for capabilities to be set in cohosting, and some version of this code will stay. The way it works is that endpoints apply their own capabilities, so as we expose an endpoint to cohosting, via MEF, we remove it from the normal server, but not registering it in DI.
| @@ -45,4 +45,6 @@ internal class TestLanguageServerFeatureOptions : LanguageServerFeatureOptions | |||
| public override bool IncludeProjectKeyInGeneratedFilePath => _includeProjectKeyInGeneratedFilePath; | |||
|
|
|||
| public override bool MonitorWorkspaceFolderForConfigurationFiles => _monitorWorkspaceFolderForConfigurationFiles; | |||
|
|
|||
| public override bool UseRazorCohostServer => throw new System.NotImplementedException(); | |||
There was a problem hiding this comment.
Should we just go ahead and add an optional constructor param for this?
There was a problem hiding this comment.
So far I've only done it as needed. This was hardoded to return false in another PR to the feature branch
| snapshot.DocumentFilePaths, | ||
| filePath => Assert.Equal("C:\\Path\\Index.cshtml", filePath), | ||
| filePath => Assert.Equal("C:\\Path\\About.cshtml", filePath)); | ||
| snapshot.DocumentFilePaths.OrderBy(d => d), |
There was a problem hiding this comment.
Interesting that this hasn't broken before. I guess just changing the comparer did it?
There was a problem hiding this comment.
Yeah, I guess so. Perhaps its stored in hashcode order, and we happened to get lucky with the previous hashcode calculation?
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/RazorDocumentMappingService.cs
Show resolved
Hide resolved
ghost
added this to the 
First part of #9519
Razor side of dotnet/roslyn#70819
This PR consumes the new Cohost server from Roslyn, and when enabled, moves DocumentColor requests from being handled by our existing Razor Language Server, to instead be handled by the new server. There is currently no sharing of project system data or anything like that yet. A few TODOs for follow ups,
can't be merged, and indeed won't build, until a Roslyn package is available etc. butit's reviewable while we keep working, and more important its shippable in its "off" state.Ignore the last commit, obviously won't be merged :)