Fix project publish (or: Various project system fixes) #10983
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
This was previously using the hash code of the document snapshot, therefore publishing every request. This meant we got publishes for old documents when they were superseded by newer ones.
The things receiving project events on the server side were all batching their work together, so quick remove-then-add sequences for documents would be merged together on the server side. Not doing that on the client side too just made it much harder for things to ever be in sync.
When publishing, we see a project remove for obj/Debug, and move all files to the misc project, then we'd see a new project for obj/Release, and add that. That add would then cause us to go through all of the misc files, and add them to the first project that could possibly have them - in this case, adding them right back to obj/Debug. Generally speaking we shouldn't be guessing at anything with project info, but rather rely on correct input from systems. ie, if they add a project, they presumably will then follow up with telling us which documents are in it.
Now that snapshots are self-versioned, if we don't keep our "previously published data" clean we'll end up re-publishing versions of documents that are new on the client side, but not new on the server.
…project When all documents are removed from a project, if the imports file comes first, it will cause a recompile of all open razor files, even though they will themselves later on be removed. This didn't used to cause any issues during publish, because the generated document would be sent over, and later clobbered with a subsequent change. Now that documents are self-versioned, that subsequent change could be for a version number less than the removed document, and hence be thrown away. Plus it's wasted work.
davidwengier
commented
Oct 8, 2024
| @@ -341,8 +341,6 @@ private ProjectKey AddProjectCore(ProjectSnapshotManager.Updater updater, string | |||
|
|
|||
| _logger.LogInformation($"Added project '{filePath}' with key {hostProject.Key} to project system."); | |||
|
|
|||
| TryMigrateMiscellaneousDocumentsToProject(updater); | |||
There was a problem hiding this comment.
Personally I think this method shouldn't be here, but for the record it wouldn't cause a problem if we actually removed projects when we get a RemoveProject call, rather than just zeroing it out. I can't for the life of me remember why we don't just remove them. Does anyone know?
There was a problem hiding this comment.
I have no idea. It's always seemed a wart to me.
There was a problem hiding this comment.
I wonder if this was just something that helped Razor get into a steady state when RazorProjectInfo was read from a file on disk.
DustinCampbell
approved these changes
Oct 8, 2024
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/GeneratedDocumentPublisher.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/GeneratedDocumentSynchronizer.cs
Outdated
Show resolved
Hide resolved
| @@ -341,8 +341,6 @@ private ProjectKey AddProjectCore(ProjectSnapshotManager.Updater updater, string | |||
|
|
|||
| _logger.LogInformation($"Added project '{filePath}' with key {hostProject.Key} to project system."); | |||
|
|
|||
| TryMigrateMiscellaneousDocumentsToProject(updater); | |||
There was a problem hiding this comment.
I have no idea. It's always seemed a wart to me.
| @@ -341,8 +341,6 @@ private ProjectKey AddProjectCore(ProjectSnapshotManager.Updater updater, string | |||
|
|
|||
| _logger.LogInformation($"Added project '{filePath}' with key {hostProject.Key} to project system."); | |||
|
|
|||
| TryMigrateMiscellaneousDocumentsToProject(updater); | |||
There was a problem hiding this comment.
I wonder if this was just something that helped Razor get into a steady state when RazorProjectInfo was read from a file on disk.
This was referenced Oct 15, 2024
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 #10970
Each commit in this PR essentially hardens the project system a little bit more. Together they fix the publish issue, which is a particularly difficult scenario, as it essentially removes and quickly re-adds a project, but I think the changes are pretty good overall. No one commit fixes the publish issue, and I wouldn't say there is no more work to do, but forward progress is forward progress.
Each commit is isolated to a particular service and has a brief description of what the change is and why, but I think it's perfectly understood when looked at as a whole too. Review however you prefer :)