Don't try to find "potential" projects for newly created .razor files #10323
Conversation
Since the misc files project is not a real project, no razor file will actually be a child of it. It's not even a real path that exists on disk
Sadly most of the usage of the AddDocument method, that took advantage of the fact that it would try to find a real project, were tests.
This was mostly covered by other tests, but they used the project snapshot manager directly.
The default factory can't handle the misc files project, so these tests were failing unnecessarily
DustinCampbell
left a comment
DustinCampbell
left a comment
There was a problem hiding this comment.
Looks good! Note that #10322 will cause merge conflicts for you.
| internal interface IRazorProjectService | ||
| { | ||
| Task AddDocumentAsync(string filePath, CancellationToken cancellationToken); | ||
| Task AddDocumentToMiscProjectAsync(string filePath, CancellationToken cancellationToken); |
There was a problem hiding this comment.
I understand the reason for the name change, and I like its specificity, but it makes me a little sad that the symmetry is lost.
There was a problem hiding this comment.
Somewhere in this PR, I'd love to see comments to explain why this is the way it is. Your PR description was good, and I think it'd be good to capture it in the code.
There was a problem hiding this comment.
it makes me a little sad that the symmetry is lost
RemoveDocument is in a similar position as AddDocument was: It's only called by the file watcher, and in lots and lots of tests. Given that it's removing documents though, its not having to guess so its a little more reasonable, though I'd still be a fan of having the client be the source of truth (via project.razor.vs.bin), I didn't want to break into that jail right now. Plus it would mean RemoveDocument would become MoveDocumentsToMiscProject so we'd lose symmetry anyway
Somewhere in this PR, I'd love to see comments to explain why this is the way it is. Your PR description was good, and I think it'd be good to capture it in the code.
That's why I renamed the method, but I'll add some comments around explaining why we don't want to try to do anything smarter, lest someone is tempted to repeat history :)
Fixes #9986
The issue at heart here is a race condition between our two project systems (one in the server, one in VS) and how they deal with/find new Razor files. What was happening was that the LSP server was discovering the newly named file, finding a project that it could fit in (based on path) and adding it to that project. It then starts generating C# code for that file, and sending it to VS. Meanwhile VS is discovering that same new file at its own pace, via CPS events etc., and adding the file to the project. VS then tells the LSP server about the new file, via project.razor.vs.bin, and all is right with the world. The problem comes when the LSP server sends generated C# to VS, before VS knows about the new file, but the server doesn't know that VS doesn't know. Once VS does know about the file, it receives an updated generated C# file at some point, which contains changes since the previous push, but to VS it's the first push.
I've long thought that the razor file watching has been unnecessary in the LSP server, as the client should be the source of truth for which files are in which projects. I'm not quite brave enough to remove it entirely though, so instead this change means newly created files are only added to the misc files project, and we never guess at which project(s) they should belong to. We wait for the client to tell us, and hence we avoid the race condition.