Rename component tags and type references when a Razor file is renamed #12561
Conversation
…g edits after the fact
src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/Rename/RemoteRenameService.cs
Outdated
Show resolved
Hide resolved
...t.VisualStudio.LanguageServices.Razor/LanguageClient/Cohost/CohostApplyRenameEditEndpoint.cs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| // Make sure that the filename itself is actually changing (ie, we don't care about file moves) | ||
| if (newFileName == Path.GetFileNameWithoutExtension(oldDoc.FilePath)) |
There was a problem hiding this comment.
Good question. Case sensitivity is something that we need to follow up on across the board, right now tooling and compiler behave differently (#12202) but based on the comment linked from that issue, I think this is correct. ie, CouNter.razor and Counter.razor are technically different components according to the compiler, so we'd still want to rename if only case was changing.
| """) | ||
| ], | ||
| expectedFiles: [ | ||
| (FileUri("OtherComponent.razor"), """ |
There was a problem hiding this comment.
The expectedFiles is just for validating the edits that we'll send to the IDE. The original component rename is not part of them, because it's already happened, we're just responding to it.
.../src/Microsoft.VisualStudio.LanguageServices.Razor/ProjectSystem/RenameProjectTreeHandler.cs
Outdated
Show resolved
Hide resolved
| return; | ||
| } | ||
|
|
||
| ApplyWorkspaceEditAsync(request).Forget(); |
There was a problem hiding this comment.
Where this gets called by CPS, if we await this task we end up with a JTF hang. Sadly there are no tests for this file, only for the things it calls that do the actual work.
...t.VisualStudio.LanguageServices.Razor/LanguageClient/Cohost/CohostApplyRenameEditEndpoint.cs
Outdated
Show resolved
Hide resolved
...t.VisualStudio.LanguageServices.Razor/LanguageClient/Cohost/CohostApplyRenameEditEndpoint.cs
Outdated
Show resolved
Hide resolved
| { | ||
| if (edit.TryGetFirst(out var textDocumentEdit) && | ||
| textDocumentEdit.TextDocument.DocumentUri is { UriString: { } uriString } documentUri && | ||
| !fileSystem.FileExists(documentUri.GetRequiredParsedUri().GetDocumentFilePath())) |
There was a problem hiding this comment.
Should these two conditions be inside the if block? I'm just curious if the file doesn't exist whether we still want the edit added to documentChanges.
There was a problem hiding this comment.
I don't think so. These edits are separate from the rename, and the only thing that produces them is us, so we're not dropping anyone elses data on the floor here. This is more for the "what if the file gets deleted/renamed again while we were in the request queue" case.
| """) | ||
| ], | ||
| expectedFiles: [ | ||
| (FileUri("Component.razor"), """ |
There was a problem hiding this comment.
Don't feel dumb, the timeline of these things is all very confusing :)
This is testing willRename, which happens before the rename. So VS Code will rename Component.razor for us, but before it does we get to edit its contents.
| var fileNameWithoutExtension = Path.GetFileNameWithoutExtension(uriString); | ||
| var fileNamePartLength = fileNameWithoutExtension.Length + extension.Length; | ||
|
|
||
| Debug.Assert(uriString.Length >= fileNamePartLength); |
| var fromComponentName = Path.GetFileNameWithoutExtension(request.OldFilePath); | ||
| var toComponentName = Path.GetFileNameWithoutExtension(request.NewFilePath); | ||
|
|
||
| var dialogFactory = (IVsThreadedWaitDialogFactory)_serviceProvider.GetService(typeof(SVsThreadedWaitDialogFactory)); |
There was a problem hiding this comment.
I don't know, I'm far from an expert at this. The comments on the service indicate its safe to get from any thread, so my assumption is that it wouldn't make a difference how the service is retrieved because it wouldn't need to thread switch. Am I misunderstanding?
There was a problem hiding this comment.
I'm not sure what that service initialization looks like. I just thought that since the service was being obtained in an async context that it might as well use the async version to get it.
| var position = text.GetLinePosition(declaration.Identifier.SpanStart); | ||
|
|
||
| string newComponentName; | ||
| using (var _ = StringBuilderPool.GetPooledObject(out var builder)) |
There was a problem hiding this comment.
Yeah, but then the string builder doesnt get returned to the pool until after the OOP call. I felt bad holding it for so long :)
3 participants
Fixes #8541
Fixes #11493
Needs dotnet/roslyn#81549 before this will build, but Roslyn CI seems to be having issues, so putting this up while we wait.
This implements
workspace/willRenameFilessupport in our LSP server, so that when a.razorfile is renamed, we perform a full rename of the component, including updating component tags and getting Roslyn changes for renaming the class name. In VS we also include a poly-fill sincewillRenameFilesis not supported, and an extra "apply rename" method to fill in the difference between realwillRenameFilessupport, where edits are made before file renames, and our polyfill which lets CPS handle the file rename, and applies the edits afterwards.VS:
VS Code:
