-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Implement DynamicComponent #28082
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
Implement DynamicComponent #28082
Conversation
Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
{ | ||
foreach (var entry in Parameters) | ||
{ | ||
builder.AddAttribute(1, entry.Key, entry.Value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super familiar with the diffing behavior, what does it mean for all attributes to have the same sequence number? Should we be doing something special to ensure that we enumerate the dictionary in some consistent order (since dictionary has no enumeration guarantees)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point to raise. However I don't think we want to rely on enforcing consistent enumeration order. Presumably that would mean something like sorting the key-value pairs, which would immediately be the most expensive thing going on here.
In practice, I'm assuming that the evaluation order will (at least mostly) be consistent on successive renders. If it is, the attribute diffing rules will produce identical and optimal behavior regardless of whether the sequence numbers are all equal or if they are increasing.
If the evaluation order sometimes changes significantly between renders, then the attribute diffing system will also handle this just fine, but will involve more operations internally as the non-matching attribute names will trigger it going into its "dictionary building" attribute diffing mode which isn't as fast as the pure forward path it takes if the order hasn't changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"dictionary building" attribute diffing mode
magic. got it. But yeah, in the most common case, i.e using a Dictionary<TKey, TValue>
, the observed behavior is that you enumerate based on enumeration. While it's good to know that we don't rely on this behavior, it's fairly likely we're going to hit the fast past.
|
||
void Render(RenderTreeBuilder builder) | ||
{ | ||
if (Type is null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to do this as part of SetParametersAsync
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In virtually every imaginable scenario it won't make any difference, either to behavior or to the number of operations executed. So mostly I think it doesn't matter.
If we're willing to stretch plausibility a bit, we could theoretically imagine a scenario where, in between SetParametersAsync
and the Render
delegate being invoked, the developer changed Type
to be null
. In this scenario, you would prefer the behavior to be what we have here, i.e., that we complain about the null only at the point where we're trying to use the null.
Overall though I don't think it makes any real difference here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: I needed to submit another commit to restart CI which seems to have got stuck, so took the opportunity to move this check as suggested. On consideration I think your expectation that this happens in SetParametersAsync
would most likely line up with other people's expectations, so best to avoid surprise.
Co-authored-by: Pranav K <prkrishn@hotmail.com>
Implements #26781
Although developers have been able to implement custom rendertree builder for dynamic component selection since the beginning of Blazor, we've always intended to make this a simpler built-in feature.
It's fairly simple, so I can't think of any reason not to just do it now.
Usage
... or:
API design notes
Naming
I did consider calling this simply
<Component>
, but I think<DynamicComponent>
is better because:<component>
tag helperParameter passing
I also considered catch-all parameter passing, like
<DynamicComponent Type="@someType" MyParameter="Hello" IsCool="true" />
.However I think it's important not to do that, and only to allow passing an explicit dictionary, because if we do catch-all parameters, then every explicit parameter on
DynamicComponent
itself - now and in the future - effectively becomes a reserved word that you can't pass to a dynamic child. It would become a breaking change to add any new parameters toDynamicComponent
as they would start shadowing child component parameters that happen to have the same name.Additionally it's unclear that there would be good use cases for the catch-all passing style. The only purpose of
<DynamicComponent>
is to deal with cases where the call site doesn't know what child component type it's rendering. In this case, it's unlikely that the call site knows of some fixed set of parameter names to pass to all possible dynamic children. So it's going to be far more common to want to pass a dictionary. And if we supported multiple different parameter passing styles, then that's more to understand/document and deal with concerns like ordering, shadowing, and so on.So in conclusion I think it's safer and clearer to keep it simple and only allow passing a dictionary explicitly.