Skip to content
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

ExternalAccess.Razor breaks Roslyn layering rules #76674

Open
tmat opened this issue Jan 8, 2025 · 8 comments
Open

ExternalAccess.Razor breaks Roslyn layering rules #76674

tmat opened this issue Jan 8, 2025 · 8 comments
Assignees
Labels

Comments

@tmat
Copy link
Member

tmat commented Jan 8, 2025

Microsoft.CodeAnalysis.Remote.ServiceHub should not depend on EditorFeatures layer, but it does via EA.Razor:

Image

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 8, 2025
@tmat tmat changed the title ExternalAccess.Razor is breaking Roslyn layering ExternalAccess.Razor is breaking Roslyn layering rules Jan 8, 2025
@tmat tmat changed the title ExternalAccess.Razor is breaking Roslyn layering rules ExternalAccess.Razor breaks Roslyn layering rules Jan 8, 2025
@davidwengier
Copy link
Member

We need to split EA.Razor up into two different assemblies, one for EditorFeatures (and above) and one for everything else. This will be needed for cohosting in VS Code anyway.

FYI @phil-allen-msft

@CyrusNajmabadi
Copy link
Member

@davidwengier @phil-allen-msft plz. asap :)

@davidwengier
Copy link
Member

@ryzngard is working on this as we speak :)

@ryzngard
Copy link
Contributor

#77715 is the start. Once that's in I think we can do more cleanup and make sure Microsoft.CodeAnalysis.Remote.ServiceHub references the Features EA. Is that only loaded in OOP?

@tmat
Copy link
Member Author

tmat commented Mar 20, 2025

Yes, Remote.ServiceHub is OOP only

@deepakrathore33 deepakrathore33 removed the untriaged Issues and PRs which have not yet been triaged by a lead label Mar 26, 2025
@ryzngard
Copy link
Contributor

Alright, I did an initial pass to see what is needed from EditorFeatures

  1. RazorDynamicRegistrationServiceFactory uses IUIContextActivationService
  2. RazorCSharpInterceptionMiddleLayerWrapper uses AbstractLanguageClientMiddleLayer

That doesn't seem too bad. I think the easiest path forward would be to move everything to shared that isn't editorfeatures, make Roslyn OOP uses the new Microsoft.CodeAnalysis.ExternalAccess.Razor.Features dll, and move razor over. That should also avoid needing to dual insert. Will have to test if this ends up being another binary load.

@davidwengier does that make sense for the direction of cohosting and what requires the editor?

@davidwengier
Copy link
Member

Yep, makes sense, those two things are specifically VS concepts. Eventually we might want VS Code equivalents though. I thought there was also some stuff we used in tests that created a Roslyn language server, but maybe that's already abstracted on the Roslyn side.

@CyrusNajmabadi
Copy link
Member

Makes sense to me as well. Do you think this can be done soon? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants