Allow Razor cohosting to run in VS Code, and fix it in VS #78167
Conversation
…service can be moved
ghost
added
Area-IDE
untriaged
| <Asset Type="Microsoft.VisualStudio.MefComponent" Path="Microsoft.CodeAnalysis.ExternalAccess.AspNetCore.dll" /> | ||
| <Asset Type="Microsoft.VisualStudio.MefComponent" Path="Microsoft.CodeAnalysis.ExternalAccess.Razor.dll" /> | ||
| <Asset Type="Microsoft.VisualStudio.MefComponent" Path="Microsoft.CodeAnalysis.ExternalAccess.Razor.Features.dll" /> | ||
| <Asset Type="Microsoft.VisualStudio.MefComponent" Path="Microsoft.CodeAnalysis.ExternalAccess.Razor.EditorFeatures.dll" /> |
There was a problem hiding this comment.
Wouldn't it be fun if this is even harder to get an exception for because it causes a 2 DLL load regression, instead of just the 1, because we accidentally improved DLL loads by trying to load a DLL that didn't exist?
| if (Directory.Exists(serviceHubCoreDirectory)) | ||
| { | ||
| directory = serviceHubCoreDirectory; | ||
| } |
There was a problem hiding this comment.
@chsienki this is a bit of a speculative change on my part, because it either doesn't matter whether its there, or I just always won the race, but it makes sense to me. In VS Code there is no ServiceHubCore folder. Does that make sense?
Also, doesn't matter if this isn't perfect for this PR, there will be more to come :)
There was a problem hiding this comment.
Yep, I'm fine with this. A secondary question to which I suspect I know the answer: do we have integration tests in Razor for VSCode? I would have expected this to break them if we do.
There was a problem hiding this comment.
We do, in the VS Code repo, though none with cohosting turned on (yet?).
I'm thinking of reverting this change though, as it was a bit speculative when I was having all sorts of source generator loading issues, so I was doing a bit of whack-a-mole to try to fix them.
There was a problem hiding this comment.
If there is no ServiceHubCore folder in VS, then I think this is fixing a legitimate issue. If we're redirect to a non-existent assembly, the later logic will fail, and we'll ultimately fall back to loading from the SDK, meaning our redirection is broken.
| <InternalsVisibleTo Include="Microsoft.CodeAnalysis.ExternalAccess.AspNetCore" /> | ||
| <InternalsVisibleTo Include="Microsoft.CodeAnalysis.ExternalAccess.OmniSharp.CSharp" /> | ||
| <InternalsVisibleTo Include="Microsoft.CodeAnalysis.ExternalAccess.OmniSharp.UnitTests" /> | ||
| <InternalsVisibleTo Include="Microsoft.CodeAnalysis.ExternalAccess.Razor" /> |
There was a problem hiding this comment.
Thank you 🙏 I thought I grep'ed through to see if there were more
| internal static string GenerateFullNameForOption(IOption2 option) | ||
| { | ||
| var optionGroupName = GenerateOptionGroupName(option); | ||
| // All options send to the client should have group name and config name. |
There was a problem hiding this comment.
You were right, but this all got reverted so it's back to being correct :)
|
|
||
| if (cohostStartupService is not null) | ||
| { | ||
| await cohostStartupService.Value.StartupAsync(serializedClientCapabilities, requestContext, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
consider trace logging on which service actually gets used, in case we need to debug something
There was a problem hiding this comment.
The two services are just dual-insertion prevention. I'm going to remove the old one as soon as dotnet/razor#11748 merges.
src/Tools/ExternalAccess/Razor/Features/Cohost/IRazorCohostDynamicRegistrationService.cs
Outdated
Show resolved
Hide resolved
…amicRegistrationService.cs Co-authored-by: Dustin Campbell <dustin@teamcampbell.org>
Goes with dotnet/roslyn#78167 and dotnet/vscode-csharp#8189 to expose cohosting to VS Code. Needs dotnet/roslyn#78167 merged and referenced before it will build.
6 participants
Goes with dotnet/vscode-csharp#8189 and dotnet/razor#11748 to allow Razor cohosting in VS Code, but also fixes some VSIX issues that broke cohosting in VS too. Reviewing commit-at-a-time is probably recommended.