Including @using for Out-of-Scope Razor Component References #10651

marcarro · 2024-07-19T19:57:58Z

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

FROM MERGE

ryzngard

Overall looks fairly good. It's hard to tell what is fixed in a next review vs what I should comment here. Nothing seems blocking yet. There does seem to be a few things I would say about the design that I'm not sure are tested:

It looks for all components (tag helpers) and gets their namespace. How do we know the @using will need to be added? It could be unnecessary (i.e: in _Imports, in a parent namespace, in global usings...)
There was work for identifiers that I didn't understand why was necessary.
There's a few hash sets that may not be necessary, and if they are should likely be pooled since they're all string hash sets

ryzngard · 2024-08-23T23:28:17Z

...oft.AspNetCore.Razor.LanguageServer/CodeActions/Models/ExtractToComponentCodeActionParams.cs

+    public required List<string> Dependencies { get; set; }
+
+    [JsonPropertyName("usedIdentifiers")]
+    public required HashSet<string> UsedIdentifiers { get; set; }


I'll have to look, but do we use HashSet in our serialization objects? It feels weird but if it just serializes/deserializes the same way a list would I guess it would be okay?

ryzngard · 2024-08-23T23:37:19Z

...ft.AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToComponentCodeActionProvider.cs

+        }
+    }
+
+    private static void GetUsedIdentifiers(SyntaxNode divNode, SyntaxNode documentRoot, ExtractToComponentCodeActionParams actionParams)


I'm guessing this is for a future review?

ryzngard · 2024-08-23T23:44:37Z

...ft.AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToComponentCodeActionProvider.cs

+                                        actionParams.ExtractStart,
+                                        actionParams.ExtractEnd,
+                                        actionParams);
+        GetUsedIdentifiers(utilityScanRoot, syntaxTree.Root, actionParams);


It's unclear to me why we need identifiers here for adding usings for components

ryzngard · 2024-08-23T23:51:13Z

...ft.AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToComponentCodeActionProvider.cs

+                        !components.Contains(metadata.Value))
+                    {
+                        components.Add(metadata.Value);
+                        actionParams.Dependencies.Add($"@using {metadata.Value}");


Dependencies is already a hashset correct? Why do we need another one (components)?

ryzngard · 2024-08-23T23:52:00Z

...ft.AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToComponentCodeActionResolver.cs

+internal sealed class ExtractToComponentCodeActionResolver
+    (


Suggested change

internal sealed class ExtractToComponentCodeActionResolver

(

internal sealed class ExtractToComponentCodeActionResolver(

ryzngard · 2024-08-24T00:10:28Z

...t/Microsoft.AspNetCore.Razor.LanguageServer.Test/CodeActions/CodeActionEndToEndTest.NetFx.cs

@@ -1306,4 +1409,78 @@ static IEnumerable<TagHelperDescriptor> BuildTagHelpers()
            }
        }
    }
+
+    private class ExtractToComponentResolverDocumentContextFactory : TestDocumentContextFactory


This file is getting fairly large. Can we make it either a partial class and create CodeActionEndToEndTest.ExtractToComponent.NetFx or make a base class for these?

davidwengier

I don't know if this PR is even worth reviewing or merging, if everything has been removed in a future PR anyway.

Have you considered working on one thing in one branch at a time? 😛

davidwengier · 2024-08-24T11:52:43Z

...ft.AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToComponentCodeActionProvider.cs

+            if (IsMarkupTagHelperElement(node, extractSpan))
+            {
+                var tagHelperInfo = GetTagHelperInfo(node);
+                if (tagHelperInfo is not null)
+                {
+                    AddDependenciesFromTagHelperInfo(tagHelperInfo, components, actionParams);
+                }
+            }


Consider if these two "helper" methods are actually helping. Without them, this whole loop body could be:

if (extractSpan.Contains(node.Span) && node is MarkupTagHelperElementSyntax { TagHelperInfo: { } tagHelperInfo }) { AddDependenciesFromTagHelperInfo(...) }

davidwengier · 2024-08-24T11:54:32Z

...ft.AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToComponentCodeActionProvider.cs

+                    if (metadata.Key == TagHelperMetadata.Common.TypeNamespace &&
+                        metadata.Value is not null &&


If you search for other uses of TagHelperMetadata.Common.TypeNamespace you'll find there are helper methods for most, if not all, of these. eg

razor/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/TagHelperDescriptorExtensions.cs

Line 23 in 3423142

public static string GetTypeNamespace(this TagHelperDescriptor tagHelper)

davidwengier · 2024-08-24T11:55:06Z

...ft.AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToComponentCodeActionProvider.cs

+                {
+                    if (metadata.Key == TagHelperMetadata.Common.TypeNamespace &&
+                        metadata.Value is not null &&
+                        !components.Contains(metadata.Value))


Since components is a hashset, there is no need to check Contains before Add. Just call Add.

davidwengier · 2024-08-24T11:56:47Z

...t/Microsoft.AspNetCore.Razor.LanguageServer.Test/CodeActions/CodeActionEndToEndTest.NetFx.cs

+            """;
+
+        var expectedRazorComponent = """
+            <div id="a">


Shouldn't this have the using statement for Book?

davidwengier · 2024-08-24T11:58:27Z

...ft.AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToComponentCodeActionResolver.cs

@@ -90,7 +91,15 @@ internal sealed class ExtractToNewComponentCodeActionResolver(
        }

        var componentName = Path.GetFileNameWithoutExtension(componentPath);
-        var newComponentContent = text.GetSubTextString(new TextSpan(actionParams.ExtractStart, actionParams.ExtractEnd - actionParams.ExtractStart)).Trim();
+        var newComponentContent = string.Empty;


This should use a StringBuilder, not just concatenate strings, and use a pooled one at that.

davidwengier · 2024-08-24T12:00:24Z

...ft.AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToComponentCodeActionProvider.cs

+                        !components.Contains(metadata.Value))
+                    {
+                        components.Add(metadata.Value);
+                        actionParams.Dependencies.Add($"@using {metadata.Value}");


I don't think Dependencies is the right name for this, though I think perhaps its been removed in a follow up anyway. I would just call it UsingDirectives though. I'd also consider whether its simpler for the Params type to just use a string, but then again if it's been removed in a future PR this whole comment is mute.

This has been renamed to ComponentDependencies

Honesty, that's just leaning in to being incorrect. They were never dependencies, and they certainly aren't components. Objectively this is a list of using directives. Unless those using directives are in the witness protection program, we should call them what they are.

Will do. Was basing variable names off some of the phrasing in the spec.

…ed tests

ryzngard · 2024-08-26T19:52:37Z

...ft.AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToComponentCodeActionProvider.cs

        ProcessSelection(startElementNode, endElementNode, actionParams);

        var utilityScanRoot = FindNearestCommonAncestor(startElementNode, endElementNode) ?? startElementNode;
+
+        // The new component usings are going to be a subset of the usings in the source razor file
+        var usingsInFile = context.SourceText.ToString()


In general in tooling we don't want to read the sourcetext directly to get syntax. It's better to use the syntax tree. In this case it will be a CSharpStatementLiteral where there's a keyword of using in it

the other thing is that the text in the using may not be the full namespace. Since usings are put inside the class they can be relative. So if you have a component MyApp.MyComponents.Header and a component MyApp.HomePage with @using MyComponents then Header is valid to use

https://github.com/dotnet/razor/blob/main/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Extensions/RazorSyntaxNodeExtensions.cs#L14

perfect, I should have checked if there was an extension or helper

ryzngard · 2024-08-26T20:33:18Z

...ft.AspNetCore.Razor.LanguageServer/CodeActions/Razor/ExtractToComponentCodeActionProvider.cs

+            .OfType<CSharpStatementLiteralSyntax>()
+            .Where(literal => literal.SerializedValue.Contains("using"))
+            .Select(literal => literal.SerializedValue)


Close but this is still wrong because this will be true for statements where a variable has using in the name (and worth adding a test for that case). I think this is correct but I'm coding in github so 🤷

Suggested change

.OfType<CSharpStatementLiteralSyntax>()

.Where(literal => literal.SerializedValue.Contains("using"))

.Select(literal => literal.SerializedValue)

.OfType<CSharpStatementLiteralSyntax>()

.Where(statement => statement.LiteralTokens.FirstOrDefault() is SyntaxToken { Kind: SyntaxKind.Keyword } token && CSharpTokenizer.GetTokenKeyword(token) == CSharpSyntaxKind.UsingKeyword))

Ahhh, you're right

…espaces

marcarro requested a review from a team as a code owner July 19, 2024 19:57

marcarro requested a review from a team as a code owner July 30, 2024 16:09

marcarro added 10 commits August 22, 2024 12:53

Arranging selection logic

0471dbf

From merge

Completed and corrected selected range extraction functionality

1b6e035

FROM MERGE

Base component dependency functionality and fixed range selection bug

cba6cf1

FROM MERGE

Include only dependencies included in the selected range

cf33fd7

Test updates

019d6a4

Completed and corrected selected range extraction functionality

dcee923

Nits

6e8d2aa

Added method to scan identifiers

17479d4

Corrected naming and nits

2c4b709

Set up basic end to end test for extract component

5bab548

marcarro force-pushed the includeUsings branch from 21507c8 to 5bab548 Compare August 22, 2024 20:30

Finished fixing rebase stuff

83703f6

ryzngard approved these changes Aug 24, 2024

View reviewed changes

davidwengier approved these changes Aug 24, 2024

View reviewed changes

marcarro added 4 commits August 25, 2024 15:46

Some of the PR feedback that wasn't deferred to the next one, and add…

33c66f4

…ed tests

More PR Feedback

b170f2a

Naming corrections

5b7e0c6

Exclude usings that are already in _Imports.razor

3d9614c

phil-allen-msft removed the request for review from a team August 26, 2024 18:26

Another approach for refining new component usings

d83031b

ryzngard reviewed Aug 26, 2024

View reviewed changes

Use syntaxTree instead of sourceText

1c239a1

ryzngard reviewed Aug 26, 2024

View reviewed changes

marcarro added 2 commits August 26, 2024 17:09

Adjustments to addUsingDirectives and adjusted for relative using nam…

cd677c7

…espaces

Nits. I'm not sure why tests are failing

851fb45

ryzngard merged commit be3686f into dotnet:features/extract-to-component Aug 30, 2024
12 checks passed

This was referenced Oct 22, 2024

[Automated] PRs inserted in VS build main-35421.179 #11056

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35424.209 #11093

Closed

dotnet-bot mentioned this pull request Oct 29, 2024

[Automated] PRs inserted in VS build feature.debugger.shadowDebug-35428.160 #11107

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Including @using for Out-of-Scope Razor Component References #10651

Including @using for Out-of-Scope Razor Component References #10651

marcarro commented Jul 19, 2024 •

edited

Loading

ryzngard left a comment

ryzngard Aug 23, 2024

ryzngard Aug 23, 2024

ryzngard Aug 23, 2024

ryzngard Aug 23, 2024

ryzngard Aug 23, 2024

ryzngard Aug 24, 2024

davidwengier left a comment

davidwengier Aug 24, 2024

davidwengier Aug 24, 2024

davidwengier Aug 24, 2024

davidwengier Aug 24, 2024

davidwengier Aug 24, 2024

davidwengier Aug 24, 2024

marcarro Aug 25, 2024

davidwengier Aug 25, 2024

marcarro Aug 25, 2024

ryzngard Aug 26, 2024

ryzngard Aug 26, 2024

davidwengier Aug 26, 2024

ryzngard Aug 26, 2024

ryzngard Aug 26, 2024

marcarro Aug 26, 2024

	internal sealed class ExtractToComponentCodeActionResolver
	(
	internal sealed class ExtractToComponentCodeActionResolver(

		if (metadata.Key == TagHelperMetadata.Common.TypeNamespace &&
		metadata.Value is not null &&

Including @using for Out-of-Scope Razor Component References #10651

Including @using for Out-of-Scope Razor Component References #10651

Conversation

marcarro commented Jul 19, 2024 • edited Loading

Summary of the changes

Important!

ryzngard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidwengier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcarro commented Jul 19, 2024 •

edited

Loading