dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.36k stars 9.99k forks source link

Strongly typed child component parameters #29379

Closed mrpmorris closed 1 year ago

mrpmorris commented 3 years ago

Problem

It is currently not possible to have razor markup for [Parameter] properties that are anything other than a render fragment, This means that building complex components such as a grid with columns is not ideal.

<Grid TModel="Person">
  <Columns>
    <Column Field="Salutation" Title="Salutation"/>
    <Column Field="GivenName" Title="Given name"/>
    <Column Field="FamilyName" Title="FamilyName"/>
  </Columns>
</Grid>

Requested solution

We'd like the ability to have the consumer of our component to be limited in what they can provide as a [Parameter]. For example, in a hypothetical Grid component

Instead of this

[Parameter]
public RenderFragment Columns { get; set; }

Which would allow the consumer to type anything they wish inside the Columns section.

  <Columns>
    <Column Field="Salutation" Title="Salutation"/>

    <h1>This makes no sense as it is not a column component</h1>

    <Column Field="GivenName" Title="Given name"/>
    <Column Field="FamilyName" Title="FamilyName"/>
  </Columns>

Support this

[ComponentParameter]
public IReadOnlyList<Column<TModel>> Columns { get; set; }

Which would reject anything other than a <Column> component at compile time.

SteveSandersonMS commented 3 years ago

Thanks for filing this. I agree it is an appealing idea and that if this sort of thing turned out to be practical, it would eliminate the need for #29349.

However I want to set expectations before anyone in the community develops expectations that it’s a likely direction. I’m extremely unsure about the practicality of it, knowing how both the runtime and Razor compiler work. For example,

So I do recognise this looks appealing at first glance, but the consequences of such a broad change require a lot of thought, and I definitely can’t promise we have anywhere near the capacity to handle such a big set of new functionality alongside what we’ve already committed for .NET 6.

mrpmorris commented 3 years ago

@SteveSandersonMS I can imagine it wouldn't be simple at all. Obviously having a TabControl that can only contain TabPage instances would be good, along with many other obvious scenarios.

I wonder if a compromise would be to somehow specify that a render fragment we have can only contain certain top-components? It remains a render fragment so we write the same code we currently do to add TabPages to the TabControl (usually via a CascadingValue with the value set to this), but we could at least prevent illogical items from being added.

So, for the grid/column example, the following would only allow GridColumn or GridColumn<TModel> components inside its <Columns> RenderFragment.

[Parameter]
public StrictRenderFragment<GridColumn> Columns { get; set; } // Same as RenderFragment

[Parameter]
public StrictRenderFragment<GridColumn<TModel>> Columns { get; set; } // Same as RenderFragment

[Parameter]
public StrictRenderFragment<GridColumn, Grid<TModel>> Columns { get; set; } // Same as RenderFragment<TModel>

[Parameter]
public StrictRenderFragment<GridColumn<TModel>, Grid<TModel>> Columns { get; set; } // Same as RenderFragment<TModel>

That should be much easier to achieve (not necessarily easy, but much easier).

Is that a reasonable compromise?

I expect the implementation would be to have the __builder be a different class passed to that RenderFragment where OpenComponent is a generic method called as AddComponent<GridColumn>(1) and there is a constraint on the <T> to ensure that whatever the type of the component is then it is descended from the <TComponent> of the restricted render fragment's builder. Then the developer could even declare sub-classes of the specified type.

stefanloerwald commented 3 years ago

It's relatively straightforward to implement such a StrictRenderFragment delegate:

public delegate void StrictRenderFragment<TAllowed>(IStrictRenderTreeBuilder<TAllowed> builder);

public interface IStrictRenderTreeBuilder<TAllowed>
{
    // important addition
    public void OpenComponent<T>(int sequence) where T : TAllowed;

    public void AddAttribute(int sequence, RenderTreeFrame frame);
    public void AddAttribute<TArgument>(int sequence, string name, EventCallback<TArgument> value);
    public void AddAttribute(int sequence, string name, EventCallback value);
    public void AddAttribute(int sequence, string name, MulticastDelegate? value);
    public void AddAttribute(int sequence, string name, string? value);
    public void AddAttribute(int sequence, string name, bool value);
    public void AddAttribute(int sequence, string name);
    public void AddAttribute(int sequence, string name, object? value);
    public void AddComponentReferenceCapture(int sequence, Action<object> componentReferenceCaptureAction);
    public void AddContent(int sequence, MarkupString markupContent);
    public void AddContent<TValue>(int sequence, RenderFragment<TValue>? fragment, TValue value);
    public void AddContent(int sequence, RenderFragment? fragment);
    public void AddContent(int sequence, string? textContent);
    public void AddContent(int sequence, object? textContent);
    public void AddElementReferenceCapture(int sequence, Action<ElementReference> elementReferenceCaptureAction);
    public void AddMarkupContent(int sequence, string? markupContent);
    public void AddMultipleAttributes(int sequence, IEnumerable<KeyValuePair<string, object>>? attributes);
    public void Clear();
    public void CloseComponent();
    public void CloseElement();
    public void CloseRegion();
    public ArrayRange<RenderTreeFrame> GetFrames();
    public void OpenRegion(int sequence);
    public void SetKey(object? value);
    public void SetUpdatesAttributeName(string updatesAttributeName);

    //removed in comparison to RenderTreeBuilder:
    //public void OpenComponent<TComponent>(int sequence) where TComponent : notnull, IComponent;
    //public void OpenElement(int sequence, string elementName);
    //public void OpenComponent (int sequence, Type componentType);
}

The important difference to RenderTreeBuilder is the lack of OpenElement(int sequence, string elementName); and OpenComponent<TComponent>(int sequence) where TComponent: notnull, IComponent; and the addition of OpenComponent<T>(int sequence) where T : TAllowed. I also omitted the OpenComponent(int, Type), but I think it's not relevant for code generation.

An issue can arise from AddMarkupContent, where pure html, such as <div>No C# here</div> would not be detected as invalid in the context where we only expect a certain component type. Removing it wasn't possible in my sandbox project, because the razor compiler added a few AddMarkupContent(..., "\r\n ");, so pure whitespace markup (which I thought was removed in net5.0, but apparently not entirely).

What remains to be addressed with such a IStrictRenderTreeBuilder is how it could/should be integrated into RenderTreeBuilder. Is it maybe sufficient to add the method public void AddContent<T>(int sequence, StrictRenderFragment<T>? fragment);?

ghost commented 3 years ago

Thanks for contacting us. We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

SteveSandersonMS commented 3 years ago

Yesterday, @javiercn and I spent a lot of time thinking this through (alongside #29349). We think this proposal is definitely interesting and really want to give it the level of focus it deserves on a proper design effort.

The TL;DR is that we think this opens a very broad range of design questions, and that doing it in a worthwhile way that's general and useful enough would be extremely expensive. I'm going to write down some of the reasons below.

The suggested simplification

I wonder if a compromise would be to somehow specify that a render fragment we have can only contain certain top-components? ... we could at least prevent illogical items from being added.

That's a valid request and might be a lot cheaper than the does-everything solution, but it still has some big issues. To start with, we're quite limited in terms of how the developer would actually express this rule.

Even if we solved the above, @javiercn and I are still not convinced the payoff of the feature would be high.

So in summary, since we don't have a great way to do this, and even if we did the outcome would be of limited usefulness, we're more inclined to put more focus on a longer-term effort to build a more comprehensive solution, even though it would be very expensive. We're more likely to focus on #29349 in the short term, which addresses a different requirement and doesn't preclude us from doing this in the future.

A longer-term possibility

In the long run, it seems like what we want for #29379 is a genuine peer-level alternative to RenderFragment. For example:

Clearly, both have legitimate and different purposes.

So, what could you do with ChildComponents<T>?

This is super powerful but is also really disruptive to how rendering works, and raises other design challenges.

Design challenges

There will be ways to overcome all of the following, so please don't interpret this as meaning "we're unwilling to try". I think we are open to trying, eventually, but it's always valuable to think of the hardest problems up front so you can account for them in design and estimate costs.

<Grid>
    <ColGroup>
        <Column ... />
        <Column ... />
    </ColGroup>
    <Column ... />
</Grid>
Liander commented 3 years ago

Ok, so maybe that doesn't work. Instead, we could say that ChildComponents<T> is a bit like a @refand remains empty until after the parent has rendered. This simplifies the implementation massively, but pushes the complexity back on the developers using Blazor.

For me, the whole point is to get rid of messy workarounds of accessing state from child components and timing of renderings. That makes me question if that declaration of columns really should represent render components. It would be interesting if you could explore other constructs of it.

Just imagine for a moment that you can specify data as markup, then those columns would be just strongly typed data properties handed over as a parameter. The owning component (the Grid) will make use of that data and might in turn use some component to render them, but from the user perspective it would look and mean the same thing.

For me that would be enough, but an interesting thought is to take it a bit further and let those "data markup" be view-models and having a matching view to them automatically attached. Those views would be ordinary render components, perhaps named matched by convention with some suffix to the "view-model component" to pair them up. Then you are back to having render components of the columns but its data is initialized from data owned in parent Grid and set as parameters before the Grid renders.

My guess is that this would need to be a similar code construction to #29349 for building the data list with synthetic arguments to infer type, like:

       EmitGridComponent(builder, arg0: people, arg1: BuildColumns(syntheticArg0: people)
         .AppendColumn(syntheticArg0: people, arg0: "Salution", arg1: "Salution")
         .AppendColumn(syntheticArg0: people, arg0: "GivenName", arg1: "Given name"));
csharper2010 commented 3 years ago

I am not deep into Blazor yet so please excuse if my understanding is wrong or incorrect. But I am really excited about Blazor and want to experiment and learn more about it.

In my eyes, in all the examples we have (Columns of Grids, Tab Pages of Tab Controls), the artifacts to be added to the parent component aren't really Render Fragments. They are really Child Components that can be rendered later on in some way but only under the control of the parent component. The Child Components themselves can only generate Render Fragments but they aren't RenderFragments to be displayed as part of the parent.

If we look at the Grid Column, it might generate cell content and it might generate column header content. We can say that the main purpose of a Grid Column component is to render cells so the natural design for the Grid Column control design might be to model the grid cell as the control's content. For the Tag Page it would be the Tab Page content whereas the header would be some kind of secondary content. But on the other hand, even this "primary" purpose isn't that clear so those things could also be some kind of "faceless" components (effectively they are like that e.g. in the RadZen data grid).

Looking at this, the ChildComponent<T> idea might fit quite well into the feature request but it would require a different lifecycle. The special children that are not RenderFragments themselves are more like ordinary parameters, their lifecycle must be ahead of the parent component lifecycle.

So what if those items were not ordinary components at all but derived from a new kind of base class? They can't live on their own and they are never rendered directly to the parent component's render tree but, maybe they don't have a render tree at all.

They are fully controlled by the parent and can generate render artifacts through properties and methods, the render fragment generating all thos child components would only be used for maintaining the child component instances. This solution would also allow ifs and foreaches to conditionally create those child components. The child components can themselves have child components, ordinary parameters or parameters with render fragments (e.g. for cell templates).

Before each render, the render fragments for child component parameters must be generated, maybe that could be a new lifecycle method or integrated into the Parameter-lifecycle.

If I understand correctly, there curently is a distinction between component attributes for ordinary parameters and child element content for all kinds of render fragments. Either the attribute side could be extended to support child component trees like so (reflecting the fact that they are like other parameters):

<DataGrid Columns="@(<DataGridColumn /><DataGridColumn />) />

or there could be a convention to distinguish the child parts that should be held in the background instead of directly rendered as child of the component:

<DataGrid><DataGrid.Columns><DataGridColumn /><DataGridColumn /></DataGrid.Columns><DataGrid>

So finishing that up, I might be totally wrong because I don't know much about Blazor but at least I got the impression that this would really be a strong concept leading a lot of new opportunities in component design and reduce the need for workarounds.

SteveSandersonMS commented 3 years ago

@csharper2010 I think your understanding is totally on track. What you're describing sounds exactly like what is being asked for in this issue.

TomGathercole commented 3 years ago

Clearly it would be nice if they do, because you might want that information up front, plus it's consistent with how other parameters behave. But this really messes up the rendering order. We'd have to render the child components definition into an entirely separate RenderFrame buffer, diff against any previous output, update the parameter value by inserting/deleting/reordering the ChildComponents collection, and actually instantiate any new child components and call their SetParametersAsync so they can populate their own parameter properties. But, when those children ask to render themselves, we'd have to capture that information in a separate queue so we could defer it until their parent has been rendered, in order to preserve the parent->child render order.

Is there a middle ground where we populate a collection of ChildComponents<T> up front, but they're not actually rendered or initialized yet? Potentially they could be lazily instantiated inside the parent component when they're first accesed?

Sorry if that's a gross simplification of the problem at hand!

ghost commented 3 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

Dreamescaper commented 2 years ago

Should I be able to add coded logic?

<Grid TModel="Person">
  <Columns>
    @if(myCondition)
     {
        <Column Field="Salutation" Title="Salutation"/>
     }
  </Columns>
</Grid>

Should I be able to refactor that coded logic to a separate component, and use that one?

<Grid TModel="Person">
  <Columns>
        <MyConditionalColumn />
  </Columns>
</Grid>

MyConditionalColumn.razor

    @if(myCondition)
     {
        <Column Field="Salutation" Title="Salutation"/>
     }
...
the-avid-engineer commented 2 years ago

Personally, I would say that making a column conditional would be necessary but the refactored component should not be allowed given that the request is that the Parameter only allows <Column> children

SteveSandersonMS commented 1 year ago

We've continued to think about this, and think it breaks down into two sub-requests:

These two sub-features are described in two existing, older issues (https://github.com/dotnet/aspnetcore/issues/12302 and https://github.com/dotnet/aspnetcore/issues/17200), so to avoid duplicating design conversations, I'm closing this as a duplicate of those two.