Blazor API Review: Components Programming Model

[rynowak]

Blazor API Review: Components programming model

This is the API for the primary programming surface for authoring components. This includes types and attributes that make up ComponentBase as well as types that you will use like MarkupString and ElementRef. This excludes feature areas like IUriHelper, RenderTreeBuilder, or auth which get their own API review.

ComponentBase

namespace Microsoft.AspNetCore.Components
{
    public abstract partial class ComponentBase : Microsoft.AspNetCore.Components.IComponent, Microsoft.AspNetCore.Components.IHandleAfterRender, Microsoft.AspNetCore.Components.IHandleEvent
    {
        public ComponentBase() { }
        protected virtual void BuildRenderTree(Microsoft.AspNetCore.Components.RenderTree.RenderTreeBuilder builder) { }
        protected System.Threading.Tasks.Task Invoke(System.Action workItem) { throw null; }
        protected System.Threading.Tasks.Task InvokeAsync(System.Func<System.Threading.Tasks.Task> workItem) { throw null; }
        void Microsoft.AspNetCore.Components.IComponent.Configure(Microsoft.AspNetCore.Components.RenderHandle renderHandle) { }
        System.Threading.Tasks.Task Microsoft.AspNetCore.Components.IHandleAfterRender.OnAfterRenderAsync() { throw null; }
        System.Threading.Tasks.Task Microsoft.AspNetCore.Components.IHandleEvent.HandleEventAsync(Microsoft.AspNetCore.Components.EventCallbackWorkItem callback, object arg) { throw null; }
        protected virtual void OnAfterRender() { }
        protected virtual System.Threading.Tasks.Task OnAfterRenderAsync() { throw null; }
        protected virtual void OnInit() { }
        protected virtual System.Threading.Tasks.Task OnInitAsync() { throw null; }
        protected virtual void OnParametersSet() { }
        protected virtual System.Threading.Tasks.Task OnParametersSetAsync() { throw null; }
        public virtual System.Threading.Tasks.Task SetParametersAsync(Microsoft.AspNetCore.Components.ParameterCollection parameters) { throw null; }
        protected virtual bool ShouldRender() { throw null; }
        protected void StateHasChanged() { }
    }

    public partial interface IComponent
    {
        void Configure(Microsoft.AspNetCore.Components.RenderHandle renderHandle);
        System.Threading.Tasks.Task SetParametersAsync(Microsoft.AspNetCore.Components.ParameterCollection parameters);
    }

    public partial interface IComponentContext
    {
        bool IsConnected { get; }
    }

    public partial interface IHandleAfterRender
    {
        System.Threading.Tasks.Task OnAfterRenderAsync();
    }

    [System.Runtime.InteropServices.StructLayoutAttribute(System.Runtime.InteropServices.LayoutKind.Sequential)]
    public readonly partial struct RenderHandle
    {
        private readonly object _dummy;
        private readonly int _dummyPrimitive;
        public bool IsInitialized { get { throw null; } }
        public System.Threading.Tasks.Task Invoke(System.Action workItem) { throw null; }
        public System.Threading.Tasks.Task InvokeAsync(System.Func<System.Threading.Tasks.Task> workItem) { throw null; }
        public void Render(Microsoft.AspNetCore.Components.RenderFragment renderFragment) { }
    }
}

Some topics to discuss here:

• I don't feel great about the fact that Invoke and InvokeAsync are different method instead of overloads (here, on IDispatcher and RenderHandle).
• Should we have overloads of Invoke and InvokeAsync that return something here as well?
• Do we really want OnInit vs OnInitialized? Abbreviations in APIs always feel bad to me, even if they are known things like OnInit.
• Are we sure that it's worth providing sync and async versions of all of our lifecycle methods?
• SetParametersAsync should probably be an explicit implementation, the user cannot duplicate the behaviour of the ComponentBase implementation, so it's not useful to override.
• We don't expose any lifecyle methods related to IHandleEvents (other than ShouldRender). That's probably ok? it could be done in the future.
• Should we expose a property HasRendered - basically provide some API for the common use case of initializing JS interop?
• RenderHandle seems to wear two hats. Should we pass the dispatcher into the component instead?
• Is there a really good reason for RenderHandle to be a struct?
• I don't love Configure as a name. Other ideas?

Events and EventCallback

namespace Microsoft.AspNetCore.Components
{
    [System.Runtime.InteropServices.StructLayoutAttribute(System.Runtime.InteropServices.LayoutKind.Sequential)]
    public partial struct EventCallbackWorkItem
    {
        private object _dummy;
        public static readonly Microsoft.AspNetCore.Components.EventCallbackWorkItem Empty;
        public EventCallbackWorkItem(System.MulticastDelegate @delegate) { throw null; }
        public System.Threading.Tasks.Task InvokeAsync(object arg) { throw null; }
    }

    [System.Runtime.InteropServices.StructLayoutAttribute(System.Runtime.InteropServices.LayoutKind.Sequential)]
    public readonly partial struct EventCallback
    {
        private readonly object _dummy;
        public static readonly Microsoft.AspNetCore.Components.EventCallback Empty;
        public static readonly Microsoft.AspNetCore.Components.EventCallbackFactory Factory;
        public EventCallback(Microsoft.AspNetCore.Components.IHandleEvent receiver, System.MulticastDelegate @delegate) { throw null; }
        public bool HasDelegate { get { throw null; } }
        public System.Threading.Tasks.Task InvokeAsync(object arg) { throw null; }
    }

    [System.Runtime.InteropServices.StructLayoutAttribute(System.Runtime.InteropServices.LayoutKind.Sequential)]
    public readonly partial struct EventCallback<T>
    {
        private readonly object _dummy;
        public EventCallback(Microsoft.AspNetCore.Components.IHandleEvent receiver, System.MulticastDelegate @delegate) { throw null; }
        public bool HasDelegate { get { throw null; } }
        public System.Threading.Tasks.Task InvokeAsync(T arg) { throw null; }
    }

    public partial interface IHandleEvent
    {
        System.Threading.Tasks.Task HandleEventAsync(Microsoft.AspNetCore.Components.EventCallbackWorkItem item, object arg);
    }

    public sealed partial class EventCallbackFactory
    {
        public EventCallbackFactory() { }
        [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
        public Microsoft.AspNetCore.Components.EventCallback Create(object receiver, Microsoft.AspNetCore.Components.EventCallback callback) { throw null; }
        public Microsoft.AspNetCore.Components.EventCallback Create(object receiver, System.Action callback) { throw null; }
        public Microsoft.AspNetCore.Components.EventCallback Create(object receiver, System.Action<object> callback) { throw null; }
        public Microsoft.AspNetCore.Components.EventCallback Create(object receiver, System.Func<object, System.Threading.Tasks.Task> callback) { throw null; }
        public Microsoft.AspNetCore.Components.EventCallback Create(object receiver, System.Func<System.Threading.Tasks.Task> callback) { throw null; }
        [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
        public Microsoft.AspNetCore.Components.EventCallback<T> CreateInferred<T>(object receiver, System.Action<T> callback, T value) { throw null; }
        [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
        public Microsoft.AspNetCore.Components.EventCallback<T> CreateInferred<T>(object receiver, System.Func<T, System.Threading.Tasks.Task> callback, T value) { throw null; }
        [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
        public Microsoft.AspNetCore.Components.EventCallback<T> Create<T>(object receiver, Microsoft.AspNetCore.Components.EventCallback callback) { throw null; }
        [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
        public Microsoft.AspNetCore.Components.EventCallback<T> Create<T>(object receiver, Microsoft.AspNetCore.Components.EventCallback<T> callback) { throw null; }
        public Microsoft.AspNetCore.Components.EventCallback<T> Create<T>(object receiver, System.Action callback) { throw null; }
        public Microsoft.AspNetCore.Components.EventCallback<T> Create<T>(object receiver, System.Action<T> callback) { throw null; }
        public Microsoft.AspNetCore.Components.EventCallback<T> Create<T>(object receiver, System.Func<System.Threading.Tasks.Task> callback) { throw null; }
        public Microsoft.AspNetCore.Components.EventCallback<T> Create<T>(object receiver, System.Func<T, System.Threading.Tasks.Task> callback) { throw null; }
        [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
        public string Create<T>(object receiver, string callback) { throw null; }
    }

    public static partial class EventCallbackFactoryBinderExtensions
    {
        public static Microsoft.AspNetCore.Components.EventCallback<Microsoft.AspNetCore.Components.UIChangeEventArgs> CreateBinder(this Microsoft.AspNetCore.Components.EventCallbackFactory factory, object receiver, System.Action<bool> setter, bool existingValue) { throw null; }
        public static Microsoft.AspNetCore.Components.EventCallback<Microsoft.AspNetCore.Components.UIChangeEventArgs> CreateBinder(this Microsoft.AspNetCore.Components.EventCallbackFactory factory, object receiver, System.Action<System.DateTime> setter, System.DateTime existingValue) { throw null; }
        public static Microsoft.AspNetCore.Components.EventCallback<Microsoft.AspNetCore.Components.UIChangeEventArgs> CreateBinder(this Microsoft.AspNetCore.Components.EventCallbackFactory factory, object receiver, System.Action<System.DateTime> setter, System.DateTime existingValue, string format) { throw null; }
        public static Microsoft.AspNetCore.Components.EventCallback<Microsoft.AspNetCore.Components.UIChangeEventArgs> CreateBinder(this Microsoft.AspNetCore.Components.EventCallbackFactory factory, object receiver, System.Action<decimal> setter, decimal existingValue) { throw null; }
        public static Microsoft.AspNetCore.Components.EventCallback<Microsoft.AspNetCore.Components.UIChangeEventArgs> CreateBinder(this Microsoft.AspNetCore.Components.EventCallbackFactory factory, object receiver, System.Action<double> setter, double existingValue) { throw null; }
        public static Microsoft.AspNetCore.Components.EventCallback<Microsoft.AspNetCore.Components.UIChangeEventArgs> CreateBinder(this Microsoft.AspNetCore.Components.EventCallbackFactory factory, object receiver, System.Action<int> setter, int existingValue) { throw null; }
        public static Microsoft.AspNetCore.Components.EventCallback<Microsoft.AspNetCore.Components.UIChangeEventArgs> CreateBinder(this Microsoft.AspNetCore.Components.EventCallbackFactory factory, object receiver, System.Action<long> setter, long existingValue) { throw null; }
        public static Microsoft.AspNetCore.Components.EventCallback<Microsoft.AspNetCore.Components.UIChangeEventArgs> CreateBinder(this Microsoft.AspNetCore.Components.EventCallbackFactory factory, object receiver, System.Action<bool?> setter, bool? existingValue) { throw null; }
        public static Microsoft.AspNetCore.Components.EventCallback<Microsoft.AspNetCore.Components.UIChangeEventArgs> CreateBinder(this Microsoft.AspNetCore.Components.EventCallbackFactory factory, object receiver, System.Action<System.DateTime?> setter, System.DateTime? existingValue) { throw null; }
        public static Microsoft.AspNetCore.Components.EventCallback<Microsoft.AspNetCore.Components.UIChangeEventArgs> CreateBinder(this Microsoft.AspNetCore.Components.EventCallbackFactory factory, object receiver, System.Action<decimal?> setter, decimal? existingValue) { throw null; }
        public static Microsoft.AspNetCore.Components.EventCallback<Microsoft.AspNetCore.Components.UIChangeEventArgs> CreateBinder(this Microsoft.AspNetCore.Components.EventCallbackFactory factory, object receiver, System.Action<double?> setter, double? existingValue) { throw null; }
        public static Microsoft.AspNetCore.Components.EventCallback<Microsoft.AspNetCore.Components.UIChangeEventArgs> CreateBinder(this Microsoft.AspNetCore.Components.EventCallbackFactory factory, object receiver, System.Action<int?> setter, int? existingValue) { throw null; }
        public static Microsoft.AspNetCore.Components.EventCallback<Microsoft.AspNetCore.Components.UIChangeEventArgs> CreateBinder(this Microsoft.AspNetCore.Components.EventCallbackFactory factory, object receiver, System.Action<long?> setter, long? existingValue) { throw null; }
        public static Microsoft.AspNetCore.Components.EventCallback<Microsoft.AspNetCore.Components.UIChangeEventArgs> CreateBinder(this Microsoft.AspNetCore.Components.EventCallbackFactory factory, object receiver, System.Action<float?> setter, float? existingValue) { throw null; }
        public static Microsoft.AspNetCore.Components.EventCallback<Microsoft.AspNetCore.Components.UIChangeEventArgs> CreateBinder(this Microsoft.AspNetCore.Components.EventCallbackFactory factory, object receiver, System.Action<float> setter, float existingValue) { throw null; }
        public static Microsoft.AspNetCore.Components.EventCallback<Microsoft.AspNetCore.Components.UIChangeEventArgs> CreateBinder(this Microsoft.AspNetCore.Components.EventCallbackFactory factory, object receiver, System.Action<string> setter, string existingValue) { throw null; }
        public static Microsoft.AspNetCore.Components.EventCallback<Microsoft.AspNetCore.Components.UIChangeEventArgs> CreateBinder<T>(this Microsoft.AspNetCore.Components.EventCallbackFactory factory, object receiver, System.Action<T> setter, T existingValue) { throw null; }
    }
}

Some topics to discuss here:

• EventCallbackWorkItem should be readonly.
• EventCallbackWorkItem is needed to keep the API from being insane due to generics, but I feel like it doesn't provide any useful information.
• Should EventCallback and EventCallback<> have an InvokeAsync() method? It feels bad to pass null to the existing method.
• EventCallback<> should probably have an Empty member for consistency.
• public string Create<T>(object receiver, string callback) { throw null; } should be removed.
• EventCallbackFactoryBinderExtensions - we have a problem here. All of our existing bind functionality for elements is tied to UIChangeEventArgs which is an HTML/DOM concept. I have no brilliant ideas what to do about this. We decided to leave UIChangeEventArgs where it is for now.

Dispatcher

namespace Microsoft.AspNetCore.Components.Rendering
{
    public partial interface IDispatcher
    {
        System.Threading.Tasks.Task Invoke(System.Action action);
        System.Threading.Tasks.Task InvokeAsync(System.Func<System.Threading.Tasks.Task> asyncAction);
        System.Threading.Tasks.Task<TResult> InvokeAsync<TResult>(System.Func<System.Threading.Tasks.Task<TResult>> asyncFunction);
        System.Threading.Tasks.Task<TResult> Invoke<TResult>(System.Func<TResult> function);
    }
}

Some topics to discuss here:

• This should probably be an abstract class. We have a single implementation.
  • We could also implement this on ComponentBase or RenderHandle (or both).
• I don't love that these are different method names, but I'm not sure how to resolve the conflict between Func<TResult> and Func<Task>.

Parameters

namespace Microsoft.AspNetCore.Components
{
    [System.Runtime.InteropServices.StructLayoutAttribute(System.Runtime.InteropServices.LayoutKind.Sequential)]
    public readonly partial struct Parameter
    {
        private readonly object _dummy;
        private readonly int _dummyPrimitive;
        public bool Cascading { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
        public string Name { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
        public object Value { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
    }

    [System.Runtime.InteropServices.StructLayoutAttribute(System.Runtime.InteropServices.LayoutKind.Sequential)]
    public readonly partial struct ParameterCollection
    {
        private readonly object _dummy;
        private readonly int _dummyPrimitive;
        public static Microsoft.AspNetCore.Components.ParameterCollection Empty { get { throw null; } }
        public static Microsoft.AspNetCore.Components.ParameterCollection FromDictionary(System.Collections.Generic.IDictionary<string, object> parameters) { throw null; }
        public Microsoft.AspNetCore.Components.ParameterEnumerator GetEnumerator() { throw null; }
        public T GetValueOrDefault<T>(string parameterName) { throw null; }
        public T GetValueOrDefault<T>(string parameterName, T defaultValue) { throw null; }
        public System.Collections.Generic.IReadOnlyDictionary<string, object> ToDictionary() { throw null; }
        public bool TryGetValue<T>(string parameterName, out T result) { throw null; }
    }

    public static partial class ParameterCollectionExtensions
    {
        public static void SetParameterProperties(this in Microsoft.AspNetCore.Components.ParameterCollection parameterCollection, object target) { }
    }

    [System.Runtime.InteropServices.StructLayoutAttribute(System.Runtime.InteropServices.LayoutKind.Sequential)]
    public partial struct ParameterEnumerator
    {
        private object _dummy;
        private int _dummyPrimitive;
        public Microsoft.AspNetCore.Components.Parameter Current { get { throw null; } }
        public bool MoveNext() { throw null; }
    }
}

Some topics to discuss here:

• I'm still complaining about the name of this. Parameter is really ParameterState or ParameterValue.
• Cascading -> IsCascadingValue? Low value, not doing.
• ParameterCollection doesn't implement any interfaces, this could easily implement IReadOnlyDictionary<> and then we don't need ToDictionary(). edit we're renaming this instead.
• It's extremely wierd that SetParameterProperties is public, and extension method.
• ParameterEnumerator doesn't implement IEnumerator. This is OK, there is predendent for this in the BCL

Attributes

namespace Microsoft.AspNetCore.Components
{
    [System.AttributeUsageAttribute(System.AttributeTargets.Property, AllowMultiple=false, Inherited=false)]
    public sealed partial class CascadingParameterAttribute : System.Attribute
    {
        public CascadingParameterAttribute() { }
        public string Name { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
    }

    [System.AttributeUsageAttribute(System.AttributeTargets.Property, AllowMultiple=false)]
    public partial class InjectAttribute : System.Attribute
    {
        public InjectAttribute() { }
    }

    [System.AttributeUsageAttribute(System.AttributeTargets.Class, AllowMultiple=true, Inherited=false)]
    public partial class RouteAttribute : System.Attribute
    {
        public RouteAttribute(string template) { }
        public string Template { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
    }

    [System.AttributeUsageAttribute(System.AttributeTargets.Property, AllowMultiple=false, Inherited=false)]
    public sealed partial class ParameterAttribute : System.Attribute
    {
        public ParameterAttribute() { }
        public bool CaptureUnmatchedValues { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
    }
}

namespace Microsoft.AspNetCore.Components.Layouts
{
    [System.AttributeUsageAttribute(System.AttributeTargets.Class, AllowMultiple=false, Inherited=true)]
    public partial class LayoutAttribute : System.Attribute
    {
        public LayoutAttribute(System.Type layoutType) { }
        public System.Type LayoutType { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
    }
}

Some topics to discuss here:

• Make all of these sealed.
• LayoutAttribute should probably be in .Components
• Inherited = true for all by RouteAttribute (which has a good reason to prevent inheritance)

Fundamental building blocks

namespace Microsoft.AspNetCore.Components
{
    public delegate void RenderFragment(Microsoft.AspNetCore.Components.RenderTree.RenderTreeBuilder builder);

    public delegate Microsoft.AspNetCore.Components.RenderFragment RenderFragment<T>(T value);
    
    [System.Runtime.InteropServices.StructLayoutAttribute(System.Runtime.InteropServices.LayoutKind.Sequential)]
    public readonly partial struct ElementRef
    {
        private readonly object _dummy;
        [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
        public string __internalId { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
    }

    [System.Runtime.InteropServices.StructLayoutAttribute(System.Runtime.InteropServices.LayoutKind.Sequential)]
    public readonly partial struct MarkupString
    {
        private readonly object _dummy;
        public MarkupString(string value) { throw null; }
        public string Value { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
        public static explicit operator Microsoft.AspNetCore.Components.MarkupString (string value) { throw null; }
        public override string ToString() { throw null; }
    }
}

Some topics to discuss here:

• Should ElementRef be ElementReference?

[rynowak]

@SteveSandersonMS @javiercn @pranavkm @mkArtakMSFT @danroth27 @ajaybhargavb @NTaylorMullen - please have a look and give your feedback 👍

[pranavkm]

Do we really want OnInit vs OnInitialized? Abbreviations in APIs always feel bad to me, even if they are known things like OnInit.

+1 For OnInitialized. We have a few other places in the framework that use Initialize, and it makes sense to be consistent.

Are we sure that it's worth providing sync and async versions of all of our lifecycle methods?
It's wierd that we call both OnInit and OnInitAsync. Usually you just call one of these.

Seems fairly convenient to have the sync overloads. Are there particularly good reasons not to have these? We should have the async ones call the sync one and be done.
You're right, having the sync and async ones do different work is weird.

SetParametersAsync should probably be an explicit implementation, the user cannot duplicate the behaviour of the ComponentBase implementation, so it's not useful to override.
+1

I don't love Configure as a name. Other ideas?

The doc comment says it's initializing the component. Initialize?

I'd think it's generally ucommon for users to be interacting with with IDispatcher? Seems like more trouble to change the shape of the methods

Should ElementRef be ElementReference?
+1

[NTaylorMullen]

ComponentBase

• I don't feel great about the fact that Invoke and InvokeAsync are different method instead of overloads (here, on IDispatcher and RenderHandle).

Ya if we just had two InvokeAsync overloads that'd be perfect. I think it's pretty odd that Invoke method represents the parameter it's being passed not the actual functionality of the method (it being async).

• Should we have overloads of Invoke and InvokeAsync that return something here as well?

Sounds reasonable to me; however, it's also not critical because that's something we can always add in the future.

• Do we really want OnInit vs OnInitialized? Abbreviations in APIs always feel bad to me, even if they are known things like OnInit.

OnInitialized

• Are we sure that it's worth providing sync and async versions of all of our lifecycle methods?

It's convenient but I'm less convinced it's needed right now. In TagHelper land we did this and people loved having the synchronous overload. However, people also got massively confused at the fact that TagHelpers were async even though all of their code was written in a synchronous method. I'm team, don't have both because of my scars from TagHelper times 😆

• We don't expose any lifecyle methods related to IHandleEvents (other than ShouldRender). That's probably ok? it could be done in the future.

+1 for doing it in the future. We don't want to overcommit here because things can get sloppy real quick if we expose too many hooks too early.

Events and EventCallback

• Should EventCallback and EventCallback<> have an InvokeAsync() method? It feels bad to pass null to the existing method.

Ehh, less is more imo. If we get feedback we can always add that.

• EventCallbackFactoryBinderExtensions - we have a problem here. All of our existing bind functionality for elements is tied to UIChangeEventArgs which is an HTML/DOM concept. I have no brilliant ideas what to do about this.

Wont we have a Web specific assembly? Any reason the extension methods can't exist there?

General feedback on Events and EventCallback

Do we expect people to be directly interacting with any of the EventCallback types? I imagine they wouldn't. If that's the case we should consider sprinkling additional EditorBrowsableState.Nevers on more of the API surface.

Parameters

• I'm still complaining about the name of this. Parameter is really ParameterState or ParameterValue.

So I don't have a lot of context in this stack but based off of the API solely it looks more like a ParameterState.

• Cascading -> IsCascadingValue?

I'd just do IsCascading

• ParameterCollection doesn't implement any interfaces, this could easily implement IReadOnlyDictionary<> and then we don't need ToDictionary().

Definitely. I think in general the API should be slimmed down to conform to IReadOnlyDictionary. Any reason not to? Seems like there's a lot of unique API choices on ParameterCollection; all of which represent a flavor of dictionary.

• It's extremely wierd that SetParameterProperties is public, and extension method.

Ya i'd imagine this would just be part of ParameterCollection.

Attributes

public bool CaptureUnmatchedValues { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }

Thinking about this more I feel like this should be represented as its own own return type that has helper methods to extract attribute names, attribute kvp's attribute values etc. Anyhow, that's a bigger ask than an API review, I think I just have beef having CaptureUnmatchedValues as part of [Parameter].

Fundamental building blocks

• Should ElementRef be ElementReference?

If this is used with the ref attribute no, if not, yes.

[System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
        public string __internalId { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }

Why does this have to be public? Is it not a runtime concern where we're ok with it not existing in the ref assembly?

[SteveSandersonMS]

I don't feel great about the fact that Invoke and InvokeAsync are different method instead of overloads (here, on IDispatcher and RenderHandle).

Agreed - doesn't seem to serve any purpose.

Should we have overloads of Invoke and InvokeAsync that return something here as well?

Could be added in the future; don't need to now. In the short term people who need it can set things on captured variables to achieve the same affect (albeit with some perf cost).

Do we really want OnInit vs OnInitialized? Abbreviations in APIs always feel bad to me, even if they are known things like OnInit.

I'd be fine with moving to OnInitialized. Consensus above certainly points that way.

Are we sure that it's worth providing sync and async versions of all of our lifecycle methods?

It seems like the best option still. C# isn't good at dealing with this "multiple possible return types" scenario (unlike, say, TypeScript), so the sync variants are needed if we want to avoid people having to put return Task.CompletedTask boilerplate (or getting warnings about async methods not awaiting) all over the place.

SetParametersAsync should probably be an explicit implementation, the user cannot duplicate the behaviour of the ComponentBase implementation, so it's not useful to override.

I've overridden this as a way to enforce rules like "parameter X is required" because it runs before OnInit etc. But I agree the use cases are very limited and people are just as likely to confuse themselves over this. I'd be fine with making it non-virtual based on the idea of preferring a more limited API surface whenever there's doubt.

Should we expose a property HasRendered

This would need more design, as it's subtle. Calling it HasRendered would be confusing because one of the most common use cases is in OnAfterRenderAsync, where by definition HasRendered would always be true, so that's not useful. You want a value like IsFirstRender that only changes after OnAfterRender but more confusingly, it's technically possible for a subsequent OnParametersSetAsync to run before the first OnAfterRender, so what should its value be then?

If the only common use case for this is on OnAfterRenderAsync, maybe it would be much simpler to make it a parameter to OnAfterRenderAsync (e.g., OnAfterRenderAsync(bool isFirstRender)). I'm not sure what use case exists for it in OnParametersSetAsync, and there's certainly no use case for it in OnInit or during rendering itself. Obviously, if it's a method parameter, it would be a breaking change to do it later so we'd need it for 3.0.

RenderHandle seems to wear two hats. Should we pass the dispatcher into the component instead

Feels like that's a matter of terminology. If we change the name of RenderHandle to DispatcherHandle and let it be able to dispatch two types of things ("renders" and "invocations"), would that seem better? Both fit into the notion of dispatching stuff to the renderer.

Is there a really good reason for RenderHandle to be a struct?

I think there was just no reason not to, and it saves one allocation per component instance. Any cause for concern?

I don't love Configure as a name. Other ideas?

I thought the name Configure was fine. It was meant to sound a bit general-purpose since we didn't know what other kinds of pre-usage configuration would be added later. But now we're locking down the API and we know nothing else will be added, I agree other names might also work.

We could call it SetRenderHandle (or SetDispatcherHandle if we rename that), since that's literally what it's for now. Of course, it should be a method not a property, since setter-only properties are bizarre.

EventCallbackWorkItem should be readonly.

+1

Should EventCallback and EventCallback<> have an InvokeAsync() method?

I would have expected EventCallback to, but EventCallback<> not to, since the fact that you chose the generic one implies you expect to be passed a value of type T.

All of our existing bind functionality for elements is tied to UIChangeEventArgs which is an HTML/DOM concept

That's not how I was thinking of it. I thought we were describing the notion of UI events in a more abstract way, and they would still apply to other rendering technologies (such as how I gave meanings to them in the Flutter prototype). I know it just so happens that they precisely match HTML/DOM so it's not really honest to say it's more abstract.

Is it practical for us to have only UIEventArgs in Microsoft.AspNetCore.Components, and all the DOM-like subclasses (UIKeyboardEventArgs, etc.) in Microsoft.AspNetCore.Components.Web?

[Dispatcher] This should probably be an abstract class.

As suggested above, maybe we embrace that fact that there's only one implementation, call it RendererDispatcher, and give it responsibility for dispatching both renders and delegate invocations, since both do go to the renderer to be carried out.

I'm still complaining about the name of this. Parameter is really ParameterState or ParameterValue.

ParameterValue would be fine with me, though also I don't feel strongly and would be fine with leaving it as-is.

Cascading -> IsCascadingValue

Don't mind

ParameterCollection doesn't implement any interfaces, this could easily implement IReadOnlyDictionary<> and then we don't need ToDictionary().

It can't implement IReadOnlyDictionary or anything like that, because it's essential that getting a dictionary view of it implicitly clones the data. The underlying memory gets reused on subsequent renders, so you're not allowed to hold a copy of it for later use.

It's extremely wierd that SetParameterProperties is public, and extension method.

Sure, should be a regular method.

ParameterEnumerator doesn't implement IEnumerator

Any reason it should do? If people consume it through IEnumerator they are boxing it. If they enumerate directly using GetEnumerator they can avoid the boxing.

[Layouts] Make all of these sealed.

+1

LayoutAttribute should probably be in .Components

Agreed. The Layouts namespace doesn't need to exist at all.

Inherited = true for all by RouteAttribute

Sure

Should ElementRef be ElementReference

Before now, I would have agreed with Taylor's point that it's good to use the Ref abbreviation if it's paired with the @ref directive attribute (and we don't want to rename that to @reference). However since we're now codegenning the backing field, in most cases people don't have to guess what the type name is: the framework does it for them. So I now think it's fine to rename it to ElementReference if that fits our aesthetics better.

[SteveSandersonMS]

@NTaylorMullen

Why does this have to be public? Is it not a runtime concern where we're ok with it not existing in the ref assembly?

It's a limitation in System.Text.Json - we have to be able to deserialize incoming JSON representations of these references. If they can now deserialize private things then we should make it private.

[rynowak]

It won't deserialize private things, but we can now write a custom converter, so we should do that.

[davidfowl]

Wait it really can't deserialize private classes?

[rynowak]

It can deserialize private classes, they don't look at private properties.

[davidfowl]

Ah ok

[rynowak]

Are we sure that it's worth providing sync and async versions of all of our lifecycle methods?
It's wierd that we call both the sync and async methods instead of having the async ones call the sync ones.

There's a little subtle wierdness here.

Usually if we provide sync and async methods the async one calls the sync one. However, we call both, so if you override OnFooAsync and OnFoo, you'll see OnFooAsync called first and then OnFoo. This is a little surprising but you'd only notice if you override both methods.

However, this is also necessary because we expect inheritance to work with components. If you override OnFoo then you have no way to call the base class OnFooAsync - which locks in our choice to call both methods.

As another strange consequence, if you override OnFoo and your base class overrides OnFooAsync then their initialization gets called before yours does. So portraying these methods as co-equal choices has some problems. This effectively makes all Blazor lifecycle methods subject to the guidance Do not call virtual methods in constructors

I think this area is worth more thought.

[javiercn]

• I don't feel great about the fact that Invoke and InvokeAsync are different method instead of overloads (here, on IDispatcher and RenderHandle).

Do you mean that they appear on the renderer and on IDispatcher? Could we make anything the user doesn't interact with internal/protected/private?

AFAIK, the only thing that calls invoke is the RemoteRenderer could we make Invoke and InvokeAsync protected on the renderer and have the render handle expose it to components?

• Do we really want OnInit vs OnInitialized? Abbreviations in APIs always feel bad to me, even if they are known things like OnInit.

I personally prefer OnInit as its more inline with what other frameworks have, but I don't feel strongly about it.

• SetParametersAsync should probably be an explicit implementation, the user cannot duplicate the behaviour of the ComponentBase implementation, so it's not useful to override.

Is it useful to override it, do something before and then call base to get the default behavior?

• Should we expose a property HasRendered - basically provide some API for the common use case of initializing JS interop?

We discussed this in the past in depth and agreed that this was not necessary and that we can add it later if we have to. JS interop is rare except in the case you are building a component framework and at that point you can simply extend ComponentBase, add this yourself and then use this as the base class for your components.

• RenderHandle seems to wear two hats. Should we pass the dispatcher into the component instead?

Id rather hide the dispatcher from the user as much as possible. We should look into what the requirement is for the dispatcher to be public and see if we can take care of that.

• I don't love Configure as a name. Other ideas?

I'm fine with configure, I feel that if we decide it to call it ThisSuperSpecificThing and we find we need to use it for something else in the future then we are "locked". I would suggest Attach as an alternative, as in this attaches the component to the render, but I feel Configure is fine.

This should probably be an abstract class. We have a single implementation.

• We could also implement this on ComponentBase or RenderHandle (or both).

I would be happier if we could completely hide IDispatcher from the user, not sure if its possible. The main case for IDispatcher is to do Invoke(()= > StateHasChanged()) to trigger a new render, if we can avoid having the concept that's better.

• ParameterEnumerator doesn't implement IEnumerator.

Could this be made to use the struct enumerator optimization? Is that something we need to add now?

• Should ElementRef be ElementReference?

I'm fine with this.

Should we go into more detail on other APIs? RenderTree, RenderTreeBuilder, etc?

[javiercn]

Are we sure that it's worth providing sync and async versions of all of our lifecycle methods?
It's wierd that we call both the sync and async methods instead of having the async ones call the sync ones.

There are good reasons why we do this, base on the experience we want to achieve in multiple scenarios. The general principle is that we want to trigger a render before any async work happens and then we want to trigger an additional render after the async work completes. That is for example to allow scenarios where a developer is populating a grid, to produce an initial render with the head of the grid and no elements and to produce an additional render with the rows of the grid.

There is a small weirdness as you pointed out, which needs to be documented, but I believe this is a corner case, a base class can always override both methods and seal one of them so it can't be overriden by derived classes.

For example a base class can override both, oninit and oninitasync and seal oninit. I am with Steve in that we don't want people to have to around filling their apps with Task.CompletedTask or disabling warnings for async methods not awaiting anything.

So I'm happy with the trade-off we took here.