Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strongly typed child component parameters #29379

Closed
mrpmorris opened this issue Jan 16, 2021 · 13 comments
Closed

Strongly typed child component parameters #29379

mrpmorris opened this issue Jan 16, 2021 · 13 comments
Labels
affected-most This issue impacts most of the customers area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-blazor-component-model Any feature that affects the component model for Blazor (Parameters, Rendering, Lifecycle, etc) feature-razor.language ✔️ Resolution: Duplicate Resolved as a duplicate of another issue Priority:2 Work that is important, but not critical for the release severity-major This label is used by an internal tool Status: Resolved

Comments

@mrpmorris
Copy link

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
Copy link
Member

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,

  • The code sketch above relies on some new kind of generic type inference, which on its own may be more difficult/expensive than the one in Infer component generic types from ancestor components #29349. I’m unsure the Razor compiler really has enough info to make this work in the strongly-typed way you expect, since the Razor compiler can’t see inside the C# compilation to actual resolved symbols - it only processes the sources at the text level.
  • At runtime it looks like this would rely on entirely new rendering and diffing systems. For example when the list of columns was generated dynamically via “foreach”, we’d have to do something like rendering into a separate new buffer which gets separately diffed using a new algorithm to understand when these structured parameters change. Or somehow change the way parameters are represented to make them structured object graphs instead of single key-value pairs as they are today.

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.

@SteveSandersonMS SteveSandersonMS changed the title [Blazor] Strongly typed parameters [Blazor] Strongly typed child component parameters Jan 16, 2021
@SteveSandersonMS SteveSandersonMS changed the title [Blazor] Strongly typed child component parameters Strongly typed child component parameters Jan 16, 2021
@mrpmorris
Copy link
Author

mrpmorris commented Jan 16, 2021

@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
Copy link

stefanloerwald commented Jan 18, 2021

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);?

@javiercn javiercn added the area-blazor Includes: Blazor, Razor Components label Jan 18, 2021
@javiercn javiercn added this to the Next sprint planning milestone Jan 18, 2021
@ghost
Copy link

ghost commented Jan 18, 2021

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
Copy link
Member

SteveSandersonMS commented Jan 19, 2021

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.

  • One suggestion above is some new thing called StrictRenderFragment<T, U>. That's quite a disruptive and expensive to do, because RenderFragment is a very special and powerful primitive in Blazor. We'd have to go through every single thing in the Razor compiler, the Blazor runtime, and the associated editor tooling to find everything that's special about RenderFragment, and make it do equivalent special things for StrictRenderFragment.
  • Another possibility that seems obvious is keeping RenderFragment as-is, but allowing some way to annotate it. For example [RestrictContent(typeof<Column<TItem>>)] public RenderFragment Columns { get; set; }. But that's not valid C# code, because attributes can't refer to the type parameter of their enclosing type.

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

  • It doesn't do anything to help with type inference in the editor. See Infer component generic types from ancestor components #29349 for discussions about how type inference has to work and what limitations it's under. And that really kills a large proportion of the overall value, because we're primarily interested in using hierarchy restrictions to improve the editing experience (adapting the autocompletion prompts).
    • Note that runtime hierarchy restrictions aren't really interesting at all, since this is not some kind of security feature. Compile-time restrictions are somewhat interesting, but could be achieved independently without needing any of this, via custom Roslyn analyzers.
  • It still has to get more complicated, for example to encode rules like "allow exactly 1 child instance" vs "allow multiple child instances".

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:

  • RenderFragment and RenderFragment<T> are parameter types that represent arbitrary blocks of component markup, and are opaque to the receiver. All you can do is render them, and internally, they're implemented as delegates. This is what we have today.
  • ChildComponent<T> and ChildComponents<T> are parameter types that represent a specific singular component instance or collection of component instances of a specified type T (for example, you could have ChildComponents<Column<TItem>>). These are not delegates, because they supply structured information.

Clearly, both have legitimate and different purposes.

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

  • You could read the Length of a ChildComponents<T>
  • You could loop over it and get the actual T instances so you can read their public members. For example a grid could count how many columns it has, and how many of them have a Visible property with value true.
  • You could loop over it and get a corresponding RenderFragment for each one, so you can emit that particular child into your render output anywhere you want

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.

  • Does a [Parameter] ChildComponents<T> SomeItems { get; set; } get populated before the receiver's first render, like other [Parameter] properties do?
    • 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<T> 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.
      • In case it's unclear, this really complicates the rendering flow, and might give rise to ordering weirdnesses that are hard to predict. For example, I have no idea how we'd make <CascadingValue> work with this, since the value doesn't exist until the parent was rendered, but the child needs to receive it before the parent is rendered in case it uses that to populate its own parameters.
      • Or another way to see it is that maybe it's not meaningful to pass <CascadingValue> from the recipient to its ChildComponents<T> instances, because they aren't really children at all - they come from a kind of side channel whose only parent is the call site, not the host component (e.g., Index.razor, not Grid.razor). This would really confuse people.
    • Ok, so maybe that doesn't work. Instead, we could say that ChildComponents<T> is a bit like a @ref and remains empty until after the parent has rendered. This simplifies the implementation massively, but pushes the complexity back on the developers using Blazor. So now a Grid<T> can't know what columns it contains until after its first render, and you have to go back to the weird dance of doing a fake empty render first, capturing the info you need, then using StateHasChanged to render again. Plausible, but not pretty, and still doesn't let you pass <CascadingValue> from Grid to Column (only from Index to Column).
  • What's the syntax for consuming this? If you have a Dialog<T> with [Parameter] public ChildComponent<Header<T>> MyHeader { get; set; }, does the consumer write <Dialog Data=@data><MyHeader><Header>content</Header></MyHeader></Dialog>, or <Dialog Data=@data><MyHeader>content</MyHeader></Dialog>, or <Dialog Data=@data><Header>content</Header></Dialog>? Only the first of those is natural to implement rigorously, but it's quite laborious to use.
  • What if you need multiple levels of structure? In the following example, ColGroup ends up having to be an actual component, the top level has hetrogeneous entries, and for each level of nesting you have to go through an extra level of weird dance waiting for descendants to render before being able to see them:
<Grid>
    <ColGroup>
        <Column ... />
        <Column ... />
    </ColGroup>
    <Column ... />
</Grid>
  • I don't actually know yet how any of this would be achieved inside the Razor compiler to produce correct autocompletion prompts and compilation errors. Most likely it's totally possible, but it's a substantial expense to be accounted for.

@Liander
Copy link

Liander commented Jan 19, 2021

Ok, so maybe that doesn't work. Instead, we could say that ChildComponents<T> is a bit like a @ref and 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"));

@mkArtakMSFT mkArtakMSFT added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Jan 25, 2021
@SteveSandersonMS SteveSandersonMS added affected-most This issue impacts most of the customers severity-major This label is used by an internal tool labels Jan 26, 2021 — with ASP.NET Core Issue Ranking
@csharper2010
Copy link

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
Copy link
Member

@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
Copy link

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!

@javiercn javiercn added area-razor.compiler feature-blazor-component-model Any feature that affects the component model for Blazor (Parameters, Rendering, Lifecycle, etc) feature-razor.language labels Apr 20, 2021
@ghost
Copy link

ghost commented Jul 20, 2021

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.

@TanayParikh TanayParikh modified the milestones: Backlog, .NET 7 Planning Oct 19, 2021
@TanayParikh TanayParikh added the Priority:1 Work that is critical for the release, but we could probably ship without label Oct 22, 2021
@Dreamescaper
Copy link

Dreamescaper commented Nov 1, 2021

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
Copy link

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
Copy link
Member

SteveSandersonMS commented Nov 23, 2022

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

  • Ability to restrict which child component types can be supplied (probably a compile-time thing)
  • Ability to know about what's in the child content that was supplied to you (definitely a runtime thing)

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

@SteveSandersonMS SteveSandersonMS added the ✔️ Resolution: Duplicate Resolved as a duplicate of another issue label Nov 23, 2022
@ghost ghost added the Status: Resolved label Nov 23, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affected-most This issue impacts most of the customers area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-blazor-component-model Any feature that affects the component model for Blazor (Parameters, Rendering, Lifecycle, etc) feature-razor.language ✔️ Resolution: Duplicate Resolved as a duplicate of another issue Priority:2 Work that is important, but not critical for the release severity-major This label is used by an internal tool Status: Resolved
Projects
None yet
Development

No branches or pull requests