Extract To Component: Code handling capabilities#10760
Extract To Component: Code handling capabilities#10760marcarro wants to merge 29 commits intodotnet:features/extract-to-componentfrom
Conversation
### Summary of the changes Addition to feature wherein razor component dependencies are now identified, and their corresponding `@usings` are now also extracted into the new document. ## **_Important!_** Notes: Some changes in this PR are further rectified in #10760, such as: `AddComponentDependenciesInRange` does not take actionParams anymore. In general, most (if not all) of the internal methods that take `actionParams` as a parameter were moved to the resolver, where each method returns a collection of appropiate objects.
From merge
…ribute promotion functionality, Resolver now handles event handlers and data binding
Integrated previous PR feedback into this branch.
marcarro
force-pushed
the
includeCode
branch
from
edbb1b4 to
5b76f23
Compare
ryzngard
left a comment
ryzngard
left a comment
There was a problem hiding this comment.
Stopped at ExtractToComponentCodeAction. There seems to be some duplicate work going on to walk the syntax tree that can be reduced.
| return context is not null && | ||
| context.SupportsFileCreation && | ||
| FileKinds.IsComponent(context.CodeDocument.GetFileKind()) && | ||
| context.CodeDocument.GetSyntaxTree()?.Root is not null; |
There was a problem hiding this comment.
nit: RazorSyntaxTree.Root can't be null, so just need to check if the syntax tree is null
| public Task<ImmutableArray<RazorVSInternalCodeAction>> ProvideAsync(RazorCodeActionContext context, CancellationToken cancellationToken) | ||
| { | ||
| if (context is null) | ||
| if (!IsValidContext(context)) |
| { | ||
| return null; | ||
| } | ||
| var tryMakeMarkupElement = owner.FirstAncestorOrSelf<MarkupElementSyntax>(); |
There was a problem hiding this comment.
You're walking the tree twice. I would suggest doing
var (startTag, endTag) = GetStartAndEndTag(owner);
if (startTag is null || endTag is null)
{
return false;
}
return startTag.Span.Contains(context.Location.AbsoluteIndex) || endTag.Span.Contains(context.Location.AbsoluteIndex);
static (MarkupTagHelperStartTagSyntax? startTag, MarkupTagHelperStartTagSyntax? endTag) GetStartAndEndTag(owner)
{
var potentialElement = owner.FirstAncestor<MarkupSyntaxNode>(e => e is MarkupElementSyntax || MarkupTagHelperElementSyntax);
return potentialElement switch
{
MarkupElementSyntax markupElement => (markupElement.StartTag, markupElement .EndTag),
MarkupTagHelperElementSyntax tagHelper => (tagHelper.StartTag, tagHelper.EndTag),
_ => (null, null);
}
}
There was a problem hiding this comment.
I did try doing this before, but MarkupElementSyntax is inconvertible to MarkupTagHelperElementSyntax, along with their start and end tags. This is why I'm doing two separate variables for it. I think I can walk up the tree once to get them both, though.
There was a problem hiding this comment.
This isn't trying to convert between the two, it's just getting the start/end tags. Although the return type should probably be (MarkupSyntaxNode? startTag, MarkupSyntaxNode? endTag) but that's what I get for coding on GitHub :P
| return !diagnostics.Any(d => d.Severity == RazorDiagnosticSeverity.Error); | ||
| } | ||
|
|
||
| private static bool TryGetNamespace(RazorCodeDocument codeDocument, [NotNullWhen(returnValue: true)] out string? @namespace) |
There was a problem hiding this comment.
Looks like this can be removed and the extension method can be used directly
| using Microsoft.CodeAnalysis.Razor.Protocol; | ||
| using Microsoft.VisualStudio.LanguageServer.Protocol; | ||
| using Newtonsoft.Json.Linq; | ||
| using static System.Net.Mime.MediaTypeNames; |
| var codeDocument = await documentContext.GetCodeDocumentAsync(cancellationToken).ConfigureAwait(false); | ||
| var syntaxTree = codeDocument.GetSyntaxTree(); | ||
|
|
||
| if (codeDocument.IsUnsupported()) |
There was a problem hiding this comment.
this seems like something that should go into the provider
|
|
||
| var componentDocument = await documentContext.GetCodeDocumentAsync(cancellationToken).ConfigureAwait(false); | ||
| if (componentDocument.IsUnsupported()) | ||
| if (!FileKinds.IsComponent(codeDocument.GetFileKind())) |
There was a problem hiding this comment.
This should always be true based on the provider check
|
|
||
| private static ExtractToComponentCodeActionParams? DeserializeActionParams(JsonElement data) | ||
| { | ||
| return data.ValueKind == JsonValueKind.Undefined |
There was a problem hiding this comment.
Is this ever possible? Seems like an exception
|
|
||
| private static (MarkupSyntaxNode? Start, MarkupSyntaxNode? End) GetStartAndEndElements(SourceText sourceText, RazorSyntaxTree syntaxTree, ExtractToComponentCodeActionParams actionParams) | ||
| { | ||
| if (syntaxTree is null) |
There was a problem hiding this comment.
According to the signature this shouldn't be null ever
| return (null, null); | ||
| } | ||
|
|
||
| var owner = syntaxTree.Root.FindInnermostNode(actionParams.AbsoluteIndex, includeWhitespace: true); |
There was a problem hiding this comment.
We're doing this twice. Why can't we provide the correct way to get the start and end in the provider? We already have to check the nodes there anyways
There was a problem hiding this comment.
Would it be an improvement if I pass in the spans of the start and element nodes to the resolver and use FindNode() on those spans?
If not, I think the way to go would be to move all of the TryAnalyzeSelection() stuff to the provider.
There was a problem hiding this comment.
I would say any analysis of validation of selection should always go in the provider, and the action of determining work beyond that is in the resolver. Keep in mind failure in a resolver means that an action was shown and just fails to apply for some reason and isn't as obvious to a user.
There was a problem hiding this comment.
Yep. Working on moving selection validation to Provider again
Intended previous commit message: Telemetry was added to provider in previous commit, and cleaned up usings. This commit: Integrated PR Feedback for IsInsideMarkupTag
…ntaxTree to scan for using directives
|
I was able to extract component on
overall the feature works fine in many cases I tried that would be great too also, I noticed if you try to extract on @inject WeatherApiClient WeatherApithe extract component code action does not appear. I wonder what it would take to enable that NOTE (update)
|
…reduce duplicate work
3 participants
Summary of the changes
Added base support for code in Razor elements. Specifically, the feature now automatically parametrizes methods when applicable, and extracts any appropriate fields.
Also includes refactoring of previously done work, which shifts complex responsibilities to the code action resolver (i.e. the provider was doing things like scanning through the syntax tree for the dependencies. These and others were moved.)
Note: Added work includes an API call to a roslyn endpoint which has not been pushed to as of writing.