Compiler: Cleanup View Components pass and code gen#12174
Compiler: Cleanup View Components pass and code gen#12174DustinCampbell merged 9 commits intodotnet:mainfrom
Conversation
Extract all of the get/set instance property strings to constants on a new ViewComponentsApi class that is modeled on ComponentsApi. Make strings that are unique to this ViewComponentTagHelperTargetExtension private consts.
Use the consts from ViewComponentApis throughout the Version1_X.ViewComponentTagHelperTargetExtension.
Use the consts from ViewComponentApis throughout the Version2_X.ViewComponentTagHelperTargetExtension.
- Enable nullability - Clean up CodeWriterExtensions helpers used to take ImmutableArray<string> and add helpers for writing separated lists - Make methods static - Use CommonModifiers in more places - Stop creating dictionaries unnecessarily - Where appropriate, rename "parameters" to "arguments" - Remove unnecessary argument-null checks
Introduce ViewComponentTagHelperTargetExtensionBase and move most implementation into it. In addition, effort is taken to avoid extra allocations: - Data is cached or created up front and reused. - Overloads have been added to some CodeWriterExtensions helpers to take CodeWriter.WriteInterpolatedStringHandler.
- Enable nullability - Make properties read-only - Add constructor - Remove unnecessary argument-null checks - Small bits of clean up
General clean up with no expected functional change.
| public static ImmutableArray<string> PublicOverrideAsync { get; } = [ | ||
| GetText(CSharpSyntaxKind.PublicKeyword), | ||
| GetText(CSharpSyntaxKind.OverrideKeyword), | ||
| GetText(CSharpSyntaxKind.AsyncKeyword)]; |
There was a problem hiding this comment.
same as previous, just different ordering?
There was a problem hiding this comment.
Correct. I don't want to change the generated output by reordering the modifiers.
| { | ||
| private static readonly ImmutableArray<string> s_publicModifiers = ["public"]; | ||
|
|
||
| public string TagHelperTypeName { get; set; } = "Microsoft.AspNetCore.Razor.TagHelpers.TagHelper"; |
There was a problem hiding this comment.
Were these all settable because they could be customized by users in their csproj somehow?
No wait, don't tell me, I don't want to know.
There was a problem hiding this comment.
No, it's not that interesting. I've found several spots in the compiler that do this, and the only reason I've found is to be able to give them different values in tests. However, the value of doing that is pretty flimsy. It allows the test to ensure that, "yes, this really did run". However, it means that the test is validating invalid compiler output. In my opinion, it's better to protect these values from being written to and ensure they're all shared.
| var className = $"__Generated__{viewComponentName}ViewComponentTagHelper"; | ||
| var fullyQualifiedName = !Namespace.Name.IsNullOrEmpty() | ||
| ? $"{Namespace.Name}.{Class.Name}.{className}" | ||
| : $"{Namespace.Name}{Class.Name}.{className}"; |
There was a problem hiding this comment.
Because that's what the code already produced. I'm trying not to change behavior.
|
@chsienki, PTAL |
5 participants
This change reworks a lot of the code generation for view component tag helpers. Code that's duplicated across the MVC versions has been consolidated and extra effort has been taken to reduce allocations and share common strings. The commits tell a story if you choose to review commit-by-commit.
CI Build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2785649&view=results
Test Insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/666958
Toolset Run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2785650&view=results