Added basic extract to component functionality on cursor over html tag #10578
Conversation
There was a problem hiding this comment.
This looks really really good! Nice work.
There are a bunch of styling changes I could suggest, but I didn't want to overwhelm you with comments. If you would prefer it, I can do that, or we can go through together on a call? Up to you.
Holding off approval just because you'll have to merge in the main branch and switch to System.Text.Json in a few places. Sorry about that!
|
|
||
| internal sealed class ExtractToComponentBehindCodeActionParams | ||
| { | ||
| public required Uri Uri { get; set; } |
There was a problem hiding this comment.
Sorry, I broke your code :(
You'll need to add a [JsonParameterName("uri")] attribute to this property, and corresponding attributes to other properties in this class
| _clientConnection = clientConnection ?? throw new ArgumentNullException(nameof(clientConnection)); | ||
| } | ||
| public string Action => LanguageServerConstants.CodeActions.ExtractToComponentBehindAction; | ||
| public async Task<WorkspaceEdit?> ResolveAsync(JObject data, CancellationToken cancellationToken) |
There was a problem hiding this comment.
Apologies again, the data parameter is a JsonElement now
| return SpecializedTasks.Null<IReadOnlyList<RazorVSInternalCodeAction>>(); | ||
| } | ||
|
|
||
| var componentNode = owner?.FirstAncestorOrSelf<MarkupElementSyntax>(); |
There was a problem hiding this comment.
Nit: Can remove the ? here. On line 58 we've verified that owner can't be null, so not only is it unnecessary, it actually sends a message to the compiler that we're not sure about it, even though we are, and could cause the compiler to produce more warnings for subsequent uses of owner.
| if (context.Location.AbsoluteIndex > componentNode.StartTag.Span.End && | ||
| context.Location.AbsoluteIndex < componentNode.EndTag.SpanStart) |
There was a problem hiding this comment.
This logic seems a bit backwards. For "inside a tag" shouldn't it be checking against the start of the start tag, and the end of the end tag? It's doing the opposite.
There was a problem hiding this comment.
I might've described the logic incorrectly, although it has the same effect regardless. The code action will not be provided if the cursor is inside html content (or text), that is, after a starting tag and before an ending tag. This doesn't conflict with other code actions, as the check for whether the componentNode is a markupElement comes just before these lines.
There was a problem hiding this comment.
I think I understand what you're saying. Would be good to have tests to verify what happens at the very edges of start tags and end tags, but perhaps more importantly, it would be good to expand the comment to be a bit clearer about what is happening, and why the logic seems backwards, even though it isn't. In other words, take most of your reply in this thread, and paste it into the code :)
| using Microsoft.CodeAnalysis.Razor.Workspaces; | ||
|
|
||
| namespace Microsoft.AspNetCore.Razor.LanguageServer.CodeActions.Razor; | ||
| internal sealed class ExtractToComponentBehindCodeActionProvider : IRazorCodeActionProvider |
There was a problem hiding this comment.
Nit: I don't think the word "Behind" belongs here. Generally the term "code behind" referes to a C# file, since this is create a razor file just "Extract to component" is enough.
This would apply to pretty much every occurance of the word "behind" in this PR, but I won't comment everywhere :)
| /// </summary> | ||
| /// <param name="path">The origin file path.</param> | ||
| /// <returns>A non-existent file path with the same base name and a code-behind extension.</returns> | ||
| private static string GenerateComponentBehindPath(string path) |
There was a problem hiding this comment.
Rather than copy this method from ExtractToCodeBehindCodeActionResolver, it would be good to move it to a shared file somewhere, and have both that class and this class use the same method.
I just pushed the main changes into the feature branch. @marcarro you can just do these commands in your local branch and then handle merge commits. I can help you with that today or Friday |
| using Microsoft.CodeAnalysis.Razor.Logging; | ||
| using Microsoft.CodeAnalysis.Razor.Workspaces; | ||
|
|
||
| namespace Microsoft.AspNetCore.Razor.LanguageServer.CodeActions.Razor; |
There was a problem hiding this comment.
| namespace Microsoft.AspNetCore.Razor.LanguageServer.CodeActions.Razor; | |
| namespace Microsoft.AspNetCore.Razor.LanguageServer.CodeActions.Razor; | |
I know create new file doesn't do this and it's annoying...
There was a problem hiding this comment.
Ah, not really sure what the change is here, is it a newline?
| if (loggerFactory is null) | ||
| { | ||
| throw new ArgumentNullException(nameof(loggerFactory)); | ||
| } |
There was a problem hiding this comment.
| if (loggerFactory is null) | |
| { | |
| throw new ArgumentNullException(nameof(loggerFactory)); | |
| } |
@davidwengier we're moving away from these checks on internal code right?
There was a problem hiding this comment.
Yep, as long as nullability analysis is enabled, we shouldnt need to null check. There are a few spots in the compiler that still aren't annotated (like with the syntax tree below this) but for contructor parameters, definitely don't need them.
There was a problem hiding this comment.
Also, if we do need ArgumentNullException checks, there's a one-liner for that in the Razor code base:
ArgHelper.ThrowIfNull(loggerFactory);| n is MarkupBlockSyntax || | ||
| n is CSharpTransitionSyntax || | ||
| n is RazorCommentBlockSyntax); |
There was a problem hiding this comment.
nit: (nit stands for nitpick)
| n is MarkupBlockSyntax || | |
| n is CSharpTransitionSyntax || | |
| n is RazorCommentBlockSyntax); | |
| n is MarkupBlockSyntax or | |
| CSharpTransitionSyntax or | |
| RazorCommentBlockSyntax); |
This is a newer csharp feature than when some of the code was written, but I find it easier to read :)
| } | ||
|
|
||
| var componentName = Path.GetFileNameWithoutExtension(componentBehindPath); | ||
| var componentBehindContent = text.GetSubTextString(new CodeAnalysis.Text.TextSpan(actionParams.ExtractStart, actionParams.ExtractEnd - actionParams.ExtractStart)).Trim(); |
There was a problem hiding this comment.
| var componentBehindContent = text.GetSubTextString(new CodeAnalysis.Text.TextSpan(actionParams.ExtractStart, actionParams.ExtractEnd - actionParams.ExtractStart)).Trim(); | |
| var newComponentContent = text.GetSubTextString(new CodeAnalysis.Text.TextSpan(actionParams.ExtractStart, actionParams.ExtractEnd - actionParams.ExtractStart)).Trim(); |
| @@ -183,4 +183,7 @@ | |||
| <data name="Statement" xml:space="preserve"> | |||
| <value>statement</value> | |||
| </data> | |||
| <data name="ExtractTo_ComponentBehind_Title" xml:space="preserve"> | |||
| <value>Extract element to component behind</value> | |||
There was a problem hiding this comment.
| <value>Extract element to component behind</value> | |
| <value>Extract element to new component</value> |
| return node.DescendantNodes().Any(n => | ||
| n is MarkupBlockSyntax || | ||
| n is CSharpTransitionSyntax || | ||
| n is RazorCommentBlockSyntax); |
There was a problem hiding this comment.
So long as we're nitpicking (and that's all it is! 😄), I'd also make that a one-liner and static lambda.
| return node.DescendantNodes().Any(n => | |
| n is MarkupBlockSyntax || | |
| n is CSharpTransitionSyntax || | |
| n is RazorCommentBlockSyntax); | |
| return node.DescendantNodes().Any(static n => n is MarkupBlockSyntax or CSharpTransitionSyntax or RazorCommentBlockSyntax); |
|
|
||
|
|
||
| _logger = loggerFactory.GetOrCreateLogger<ExtractToNewComponentCodeActionProvider>(); |
There was a problem hiding this comment.
| _logger = loggerFactory.GetOrCreateLogger<ExtractToNewComponentCodeActionProvider>(); | |
| _logger = loggerFactory.GetOrCreateLogger<ExtractToNewComponentCodeActionProvider>(); |
| } | ||
| public Task<IReadOnlyList<RazorVSInternalCodeAction>?> ProvideAsync(RazorCodeActionContext context, CancellationToken cancellationToken) |
There was a problem hiding this comment.
| } | |
| public Task<IReadOnlyList<RazorVSInternalCodeAction>?> ProvideAsync(RazorCodeActionContext context, CancellationToken cancellationToken) | |
| } | |
| public Task<IReadOnlyList<RazorVSInternalCodeAction>?> ProvideAsync(RazorCodeActionContext context, CancellationToken cancellationToken) |
| var componentNode = owner.FirstAncestorOrSelf<MarkupElementSyntax>(); | ||
|
|
||
| // Make sure we've found tag | ||
| if (componentNode is null || componentNode.Kind != SyntaxKind.MarkupElement) |
There was a problem hiding this comment.
Right now you don't need to check the kind because if componentNode is non-null then we know it's MarkupElementSyntax
| if (componentNode is null || componentNode.Kind != SyntaxKind.MarkupElement) | |
| if (componentNode is null) |
| } | ||
| public string Action => LanguageServerConstants.CodeActions.ExtractToNewComponentAction; | ||
| public async Task<WorkspaceEdit?> ResolveAsync(JsonElement data, CancellationToken cancellationToken) |
There was a problem hiding this comment.
| } | |
| public string Action => LanguageServerConstants.CodeActions.ExtractToNewComponentAction; | |
| public async Task<WorkspaceEdit?> ResolveAsync(JsonElement data, CancellationToken cancellationToken) | |
| } | |
| public string Action => LanguageServerConstants.CodeActions.ExtractToNewComponentAction; | |
| public async Task<WorkspaceEdit?> ResolveAsync(JsonElement data, CancellationToken cancellationToken) |
| var actionParams = JsonSerializer.Deserialize<ExtractToNewComponentCodeActionParams>(data.GetRawText()); | ||
|
|
||
| if (actionParams is null) |
There was a problem hiding this comment.
| var actionParams = JsonSerializer.Deserialize<ExtractToNewComponentCodeActionParams>(data.GetRawText()); | |
| if (actionParams is null) | |
| var actionParams = JsonSerializer.Deserialize<ExtractToNewComponentCodeActionParams>(data.GetRawText()); | |
| if (actionParams is null) |
| return null; | ||
| } | ||
|
|
||
| var path = FilePathNormalizer.Normalize(actionParams.Uri.GetAbsoluteOrUNCPath()); |
There was a problem hiding this comment.
I would move this down closer to when it's used. It means we can avoid some work as well if we bail out early
| /// </summary> | ||
| /// <param name="path">The origin file path.</param> | ||
| /// <returns>A non-existent file path with the same base name and a code-behind extension.</returns> | ||
| private static string GenerateComponentBehindPath(string path) |
There was a problem hiding this comment.
We probably should move this to a helper class. I'm assuming extract to code behind also does this?
| /// <param name="path">The origin file path.</param> | ||
| /// <param name="extension">The desired file extension.</param> | ||
| /// <returns>A non-existent file path with a name in the desired format and a corresponding extension.</returns> | ||
| public static string GenerateUniquePath(string path, string extension) |
There was a problem hiding this comment.
It's great that you added a helper method in a shared location, but when doing that it would be good to move the existing call to using it too. So in this case, ExtractToCodeBehindCodeActionResolver should be updated. Essentially, don't be afraid to update things that are strictly speaking outside the scope of the PR, as part of improving the overall code base.
…o general GenerateUniquePath helper
| @@ -33,10 +33,12 @@ public static class CodeActions | |||
| { | |||
| public const string GenerateEventHandler = "GenerateEventHandler"; | |||
|
|
|||
| public const string EditBasedCodeActionCommand = "EditBasedCodeActionCommand"; | |||
| public const string EditBasedCodeActionCommand = nameof(EditBasedCodeActionCommand); | |||
There was a problem hiding this comment.
nit: revert this change :)
| public class ExtractToComponentBehindCodeActionProviderTest(ITestOutputHelper testOutput) : LanguageServerTestBase(testOutput) | ||
| { | ||
| [Fact] | ||
| public async Task Handle_InvalidFileKind() |
There was a problem hiding this comment.
This test looks like it's testing a currently invalid position instead of file kind
There was a problem hiding this comment.
Tests are not fully set up yet, mostly just copied the existing one for Extract To Code. Will keep working on it
| namespace Microsoft.AspNetCore.Razor.LanguageServer.Test.CodeActions.Razor; | ||
| public class ExtractToComponentBehindCodeActionProviderTest(ITestOutputHelper testOutput) : LanguageServerTestBase(testOutput) |
There was a problem hiding this comment.
nit
| namespace Microsoft.AspNetCore.Razor.LanguageServer.Test.CodeActions.Razor; | |
| public class ExtractToComponentBehindCodeActionProviderTest(ITestOutputHelper testOutput) : LanguageServerTestBase(testOutput) | |
| namespace Microsoft.AspNetCore.Razor.LanguageServer.Test.CodeActions.Razor; | |
| public class ExtractToComponentBehindCodeActionProviderTest(ITestOutputHelper testOutput) : LanguageServerTestBase(testOutput) |
| namespace Microsoft.AspNetCore.Razor.LanguageServer.Test.CodeActions.Razor; | ||
| public class ExtractToComponentBehindCodeActionProviderTest(ITestOutputHelper testOutput) : LanguageServerTestBase(testOutput) |
There was a problem hiding this comment.
Would love to see more test cases testing the positive result of the code action. Definitely reach out if you need help getting those set up!
| /// <param name="path">The origin file path.</param> | ||
| /// <param name="extension">The desired file extension.</param> | ||
| /// <returns>A non-existent file path with a name in the desired format and a corresponding extension.</returns> | ||
| public static string GenerateUniquePath(string path, string extension) |
There was a problem hiding this comment.
Is this path expected to be a full file path? If so, would it make sense to check Path.IsPathRooted(path) and throw an ArgumentException if it returns false?
| /// <returns>A non-existent file path with a name in the desired format and a corresponding extension.</returns> | ||
| public static string GenerateUniquePath(string path, string extension) | ||
| { | ||
| var directoryName = Path.GetDirectoryName(path); |
There was a problem hiding this comment.
Rather than calling Assumes.NotNull(directoryName) in every iteration of the loop, consider just calling AssumeNotNull() on Path.GetDirectoryName(path) here:
| var directoryName = Path.GetDirectoryName(path); | |
| var directoryName = Path.GetDirectoryName(path).AssumeNotNull(); |
Then you can remove Assumes.NotNull(directoryName); below.
| /// <param name="path">The origin file path.</param> | ||
| /// <param name="extension">The desired file extension.</param> | ||
| /// <returns>A non-existent file path with a name in the desired format and a corresponding extension.</returns> | ||
| public static string GenerateUniquePath(string path, string extension) |
There was a problem hiding this comment.
From the implementation, it looks like extension is expected to include a leading ".". Could you make sure to include that information in the XML doc comment for this parameter?
| /// Generate a file path with adjacent to our input path that has the | ||
| /// correct file extension, using numbers to differentiate from | ||
| /// any collisions. |
There was a problem hiding this comment.
| /// Generate a file path with adjacent to our input path that has the | |
| /// correct file extension, using numbers to differentiate from | |
| /// any collisions. | |
| /// Generate a file path adjacent to the input path that has the | |
| /// specified file extension, using numbers to differentiate for | |
| /// any collisions. |
| /// correct file extension, using numbers to differentiate from | ||
| /// any collisions. | ||
| /// </summary> | ||
| /// <param name="path">The origin file path.</param> |
There was a problem hiding this comment.
Consider using input rather than origin, since that's what's used in the summary.
| /// <param name="path">The origin file path.</param> | |
| /// <param name="path">The input file path.</param> |
| namespace Microsoft.AspNetCore.Razor.LanguageServer.CodeActions.Razor; | ||
| internal sealed class ExtractToNewComponentCodeActionResolver : IRazorCodeActionResolver | ||
| { | ||
| private static readonly Workspace s_workspace = new AdhocWorkspace(); |
There was a problem hiding this comment.
Please remove this static field. It's not used, and it's actually quite expensive.
| } | ||
|
|
||
| var componentName = Path.GetFileNameWithoutExtension(componentPath); | ||
| var newComponentContent = text.GetSubTextString(new CodeAnalysis.Text.TextSpan(actionParams.ExtractStart, actionParams.ExtractEnd - actionParams.ExtractStart)).Trim(); |
There was a problem hiding this comment.
Is it possible to add a using directive for Microsoft.CodeAnalysis.Text rather than partially qualifying here?
| } | ||
|
|
||
| var path = FilePathNormalizer.Normalize(actionParams.Uri.GetAbsoluteOrUNCPath()); | ||
| var directoryName = Path.GetDirectoryName(path) ?? throw new InvalidOperationException($"Unable to determine the directory name for the path: {path}"); |
There was a problem hiding this comment.
AssumeNotNull() will throw a reasonable InvalidOperationException here.
| var directoryName = Path.GetDirectoryName(path) ?? throw new InvalidOperationException($"Unable to determine the directory name for the path: {path}"); | |
| var directoryName = Path.GetDirectoryName(path).AssumeNotNull(); |
| var newComponentUri = new UriBuilder | ||
| { | ||
| Scheme = Uri.UriSchemeFile, | ||
| Path = updatedComponentPath, | ||
| Host = string.Empty, | ||
| }.Uri; |
There was a problem hiding this comment.
It looks like this UriBuilder pattern is used all over the Razor code base to produce file-schema URIs. If it were me, I would create a global helper and update all the places this pattern is used. However, that's definitely outside of the scope of this PR. Would you mind filing an issue on the repo to clean that tech-debt up?
| var directoryName = Path.GetDirectoryName(path) ?? throw new InvalidOperationException($"Unable to determine the directory name for the path: {path}"); | ||
| var templatePath = Path.Combine(directoryName, "Component"); | ||
| var componentPath = FileUtilities.GenerateUniquePath(templatePath, ".razor"); |
There was a problem hiding this comment.
I'm surprised that calling Path functions on a normalized path successfully. It worries me that we'll hit an issue and it's entirely possible that the final path will have fixed slashes on Windows. cc @davidwengier for his thoughts here.
There was a problem hiding this comment.
Do you mean "mixed" slashes on Windows? I've certainly seen that in other places and it hasn't caused problems, but might be a little riskier here I guess as this is turned in to a Uri and sent back to the client. Maybe the UriBuilder fixes things?
I think there is a good argument that a bunch of this sort of code, which is present in some form here and in the other "Extract" code action, can be moved to a helper class, shared, and get some tests written. To be clear though Hector, that doesn't need to happen in this PR :)
There was a problem hiding this comment.
To be clear though Hector, that doesn't need to happen in this PR :)
Correct! I recognize that this sort of code already exists, and I think it's likely problematic. We should definitely do as you suggest @davidwengier, but also as you say, not in this PR. 😄
|
@dotnet/razor-compiler , PTAL (going into a feature branch) |
| IDocumentContextFactory documentContextFactory, | ||
| LanguageServerFeatureOptions languageServerFeatureOptions) : IRazorCodeActionResolver | ||
| { | ||
|
|
There was a problem hiding this comment.
tiny nit: extra blank line. It's absolutely OK to clean this sort of thing up later. We should get this merged into the feature branch and not block on tiny code style issues.
| var newComponentUri = new UriBuilder | ||
| { | ||
| Scheme = Uri.UriSchemeFile, | ||
| Path = updatedComponentPath, | ||
| Host = string.Empty, | ||
| }.Uri; |
phil-allen-msft
merged commit 8725265
into
dotnet:features/extract-to-component
Summary of the changes
Part of the implementation of the Extract To Component code action. Functional in one of the two cases, when the user is not selecting a certain range of a Razor component, but rather when the cursor is on either the opening or closing tag.