Introduce ComponentTagHelper #14592

pranavkm · 2019-09-30T22:45:28Z

src/Components/Samples/BlazorServerApp/Pages/_Host.cshtml

src/Components/test/E2ETest/ServerExecutionTests/ComponentWithParametersTest.cs

rynowak · 2019-09-30T23:34:40Z

src/Components/test/testassets/TestServer/Pages/ComponentWithParameters.cshtml

+<component type="typeof(ComponentWithParameters)"
+           render-mode="ServerPrerendered"
+           parameter-Param1="ComponentWithParameters.TestModelValues"
+           parameter-Param2="ComponentWithParameters.DerivedModelValue"


This is kinda an interesting case. It's interesting to think about because the same is not true with Param1. If you put an instance of DerivedModel in that list, it wouldn't roundtrip.

I think is is probably the right thing to do but I'm sure it's going to come up. This use case for serialization is really different from our usage elsewhere. We'll have to make sure to write good docs for this - the main thing is that it's not really possible for people to marshal some kinds of data (like polymorphic collections) without doing a lot of S.T.J extensibility.

Yup. I wanted to at least capture some of the ordinary cases. The experience is generally going to be bad with polymorphism. Dictionary<string, object> for instance are going to round trip as JsonElement instances. I'll make sure to specifically say that the implementation uses S.T.J to marshal the values so we recommend simple values that are easy to round trip, don't exceed the size limits, etc

rynowak · 2019-09-30T23:35:56Z

src/Mvc/Mvc.TagHelpers/src/ComponentTagHelper.cs

+    /// <summary>
+    /// A <see cref="TagHelper"/> that renders a Blazor component.
+    /// </summary>
+    [HtmlTargetElement("component", Attributes = ComponentTypeName)]


should we enforce the style here?

"Razor component" vs. "Blazor component"?
Technically, as it is right now, they're Razor Components.

rynowak · 2019-09-30T23:36:44Z

src/Mvc/Mvc.TagHelpers/src/ComponentTagHelper.cs

+            }
+
+            var componentRenderer = ViewContext.HttpContext.RequestServices.GetRequiredService<IComponentRenderer>();
+            var result = await componentRenderer.RenderComponentAsync(ViewContext, ComponentType, RenderMode, Parameters);


Do you want to try and prevent the dictionary allocation here?

rynowak · 2019-09-30T23:37:13Z

src/Mvc/Mvc.ViewFeatures/src/HtmlHelperComponentExtensions.cs

-    /// <summary>
-    /// Extensions for rendering components.
-    /// </summary>
-    public static class HtmlHelperComponentExtensions


just a folder move, ok.

rynowak · 2019-09-30T23:37:58Z

src/Mvc/Mvc.ViewFeatures/src/RazorComponents/ComponentRenderer.cs

+using Microsoft.AspNetCore.Http;
+using Microsoft.AspNetCore.Mvc.Rendering;
+
+namespace Microsoft.AspNetCore.Mvc.ViewFeatures.RazorComponents


do we need this new namespace? .ViewFeatures is already kinda a junk namespace.

Yeah, we could clean it up. I thought everything else in that directory was intentionally under the namespace, but it's just this type: https://github.com/aspnet/AspNetCore/blob/master/src/Mvc/Mvc.ViewFeatures/src/RazorComponents/StaticComponentRenderer.cs#L18

rynowak · 2019-09-30T23:39:06Z

src/Mvc/Mvc.ViewFeatures/src/RazorComponents/IComponentRenderer.cs

+
+namespace Microsoft.AspNetCore.Mvc.ViewFeatures.RazorComponents
+{
+    internal interface IComponentRenderer


What do we get out of this? If we just called the HTML helper from the tag helper, what are the problmes?

Assuming we at least add the non-generic overload to the HTML helper's existing method right?

Having the things as extensions makes it really difficult to test. Also the extension method is just a place to dump code. Outside of using the ViewContext, none of the IHtmlHelper is used.

Can we at least get rid of the interface and use an abstract class instead?

ryanbrandenburg

Neat.

src/Mvc/Mvc.TagHelpers/src/ComponentTagHelper.cs

src/Mvc/samples/MvcSandbox/Pages/Components.cshtml

src/Mvc/samples/MvcSandbox/Views/Shared/_Layout.cshtml

rynowak · 2019-10-02T22:47:43Z

src/Mvc/Mvc.ViewFeatures/src/Rendering/HtmlHelperComponentExtensions.cs

+            var componentRenderer = viewContext.HttpContext.RequestServices.GetRequiredService<IComponentRenderer>();
+            return componentRenderer.RenderComponentAsync(viewContext, typeof(TComponent), renderMode, parameters);
+        }
+    }


We should also add non-generic overloads. The HTML helper is a little too limiting.

I thought about it, but in general we wouldn't encourage users to use the html helper if there's an equally capable tag helper available. Do you feel strongly about adding the additional overload?

I don't really care that much about the TagHelper, but it should be uber cheap to add this, so I'm with @rynowak on this. There's not even need to add or change tests for them, simply chain the implementations. I trust you know what to do :)

rynowak · 2019-10-02T22:48:34Z

src/Mvc/Mvc.TagHelpers/src/ComponentTagHelper.cs

+    /// A <see cref="TagHelper"/> that renders a Razor component.
+    /// </summary>
+    [HtmlTargetElement("component", Attributes = ComponentTypeName, TagStructure = TagStructure.WithoutEndTag)]
+    public class ComponentTagHelper : TagHelper


Should we seal? Would this do something reasonable if someone inherited? I don't have a strong feeling either way.

rynowak · 2019-10-02T22:49:24Z

src/Mvc/Mvc.TagHelpers/src/ComponentTagHelper.cs

+                throw new ArgumentNullException(nameof(output));
+            }
+
+            var componentRenderer = ViewContext.HttpContext.RequestServices.GetRequiredService<IComponentRenderer>();


Should this be a constructor parameter?

I didn't want to make the utility type public as yet, at least until we got enough feedback about how users want to use it and how it fits for the client scenarios that were punted until 5.1. This forces the interface to not be a ctor parameter.

pranavkm · 2019-10-09T21:53:10Z

This is updated

javiercn · 2019-10-11T14:58:05Z

src/Components/test/E2ETest/ServerExecutionTests/ComponentWithParametersTest.cs

+        }
+
+        [Fact]
+        public void PassingParametersToComponentsWorks()


There's already an E2E test for this. Could you rename it to make it clear that this is testing inheritance?

javiercn · 2019-10-11T15:03:36Z

src/Components/test/E2ETest/ServerExecutionTests/ComponentWithParametersTest.cs

+            var parameter2 = Browser.FindElement(By.CssSelector(".Param2"));
+            Assert.Equal("Value Derived-Value", parameter2.Text);
+
+            // This check verifies CaptureUnmatchedValues works


Why are we validating this here, it feels completely unrelated to the feature or to the ability to pass parameters to the root component?

Steve noted in the other PR - #14465 (comment), that we want to ensure we have the ability to pass CaptureUnmatchedValues around. I wanted to make sure we covered that in the context of using the tag helper.

javiercn · 2019-10-11T15:12:34Z

src/Components/test/testassets/TestServer/Pages/MultipleComponents.cshtml

                <p>Some content after</p>
            </div>
        </div>
        <div id="container">
-            @(await Html.RenderComponentAsync<GreeterComponent>(RenderMode.Server, new { Name = "Albert" }))
-            @(await Html.RenderComponentAsync<GreeterComponent>(RenderMode.ServerPrerendered, new { Name = "Abraham" }))
+            <component type="typeof(GreeterComponent)" render-mode="Server" param-name='"Albert"' />


Leave some HtmlHelper instances around so that we can validate they keep working.

javiercn · 2019-10-11T15:13:28Z

src/Components/test/testassets/TestServer/Pages/PrerenderedHost.cshtml

@@ -1,13 +1,14 @@
 @page
 @using BasicTestApp.RouterTest
+


nit: unnecessary white-space.

javiercn · 2019-10-11T15:20:55Z

src/Mvc/Mvc.ViewFeatures/src/DependencyInjection/MvcViewFeaturesMvcCoreBuilderExtensions.cs

            //
+            services.TryAddScoped<IComponentRenderer, ComponentRenderer>();


Can we get rid of the interface an use an internal abstract class with a public virtual method instead?

Is there a particular benefit to changing this? Given it's internal, we should be free to modify the contract in the future if we need to.

javiercn · 2019-10-11T15:24:35Z

src/Mvc/Mvc.ViewFeatures/src/RazorComponents/ComponentRenderer.cs

+
+namespace Microsoft.AspNetCore.Mvc.ViewFeatures
+{
+    internal class ComponentRenderer : IComponentRenderer


Are there any changes to what was there in the HtmlHelper?

Nope, this was pretty much copied from the old code as-is.

javiercn

Left a couple of comments, but otherwise looks solid. The main 3 things are:

Get rid of IComponentRenderer.
Add the non-generic HtmlHelper overloads.
Leave around some HtmlHelper calls in the E2E tests to make sure they keep working E2E.

Fixes #13726

pranavkm requested review from ryanbrandenburg and SteveSandersonMS as code owners September 30, 2019 22:45

pranavkm requested a review from rynowak September 30, 2019 22:45

Eilon added the area-blazor Includes: Blazor, Razor Components label Sep 30, 2019

rynowak reviewed Sep 30, 2019

View reviewed changes

src/Components/Samples/BlazorServerApp/Pages/_Host.cshtml Show resolved Hide resolved

rynowak reviewed Sep 30, 2019

View reviewed changes

src/Components/test/E2ETest/ServerExecutionTests/ComponentWithParametersTest.cs Outdated Show resolved Hide resolved

rynowak reviewed Sep 30, 2019

View reviewed changes

ryanbrandenburg reviewed Sep 30, 2019

View reviewed changes

DamianEdwards reviewed Oct 1, 2019

View reviewed changes

src/Mvc/Mvc.TagHelpers/src/ComponentTagHelper.cs Outdated Show resolved Hide resolved

SteveSandersonMS reviewed Oct 1, 2019

View reviewed changes

src/Mvc/samples/MvcSandbox/Pages/Components.cshtml Outdated Show resolved Hide resolved

SteveSandersonMS reviewed Oct 1, 2019

View reviewed changes

src/Mvc/samples/MvcSandbox/Pages/Components.cshtml Outdated Show resolved Hide resolved

SteveSandersonMS reviewed Oct 1, 2019

View reviewed changes

src/Mvc/samples/MvcSandbox/Views/Shared/_Layout.cshtml Outdated Show resolved Hide resolved

pranavkm force-pushed the prkrishn/component-tag-helper branch from 38c7eb7 to 15aabfe Compare October 1, 2019 20:06

pranavkm requested review from dougbu and a team as code owners October 1, 2019 20:06

pranavkm force-pushed the prkrishn/component-tag-helper branch from e62dbbd to ccf5a3f Compare October 1, 2019 20:15

pranavkm changed the base branch from javiercn/server-component-parameters to release/3.1 October 1, 2019 20:16

pranavkm changed the base branch from release/3.1 to release/3.1-preview1 October 1, 2019 20:18

pranavkm force-pushed the prkrishn/component-tag-helper branch 2 times, most recently from cf8ddd0 to fe83fa0 Compare October 1, 2019 20:23

pranavkm changed the base branch from release/3.1-preview1 to javiercn/server-component-parameters October 1, 2019 20:23

pranavkm changed the base branch from javiercn/server-component-parameters to release/3.1-preview1 October 1, 2019 23:11

pranavkm force-pushed the prkrishn/component-tag-helper branch 2 times, most recently from 02a7d8d to 27556bb Compare October 2, 2019 20:04

pranavkm changed the base branch from release/3.1-preview1 to release/3.1 October 2, 2019 22:21

pranavkm added this to the 3.1.0-preview2 milestone Oct 2, 2019

rynowak reviewed Oct 2, 2019

View reviewed changes

pranavkm force-pushed the prkrishn/component-tag-helper branch from cb99d62 to 968d5e5 Compare October 9, 2019 19:34

javiercn reviewed Oct 11, 2019

View reviewed changes

javiercn approved these changes Oct 11, 2019

View reviewed changes

javiercn and others added 7 commits October 11, 2019 13:32

Introduce ComponentTagHelper

80d019d

Fixes #13726

Fixup

d1faff4

Update ref

a7d40ae

Restore the app element

2e7ff1e

More fixups

0557585

Make sealed

3fe0e30

fixup

79a1769

pranavkm force-pushed the prkrishn/component-tag-helper branch from 968d5e5 to 79a1769 Compare October 11, 2019 20:47

pranavkm merged commit d299ae2 into release/3.1 Oct 11, 2019

pranavkm deleted the prkrishn/component-tag-helper branch October 11, 2019 22:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce ComponentTagHelper #14592

Introduce ComponentTagHelper #14592

pranavkm commented Sep 30, 2019

rynowak Sep 30, 2019

pranavkm Oct 1, 2019

rynowak Sep 30, 2019

DamianEdwards Sep 30, 2019

rynowak Sep 30, 2019

rynowak Sep 30, 2019

rynowak Sep 30, 2019

rynowak Sep 30, 2019

pranavkm Oct 1, 2019

rynowak Sep 30, 2019

DamianEdwards Oct 1, 2019

pranavkm Oct 1, 2019

javiercn Oct 11, 2019

ryanbrandenburg left a comment

rynowak Oct 2, 2019

pranavkm Oct 9, 2019

javiercn Oct 11, 2019

rynowak Oct 2, 2019

rynowak Oct 2, 2019

pranavkm Oct 9, 2019

pranavkm commented Oct 9, 2019

javiercn Oct 11, 2019 •

edited

Loading

javiercn Oct 11, 2019

pranavkm Oct 11, 2019

javiercn Oct 11, 2019

javiercn Oct 11, 2019

javiercn Oct 11, 2019

pranavkm Oct 11, 2019

javiercn Oct 11, 2019

pranavkm Oct 11, 2019

javiercn left a comment

		//
		services.TryAddScoped<IComponentRenderer, ComponentRenderer>();

Introduce ComponentTagHelper #14592

Introduce ComponentTagHelper #14592

Conversation

pranavkm commented Sep 30, 2019

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

ryanbrandenburg 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

pranavkm commented Oct 9, 2019

javiercn Oct 11, 2019 • edited Loading

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

javiercn left a comment

Choose a reason for hiding this comment

javiercn Oct 11, 2019 •

edited

Loading