-
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
Rework how IProjectSnapshotManager is composed into Visual Studio and the Razor Language Server #10063
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 don't know if you wanted reviews or not, but...
...azor/src/Microsoft.VisualStudio.LanguageServices.Razor/RazorLSPTextViewConnectionListener.cs
Outdated
Show resolved
Hide resolved
...zor/src/Microsoft.AspNetCore.Razor.LanguageServer/Extensions/IServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
ba2ed93
to
2bea5b0
Compare
Gonna do one last rebase on main... |
This change removes the implementation and usages of IProjectSnapshotManagerAccessor from the language server. Instead, services can just import an IProjectSnapshotManager. This requires updating more tests to just use tooling components directly, rather than mocking them out. In particular, updating RenameEndpointTest required extra work because the mocks were inconsistent with reality.
This updates DefaultProjectSnapshotManager to stop importing IProjectSnapshotManagerTrigger instances.
This change removes IProjectSnapshotChangeTrigger implementations in the language server and introduces the concept of an IRazorStartupService. Start up services will be created when Razor tooling is started up.
…ger completely Fixes dotnet#9950 This is a pretty significant change that requires refactoring of how the project snapshot manager is exported within Visual Studio. The IProjectSnapshotManagerAccessor implementations and usages have been completely removed, and IProjectSnapshotChangeTrigger has also been deleted. Now, if a service wants to subscribe to project change events, they can import an IProjectSnapshotManager. And, if that service wants to be loaded when the Razor tooling is boot-strapped, they can export themselves as an IRazorStartupService.
This change updates the VS Razor start up service initialization to include the three entry points that bootstrap Razor tooling: 1. LegacyTextViewConnectionListener: If the legacy editor is enabled and a project is loaded with an editor already opened, Razor tooling bootstraps when a subject buffer is connected. 2. RazorLSPTextViewConnectionListener: If the LSP editor is enabled and a project is loaded with an editor already opened, Razor tooling bootstraps when a subject buffer is connected. 3. WindowsRazorProjectHostBase: If a project is loaded and there aren't any open editors, Razor tooling bootstraps at the first update to the project manager.
These methods just call into IErrorReporter.ReportError(...). Without them, ProjectSnapshotManager no longer needs to take an IErrorReporter in its constructor.
8e0096d
to
06a8872
Compare
All good now. |
davidwengier
approved these changes
Mar 13, 2024
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!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #9950
Fixes #9515
Fixes #9514
I recommend reviewing commit by commit.
The primary goal of this pull request is to change how
IProjectSnapshotManger
is created and how listeners subscribe to it. Primarily,IProjectSnapshotChangeTrigger
andIProjectSnapshotManagerAccessor
have been removed. To make sure that services previously exported asIProjectSnapshotChangeTriggers
are still loaded, I've introduced the notion of anIRazorStartupService
. This is a service that is guaranteed to be loaded when Razor tooling is bootstrapped in both the VS and Language Server. Often, these startup services are responsible for keeping the project snapshot manager up-to-date and need to be loaded before any project updates have occurred.In order to remove
IProjectSnapshotManagerAccessor
, I needed to re-work several tests to use real Razor tooling objects, rather than mocks. In particularRenameEndpointTest
received a lot of attention, though there's still more work that can be done there.