Update Razor DynamicFile Provider #76050
Conversation
dotnet-issue-labeler
bot
added
Area-Infrastructure
untriaged
ryzngard
changed the title
...geServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/RazorDynamicFileInfoProvider.cs
Outdated
Show resolved
Hide resolved
...geServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/RazorDynamicFileInfoProvider.cs
Outdated
Show resolved
Hide resolved
...geServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/RazorDynamicFileInfoProvider.cs
Outdated
Show resolved
Hide resolved
...geServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/RazorDynamicFileInfoProvider.cs
Outdated
Show resolved
Hide resolved
...geServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/RazorDynamicFileInfoProvider.cs
Outdated
Show resolved
Hide resolved
ryzngard
changed the title
...geServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/RazorDynamicFileInfoProvider.cs
Outdated
Show resolved
Hide resolved
| d => d.FilePath is not null && PathUtilities.PathsEqual(d.FilePath, dynamicFileInfoFilePath)); | ||
|
|
||
| var textChanges = response.Edits.Select(e => new TextChange(e.Span.ToTextSpan(), e.NewText)); | ||
| var textLoader = new TextChangesTextLoader(document, textChanges); |
There was a problem hiding this comment.
how often does this happen? Wondering if we need to instantiate a new loader each time or there was one TextLoader that we updated with the latest edits
There was a problem hiding this comment.
For closed documents it shouldn't be too often. It would be based on how often we have a diff when we generate. The most immediate cause I can think of would be if project info changed (going from unknown project to known project).
...eServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/RazorProvideDynamicFileParams.cs
Outdated
Show resolved
Hide resolved
...geServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/RazorDynamicFileInfoProvider.cs
Outdated
Show resolved
Hide resolved
| var sourceText = await document.GetTextAsync(cancellationToken).ConfigureAwait(false); | ||
|
|
||
| // Validate the checksum information so the edits are known to be correct | ||
| if (IsSourceTextMatching(sourceText)) |
There was a problem hiding this comment.
@davidwengier @dibarbet this is the logic I was thinking of on how to handle checksums. Telemetry would probably be done on the razor client side unless there's a preference for something else?
There was a problem hiding this comment.
We should definitely track it, fine with doing on the Razor side. However if we're doing the checks here (and reporting in Razor) we're going to lose the reason why they were different in telemetry, which might be key to figuring out where things are going wrong.
...sis.LanguageServer/HostWorkspace/Razor/RazorDynamicFileInfoProvider.TextChangesTextLoader.cs
Outdated
Show resolved
Hide resolved
...sis.LanguageServer/HostWorkspace/Razor/RazorDynamicFileInfoProvider.TextChangesTextLoader.cs
Outdated
Show resolved
Hide resolved
...sis.LanguageServer/HostWorkspace/Razor/RazorDynamicFileInfoProvider.TextChangesTextLoader.cs
Outdated
Show resolved
Hide resolved
|
Love the idea that this gives us a "get out of jail free card", in being able to get the whole document contents. Will be curious what the telemetry looks like for sure |
| var sourceText = await document.GetTextAsync(cancellationToken).ConfigureAwait(false); | ||
|
|
||
| // Validate the checksum information so the edits are known to be correct | ||
| if (IsSourceTextMatching(sourceText)) |
There was a problem hiding this comment.
We should definitely track it, fine with doing on the Razor side. However if we're doing the checks here (and reporting in Razor) we're going to lose the reason why they were different in telemetry, which might be key to figuring out where things are going wrong.
| return TextAndVersion.Create(newText, version.GetNewerVersion()); | ||
| } | ||
|
|
||
| return await GetFullDocumentFromServerAsync(razorUri, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
I would say though we should evaluate this in a month or so - if we never get non-matching texts this should be changed into an assert, and if we do, we should track down where
There was a problem hiding this comment.
💯 should revisit to make sure this number is not high because that indicates perf is suboptimal
...sis.LanguageServer/HostWorkspace/Razor/RazorDynamicFileInfoProvider.TextChangesTextLoader.cs
Outdated
Show resolved
Hide resolved
| public ServerTextChange[]? Edits { get; set; } | ||
|
|
||
| [JsonPropertyName("checksum")] | ||
| public required byte[] Checksum { get; set; } |
There was a problem hiding this comment.
Did this need to change to string? or did you work that out?
There was a problem hiding this comment.
oh woops. Have it local and didn't push
Roslyn side of dotnet/vscode-csharp#7826