Add ITagHelperComponentManager #6302

jbagga · 2017-05-20T00:26:21Z

Addresses #6282

sebastienros · 2017-05-20T00:29:52Z

src/Microsoft.AspNetCore.Mvc.Razor/TagHelpers/TagHelperComponentTagHelper.cs

            }

            if (loggerFactory == null)
            {
                throw new ArgumentNullException(nameof(loggerFactory));
            }

-            _components = components;
+            _components = new List<ITagHelperComponent>(manager.Components.OrderBy(p => p.Order));


manager.Components.OrderBy(p => p.Order).ToArray()

Why ToArray()

The ToList() operation is more readable.
Then there is no need for a list, hence ToArray

Ah, I thought this was the provider itself. This is the taghelper.

sebastienros · 2017-05-20T00:33:08Z

src/Microsoft.AspNetCore.Mvc.Razor/TagHelpers/ITagHelperComponentManager.cs

+
+namespace Microsoft.AspNetCore.Mvc.Razor.TagHelpers
+{
+    public interface ITagHelperComponentManager


It will need comments.

rynowak · 2017-05-22T15:40:13Z

src/Microsoft.AspNetCore.Mvc.Razor/TagHelpers/TagHelperComponentManager.cs

+        public void Add(ITagHelperComponent tagHelperComponent)
+        {
+            _tagHelperComponents.Add(tagHelperComponent);
+        }


Why not IList<> for the components? Is is not supported to remove components?

I can see two sensible ways that ITagHelperManager deals with ordering in a consistent way.

Expose IList<> for the components. The components from DI are always put in order for you. Callers can add or insert a component wherever they want, reorder the list, remove stuff, etc.

Expose IEnumerable<> or ICollection<>. Calling add should find the right place in the list for the new component based on its order and insert it. Use ICollection<> if you want to support removing things or IEnumerable<> if you don't.

jbagga · 2017-05-22T19:03:29Z

@sebastienros @rynowak Updated

rynowak · 2017-05-22T21:11:51Z

src/Microsoft.AspNetCore.Mvc.Razor/TagHelpers/ITagHelperComponentManager.cs

+{
+    /// <summary>
+    /// Contract used to manage <see cref="ITagHelperComponent"/>s.
+    /// </summary>


This isn't super useful, what can you do with it?

rynowak · 2017-05-22T21:12:23Z

src/Microsoft.AspNetCore.Mvc.Razor/TagHelpers/TagHelperComponentManager.cs

+using System.Collections.ObjectModel;
+using Microsoft.AspNetCore.Razor.TagHelpers;
+
+namespace Microsoft.AspNetCore.Mvc.Razor.TagHelpers


This should be in a more secretive namespace like .Internal, there's no need for anyone to interact with this type.

rynowak

from me after addressing small comments. Get a from 🇫🇷 as well

dougbu · 2017-05-22T21:55:50Z

src/Microsoft.AspNetCore.Mvc.Razor/Internal/TagHelperComponentManager.cs

+        }
+
+        /// <inheritdoc />
+        public ICollection<ITagHelperComponent> Components { get; private set; }


Suggest IList<T> since ICollection<T> is a loser interface.

remove the private set; bit

Oops, missed @rynowak's comment about the chosen collection type. We obviously want to Add() so IEnumerable<T> is out.

The main differences between ICollection<T> and IList<T> are odd gaps in ICollection<T> e.g. no RemoveAt(). But, both support Remove() and I lean toward IList<T> here.

dougbu · 2017-05-22T22:01:48Z

test/WebSites/RazorWebSite/Services/TestBodyTagHelperComponent.cs

@@ -15,7 +15,7 @@ public override Task ProcessAsync(TagHelperContext context, TagHelperOutput outp
        {
            if (string.Equals(context.TagName, "body", StringComparison.Ordinal) && output.Attributes.ContainsName("inject"))
            {
-                output.PostContent.AppendHtml("<script>'This was injected!!'</script>");
+                output.PostContent.AppendHtml("<script>'This was injected!!'</script>\r\n");


Do not add Windows-specific line endings. My preference is to leave this unchanged because this changes the baselines for no reason.

Same for TestHeadTagHelperComponent

dougbu · 2017-05-22T22:05:05Z

....AspNetCore.Mvc.FunctionalTests/compiler/resources/RazorWebSite.TagHelperComponent.Body.html

+<script>'This was injected!!'</script>
+</body>


Confusing that this and RazorWebSite.TagHelperComponent.Head.html changed at all. As mentioned later, revert the .cshtml changes.

dougbu · 2017-05-22T22:05:48Z

test/Microsoft.AspNetCore.Mvc.Razor.Test/TagHelpers/TagHelperComponentTagHelperTest.cs

            {
-                new CallbackTagHelperComponent(
+               new CallbackTagHelperComponent(


Restore the leading space.

dougbu · 2017-05-22T22:13:21Z

src/Microsoft.AspNetCore.Mvc.Razor/TagHelpers/TagHelperComponentTagHelper.cs

            }

            if (loggerFactory == null)
            {
                throw new ArgumentNullException(nameof(loggerFactory));
            }

-            _components = components;
+            _components = manager.Components.OrderBy(p => p.Order).ToArray();


Add a comment mentioning this ordering must be done after the manager is created (otherwise controller-added components won't be positioned correctly).

jbagga · 2017-05-23T01:33:49Z

@dougbu Updated

dougbu

Getting close. Sorry if I missed a couple of things in my previous read-through.

dougbu · 2017-05-23T02:15:53Z

src/Microsoft.AspNetCore.Mvc.Razor/TagHelpers/BodyTagHelper.cs

@@ -18,10 +18,11 @@ public class BodyTagHelper : TagHelperComponentTagHelper
        /// <summary>
        /// Creates a new <see cref="BodyTagHelper"/>.
        /// </summary>
-        /// <param name="components">The list of <see cref="ITagHelperComponent"/>.</param>
+        /// <param name="manager">The <see cref="ITagHelperComponentManager"/> which contains the list


"contains the list" -> "contains the collection" throughout this PR. (There's a few left after your comment updates.)

dougbu · 2017-05-23T02:18:01Z

src/Microsoft.AspNetCore.Mvc.Razor/TagHelpers/ITagHelperComponentManager.cs

+{
+    /// <summary>
+    /// An implementation of this interface provides the collection of <see cref="ITagHelperComponent"/>s
+    /// that will be used by the <see cref="TagHelperComponentTagHelper"/>.


Minor but <see cref="TagHelperComponentTagHelper"/>s is better than the <see cref="TagHelperComponentTagHelper"/> because there isn't just one <see cref="TagHelperComponentTagHelper"/>.

dougbu · 2017-05-23T02:20:24Z

src/Microsoft.AspNetCore.Mvc.Razor/TagHelpers/TagHelperComponentTagHelper.cs

            }

            if (loggerFactory == null)
            {
                throw new ArgumentNullException(nameof(loggerFactory));
            }

-            _components = components;
+            _components = manager.Components.OrderBy(p => p.Order).ToArray();


dougbu · 2017-05-23T02:24:47Z

test/Microsoft.AspNetCore.Mvc.Razor.Test/TagHelpers/TagHelperComponentTagHelperTest.cs

+
+            public void Init(TagHelperContext context)
+            {
+                context.Items["Key2"] = "Value2";


What is this Items entry used for? Same for the similar code in later test components.

dougbu · 2017-05-23T02:27:54Z

test/Microsoft.AspNetCore.Mvc.Razor.Test/TagHelpers/TagHelperComponentTagHelperTest.cs

+
+        private class TestAddTagHelperComponent : ITagHelperComponent
+        {
+            public int Order => 0;


Suggest making this settable and including it in what's added to PostContent. Then, can test that controller-added components are processed in the correct order.

dougbu · 2017-05-23T02:31:40Z

test/WebSites/RazorWebSite/RazorWebSite.csproj

    <ProjectReference Include="..\Microsoft.AspNetCore.Mvc.TestConfiguration\Microsoft.AspNetCore.Mvc.TestConfiguration.csproj" />

+    <PackageReference Include="Microsoft.AspNetCore.Razor.Runtime" Version="$(AspNetCoreVersion)" />


Remove these dependencies; Microsoft.AspNetCore.Mvc brings them in.

dougbu · 2017-05-23T02:34:54Z

test/WebSites/RazorWebSite/Services/TestViewTagHelperComponent.cs

+
+namespace RazorWebSite
+{
+    public class TestViewTagHelperComponent : ITagHelperComponent


Not quite sure why we've got multiple almost-identical components in this web site. Couldn't the bits that differ e.g. the appended content be passed into the constructor or (hmm, Order) set?

jbagga · 2017-05-23T17:41:51Z

@dougbu Updated

dougbu · 2017-05-23T18:59:29Z

test/Microsoft.AspNetCore.Mvc.Razor.Test/TagHelpers/TagHelperComponentTagHelperTest.cs


            // Act
            await testTagHelperComponentTagHelper.ProcessAsync(tagHelperContext, output);

            // Assert           
-            Assert.Equal("Processed", output.PostContent.GetContent());
+            Assert.Equal("Processed0Processed1Processed2", output.PostContent.GetContent());


dnfclas added the cla-already-signed label May 20, 2017

jbagga requested review from sebastienros and NTaylorMullen May 20, 2017 00:26

jbagga assigned NTaylorMullen May 20, 2017

sebastienros reviewed May 20, 2017

View reviewed changes

rynowak reviewed May 22, 2017

View reviewed changes

rynowak approved these changes May 22, 2017

View reviewed changes

sebastienros approved these changes May 22, 2017

View reviewed changes

dougbu suggested changes May 22, 2017

View reviewed changes

jbagga force-pushed the jbagga/ITagHelperComponents6282 branch from 67e1e9d to 8f343be Compare May 23, 2017 01:33

dougbu suggested changes May 23, 2017

View reviewed changes

dougbu approved these changes May 23, 2017

View reviewed changes

jbagga added 5 commits May 23, 2017 12:05

Add ITagHelperComponentManager

1a11ee5

Address feedback

90d7acc

Modify docs; change namespace

e525c32

Formatting and comments

431ef19

simplify test classes

a92f25c

jbagga force-pushed the jbagga/ITagHelperComponents6282 branch from b1cef98 to a92f25c Compare May 23, 2017 19:06

jbagga merged commit e681c23 into dev May 23, 2017

jbagga deleted the jbagga/ITagHelperComponents6282 branch May 23, 2017 20:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ITagHelperComponentManager #6302

Add ITagHelperComponentManager #6302

jbagga commented May 20, 2017

sebastienros May 20, 2017

davidfowl May 20, 2017

sebastienros May 20, 2017

davidfowl May 20, 2017 •

edited

Loading

sebastienros May 20, 2017

rynowak May 22, 2017

jbagga commented May 22, 2017

rynowak May 22, 2017

rynowak May 22, 2017

rynowak left a comment

dougbu May 22, 2017

dougbu May 22, 2017

dougbu May 22, 2017

dougbu May 22, 2017

dougbu May 22, 2017

dougbu May 22, 2017

dougbu May 22, 2017

dougbu May 23, 2017

jbagga commented May 23, 2017

dougbu left a comment

dougbu May 23, 2017

dougbu May 23, 2017

dougbu May 23, 2017

dougbu May 23, 2017

dougbu May 23, 2017

dougbu May 23, 2017

dougbu May 23, 2017

jbagga commented May 23, 2017

dougbu May 23, 2017

		<ProjectReference Include="..\Microsoft.AspNetCore.Mvc.TestConfiguration\Microsoft.AspNetCore.Mvc.TestConfiguration.csproj" />

		<PackageReference Include="Microsoft.AspNetCore.Razor.Runtime" Version="$(AspNetCoreVersion)" />

		<script>'This was injected!!'</script>
		</body>

Add ITagHelperComponentManager #6302

Add ITagHelperComponentManager #6302

Conversation

jbagga commented May 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl May 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbagga commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak 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

jbagga commented May 23, 2017

dougbu 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

jbagga commented May 23, 2017

Choose a reason for hiding this comment

davidfowl May 20, 2017 •

edited

Loading