Update ProjectWorkspaceState and HostProject at the same time #11191
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
(and clean up a teeny tiny bit)
|
Do not be put off by the integration test failures, the tests that failed are currently having a little flakiness moment. |
DustinCampbell
approved these changes
Nov 11, 2024
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/ProjectDifference.cs
Outdated
Show resolved
Hide resolved
Comment on lines
379
to
380
| else if (currentConfiguration.Equals(configuration) && | ||
| currentRootNamespace == rootNamespace) |
There was a problem hiding this comment.
Isn't RazorConfiguration is a record? If so, it should be equivalent to use ==.
Suggested change
| else if (currentConfiguration.Equals(configuration) && | |
| currentRootNamespace == rootNamespace) | |
| else if (currentConfiguration == configuration && | |
| currentRootNamespace == rootNamespace) |
There was a problem hiding this comment.
Yes, you're right. Paranoia because ProjectWorkspaceState isn't a record, yet we had code that did == and .Equals and I deleted one of the checks, but it was the wrong one. Looking forward to fixing ProjectWorkspaceState in future.
There was a problem hiding this comment.
I've got a few branches with ProjectWorkspaceState converted to a record that I'm hoping to resurrect.
davidwengier
added a commit
that referenced
this pull request
Nov 14, 2024
Follow up to #11191 which caused RPS regressions. There were some edge-y things I papered over in that PR, assuming they were unnecessary over-complications, but given the RPS regression I had a closer look, and they could have caused re-compilation of closed documents when only tag helpers changed, which would be new behaviour. Not _entirely_ convinced the re-compilation is all unnecessary (if a document used a Tag Helper that has only just been discovered, then its code gen would legitimately change) but it's hard to justify when I can't point to any tooling for the closed files that would actually benefit from the extra work. The key thing is not changing `DocumentCollectionVersion` when only `ProjectWorkspaceState` changes. Not re-calculating import documents was just a little extra amuse-bouche, and not caching the computed state tracker is pure paranoia.
davidwengier
added a commit
that referenced
this pull request
Nov 18, 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.
Preparation for ongoing work to hook up the Roslyn tokenizer and #11182 I suppose.
There were three places that
UpdateProjectWorkspaceStatewas called:RazorProjectService, just before callingUpdateProjectConfigurationProjectWorkspaceStateGenerator, where we will need to add a call toUpdateProjectConfigurationin future, to wire up the tokenizerPrevious attempts to plumb through more things for
RazorConfigurationresulted in RPS failures, that appeared to be simply more compilations of closed files. This makes sense because we were adding another update, which would have triggered another set ofProjectChangedevents. I thought it would make more sense to combine these two updates together, so no matter which part of the project was being updated, there could be a singleProjectChangednotification. This is that.