Remove VS dependency from cohost Html synchronizer and endpoints #11758
Conversation
After allowing for telemetry in the API, I realised diagnostics and completion in cohosting never got any actual code for it written 🤦♂️
The "generate code" bit of the publisher is IDE-agnostic, the rest isn't, so it would have had to be duplicated. Plus it was a terrible abstraction, I've no idea what I was thinking. I would introduce an IHtmlDocumentGenerator but it seems like overkill.
ryzngard
left a comment
ryzngard
left a comment
There was a problem hiding this comment.
LGTM.
Had some small nits and questions about design as a whole, and one musing that is unrelated to the PR but made me think of it.
| public async Task TrySynchronize_NewDocument_Generates() | ||
| { | ||
| var publisher = new TestHtmlDocumentPublisher(); | ||
| var synchronizer = new HtmlDocumentSynchronizer(StrictMock.Of<TrackingLSPDocumentManager>(), publisher, LoggerFactory); |
| public static Task<TResponse?> MakeHtmlLspRequestAsync<TRequest, TResponse>(this IHtmlRequestInvoker requestInvoker, TextDocument razorDocument, string method, TRequest request, CancellationToken cancellationToken) | ||
| where TRequest : notnull | ||
| { | ||
| return requestInvoker.MakeHtmlLspRequestAsync<TRequest, TResponse>(razorDocument, method, request, threshold: TimeSpan.Zero, correlationId: Guid.Empty, cancellationToken); |
There was a problem hiding this comment.
In what instances do we not want a correlation id?
There was a problem hiding this comment.
Whenever we don't want telemetry for a request. Currently we only report for map code, diagnostics, semantic tokens and completion.
There was a problem hiding this comment.
gotcha. Do we plan on adding to those?
There was a problem hiding this comment.
Your guess is as good as mine.
| var syncResult = await _htmlDocumentSynchronizer.TrySynchronizeAsync(razorDocument, cancellationToken).ConfigureAwait(false); | ||
| if (!syncResult) | ||
| { | ||
| _logger.LogDebug($"Couldn't synchronize for {razorDocument.FilePath}"); | ||
| return default; | ||
| } | ||
|
|
||
| if (!_documentManager.TryGetDocument(razorDocument.CreateUri(), out var snapshot)) |
There was a problem hiding this comment.
not specifically for this review, but it always felt weird to me to have a pattern of
var synchronized = TrySynchronize();
....
var snapshot = GetSnapshot()This could just be a separation of interests. But the synchronization result could include the snapshot that it synchronized for. In a purely "snapshot" based way that is more accurate than allowing something else to publish in between these two calls.
There was a problem hiding this comment.
But the synchronization result could include the snapshot that it synchronized for.
This is what it used to be like, and I agree it feels weird to "wait" for synchronization without getting something meaningful back, but in VS Code there is no snapshot, its really just "wait for the document to have been published". If the sync result included the snapshot, then the sync code can't be shared.
There was a problem hiding this comment.
I'd love to take this offline and workshop a bit. It seems like we could fix this, but it would be an infectious change rather than an isolated change.
There was a problem hiding this comment.
Happy to. I did have an IHtmlDocumentResult so VS Code and VS could have their own "buffer"/"snapshot" types, but in VS Code it only needs the Uri, so the abstraction was useless.
|
|
||
| _logger.LogDebug($"Making LSP request for {method} from {htmlDocument.Uri}{(request is ITextDocumentPositionParams positionParams ? $" at {positionParams.Position}" : "")}."); | ||
|
|
||
| // Passing Guid.Empty to this method will mean no tracking |
There was a problem hiding this comment.
This seems to be the pattern everywhere, not sure why it's specifically called out here.
There was a problem hiding this comment.
Just to reassure readers that I didn't forget argument validation
...rc/Microsoft.VisualStudio.LanguageServices.Razor/LanguageClient/Cohost/HtmlRequestInvoker.cs
Outdated
Show resolved
Hide resolved
...rc/Microsoft.VisualStudio.LanguageServices.Razor/LanguageClient/Cohost/HtmlRequestInvoker.cs
Outdated
Show resolved
Hide resolved
…nguageClient/Cohost/HtmlRequestInvoker.cs
3 participants
As part of porting cohosting to VS Code we need to implement the Html document sync system, but my original implementation here was quite leaky. This fixes that, but making the synchronizer and publisher just do the job their names imply, and abstracting away any ContainedLanguage dependency from the endpoints. It also abstract it away from the synchronizer, so that can be shared, which is good since its complicated. The publisher will have to be re-implemented, but its implementation will be different enough that it makes sense.
Review commit-at-a-time probably is easiest, and I must admit, I fixed a couple of minor things in the middle that aren't strictly related :)