Dedupe metadata in tag helpers #8782

DustinCampbell · 2023-05-31T22:29:07Z

This change provides an alternative API to add metadata to tag helper builder objects. The existing API allows key/value pairs to be added to an IDictionary<string, string?>, which is then converted to a MetadataCollection. The new API simply takes a MetadataCollection directly, which allows common instances to be shared across tag helpers.

A couple of notes:

I've had to make a few APIs public so that they can be called from the "MVC Extensions" projects. However, the expectation is that these aren't really a problem since they won't ever be shipped on NuGet.
There are loads of test changes here to avoid calling the existing metadata API on builders.

davidwengier

Makes sense to me, and tooling side looks good.

Would be interesting to see how many PropertyName metadata items share the same names, in a real app. I suspect there would be a lot of common attribute descriptors (Value, Items, Title etc.), it might be worth using a dictionary to share KVP instances.

DustinCampbell · 2023-06-01T19:52:08Z

it might be worth using a dictionary to share KVP instances.

It might be useful to intern the strings, but KVPs are structs, so they can't exactly be shared. FWIW, the biggest offenders were the many RequiredAttributeDescriptors with identical "IsDirectiveAttribute" metadata.

chsienki

LGTM. @333fred Have we decided if we're doing API review for Razor too?

src/Compiler/Microsoft.AspNetCore.Razor.Language/src/CommonMetadata.cs

DustinCampbell · 2023-06-02T21:02:51Z

LGTM. @333fred Have we decided if we're doing API review for Razor too?

I don't want to put words in his mouth, but @333fred and I chatted about this the other day, and I believe the conclusion was that we have to review the whole thing eventually. However, we don't need to do that yet. He'll correct me if I got that wrong.

FWIW, I made these public to avoid having to add InternalsVisibleTo for the MVC Extensions projects, which I think we don't want.

jaredpar

Got most of the way through the change. Reviewed the core implementation changes and probably half the test cases. Had a few Qs. Will pick up rest of the review later today

src/Compiler/Microsoft.AspNetCore.Razor.Language/src/BoundAttributeDescriptorBuilder.cs

...Compiler/Microsoft.AspNetCore.Razor.Language/src/BoundAttributeParameterDescriptorBuilder.cs

jaredpar · 2023-06-02T20:19:47Z

...soft.AspNetCore.Razor.Language/src/Components/TagHelperBoundAttributeDescriptorExtensions.cs

 internal static class TagHelperBoundAttributeDescriptorExtensions
 {
+    private static bool IsTrue(this BoundAttributeDescriptor attribute, string key)
+        => attribute.Metadata.TryGetValue(key, out var value) && value == bool.TrueString;


This PR effectively repeats a number of helpers between BoundAttributeDescriptor and BoundAttributeDescriptorBuilder. Did you consider using an interface for these common methods so we could have a single IsTrue, SetMetadata, etc ... methods?

I didn't consider that, no. There are loads of these helper extension methods (mostly public) all over the place, so I decided to just follow that pattern. When we can finally change the public surface area of tag helper descriptors, I want to revisit all of this.

Also, providing an interface wouldn't give us a single copy of the methods. We would have to copy the implementations to each builder type because there's no hierarchy relationship between them.

src/Compiler/Microsoft.AspNetCore.Razor.Language/src/MetadataBuilder.cs

src/Compiler/Microsoft.AspNetCore.Razor.Language/src/MetadataCollection.Enumerator.cs

src/Compiler/Microsoft.AspNetCore.Razor.Language/src/RequiredAttributeDescriptorBuilder.cs

src/Compiler/Microsoft.AspNetCore.Razor.Language/src/TagHelperDescriptorBuilder.cs

...r/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Completion/TagHelperServiceTestBase.cs

...e.Razor.LanguageServer.Test/DocumentPresentation/TextDocumentUriPresentationEndpointTests.cs

...pNetCore.Mvc.Razor.Extensions.Version1_X/test/ViewComponentTagHelperDescriptorFactoryTest.cs

333fred · 2023-06-02T21:07:43Z

Nope, you're right. I do owe you a review on this that I've started, and will try to get through the rest of today.

src/Compiler/Microsoft.AspNetCore.Razor.Language/test/BoundAttributeDescriptorExtensionsTest.cs

333fred

Finished review of the compiler layer. Overall looking good, just a few minor questions.

src/Compiler/Microsoft.AspNetCore.Razor.Language/src/TagHelperDescriptorBuilderExtensions.cs

src/Compiler/Microsoft.AspNetCore.Razor.Language/src/MetadataHolder.cs

src/Compiler/Microsoft.AspNetCore.Razor.Language/src/Resources.resx

...iler/Microsoft.AspNetCore.Razor.Language/src/RequiredAttributeDescriptorBuilderExtensions.cs

src/Compiler/Microsoft.AspNetCore.Razor.Language/src/MetadataHolder.cs

src/Compiler/Microsoft.AspNetCore.Razor.Language/src/MetadataBuilder.cs

src/Compiler/Microsoft.AspNetCore.Razor.Language/src/DefaultTagHelperDescriptorBuilder.cs

DustinCampbell requested review from a team as code owners May 31, 2023 22:29

davidwengier approved these changes Jun 1, 2023

View reviewed changes

DustinCampbell force-pushed the dedupe-metadata branch from 6bef35d to 280c6a1 Compare June 1, 2023 20:05

chsienki reviewed Jun 2, 2023

View reviewed changes

src/Compiler/Microsoft.AspNetCore.Razor.Language/src/CommonMetadata.cs Outdated Show resolved Hide resolved

chsienki approved these changes Jun 2, 2023

View reviewed changes

jaredpar reviewed Jun 2, 2023

View reviewed changes

333fred reviewed Jun 5, 2023

View reviewed changes

DustinCampbell added 10 commits June 6, 2023 08:40

Add internal API to tag helper builders for setting metadata

d1a29e5

Share metadata across BoundAttributeParameterDescriptors

d5a534e

Share metadata across BoundAttributeDescriptors

b076c21

Share metadata across RequiredAttributeDescriptors

be0dd83

Share metadata across TagHelperDescriptors

b2cbb6d

Add a handful of unit tests

50c3b2b

Unify metadata production code

242a288

Rename CommonMetadata.IsTrue to MakeTrue

f51cd0d

Mark a few APIs as #nullable enable

d7b9b5c

Fix a couple of issues in tooling tests

a8c6fe3

DustinCampbell force-pushed the dedupe-metadata branch from 280c6a1 to a8c6fe3 Compare June 6, 2023 16:34

DustinCampbell added 2 commits June 6, 2023 09:40

Fix resource string

27a21f7

Remove unused API

7e7a01d

jaredpar approved these changes Jun 6, 2023

View reviewed changes

333fred approved these changes Jun 6, 2023

View reviewed changes

DustinCampbell merged commit 762c615 into dotnet:main Jun 7, 2023

DustinCampbell deleted the dedupe-metadata branch June 7, 2023 00:12

allisonchou mentioned this pull request Jun 30, 2023

[Automated] PRs inserted in VS build main-33829.535 #8893

Closed

Dedupe metadata in tag helpers #8782

Dedupe metadata in tag helpers #8782

Uh oh!

Conversation

DustinCampbell commented May 31, 2023

Uh oh!

davidwengier left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DustinCampbell commented Jun 1, 2023

Uh oh!

chsienki left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DustinCampbell commented Jun 2, 2023

Uh oh!

jaredpar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jaredpar Jun 2, 2023

Choose a reason for hiding this comment

Uh oh!

DustinCampbell Jun 6, 2023

Choose a reason for hiding this comment

Uh oh!

DustinCampbell Jun 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

333fred commented Jun 2, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

davidwengier left a comment •

edited

Loading