Allow documents to have multiple virtual documents of a single virtual document type #9057
Conversation
…l document type These changes are deliberately crafted to (hopefully) not cause binary or source breaking changes in the consumers of the ContainedLanguage DLL. Any consumer that doesn't currently support multiple virtual documents should have unchanged behaviour, and unchanged API calls.
This started as pseudocode to allow me to reason about the shape of the API I'd need, and ended up being something that is pretty close to done, and more importantly, something that should work fine with our current system. This could have been part of a subsequent PR, but I though it might help you all understand the API better.
These are for the next PR to fill in.
Most of this is mechanical, to keep the code compiling, but there are a couple of new tests in here too.
| #pragma warning disable IDE0060 // Remove unused parameter | ||
| private static bool IsMatch(ProjectKey projectKey, VSProjectContext projectContext) | ||
| { | ||
| return true; |
There was a problem hiding this comment.
This is where my work and Andrew's work will combine (or collide head-on 😛)
Unfortunately I'm not sure if there is any way to avoid filling this in, in the next PR, which means essentially the next PR is going to have to do all of the remaining work (ie, if we turn on proper multiple virtual documents, we'll need to come up with a uri/file path scheme, which means we'll need a way to map it from a project context.
| @@ -57,7 +57,11 @@ private IReadOnlyList<VirtualDocument> CreateVirtualDocuments(ITextBuffer hostDo | |||
| { | |||
| if (factory.Metadata.ContentTypes.Any(ct => hostDocumentBuffer.ContentType.IsOfType(ct))) | |||
| { | |||
| if (factory.Value.TryCreateFor(hostDocumentBuffer, out var virtualDocument)) | |||
| if (factory.Value.TryCreateMultipleFor(hostDocumentBuffer, out var newVirtualDocuments)) | |||
There was a problem hiding this comment.
Still reading, but when would TryCreateMultipleFor fail? Could it just create a single?
There was a problem hiding this comment.
I think I understand more now. We're using this as a gate for if multiple is supported. If so, it should be called instead of the single version?
There was a problem hiding this comment.
I'm not sure I follow. It is called instead of the single version, unless it returns false. I wouldn't expect it to return false, but I followed the pattern of TryCreateFor since one of the other consumers might have a scenario where they want to return false.
The base implementation could call TryCreateFor, but I wanted to be super conservative about not changing existing code paths.
src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/CSharpVirtualDocumentFactory.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/CSharpVirtualDocumentFactory.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Andrew Hall <[email protected]>
ghost
added this to the 
Part of part 5 of #8983
This is a bit of a weird one. I'm doing it separately because the main focus of this is updating the ContainedLanguage project, which is public API consumed by Web Tools and TypeScript for their LSP servers in VS. That is also the reason that some of the changes are not the usual style of refactoring that I would do, but rather are carefully craft to not break binary or source compatibility.
The rest of this is a sort of sample usage of how this new API will be used, to provide multi-targeting support. None of this does anything yet (you'll see the hardcoded ProjectKey in one spot) as that is coming in the next PR. I could have left all of this until that PR, but I think its a bit more informative to see how the new API is used, and writing it (and the tests) gave me some confidence that I was actually creating something that would solve the problem. Unlike some, I don't actually know how to "plan" anything, and instead just write code as I think, and think as I code 😁