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

Add IComponentActivator #19642

Closed
wants to merge 13 commits into from
19 changes: 19 additions & 0 deletions src/Components/Components/src/ComponentActivator.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Copyright (c) .NET Foundation. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: rename file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that, I will rename the file now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries! I'm working on this PR now so if possible it would be good not to make further changes in case we conflict with each other. Is that OK?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already renamed a file, didn't think you would be that fast to start working :)
OK, I will stop with changes from now on so you can continue. Please let me know if you need anything else.

PS. I really appreciate all your work. Thank you!

// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;

namespace Microsoft.AspNetCore.Components
{
/// <summary>
/// Default implementation of component activator.
/// </summary>
public class ComponentActivator : IComponentActivator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with other parts of the framework, can you rename this to DefaultComponentActivator?

{
/// <inheritdoc />
public object CreateInstance(Type componentType)
stsrki marked this conversation as resolved.
Show resolved Hide resolved
{
return Activator.CreateInstance( componentType );
stsrki marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
8 changes: 7 additions & 1 deletion src/Components/Components/src/ComponentFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Linq;
using System.Reflection;
using Microsoft.AspNetCore.Components.Reflection;
using Microsoft.Extensions.DependencyInjection;

namespace Microsoft.AspNetCore.Components
{
Expand All @@ -25,7 +26,12 @@ private readonly ConcurrentDictionary<Type, Action<IServiceProvider, IComponent>

public IComponent InstantiateComponent(IServiceProvider serviceProvider, Type componentType)
{
var instance = Activator.CreateInstance(componentType);
var activator = serviceProvider.GetService<IComponentActivator>();

var instance = activator != null
? activator.CreateInstance(componentType)
: Activator.CreateInstance(componentType);
Comment on lines +29 to +33
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of asking the service provider for IComponentActivator on every call, we'll need a way of caching the activator instance. This might force us to eliminate the static ComponentFactory.Instance and instead have the renderer get its own separate component factory instance during its own construction.

If you're able to look into doing that then great, otherwise I'll see what I can do to fix this up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great if you would look into this after I merge the changes. You have a lot more insight of what needs to be changed and where.


if (!(instance is IComponent component))
{
throw new ArgumentException($"The type {componentType.FullName} does not implement {nameof(IComponent)}.", nameof(componentType));
Expand Down
20 changes: 20 additions & 0 deletions src/Components/Components/src/IComponentActivator.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;

namespace Microsoft.AspNetCore.Components
{
/// <summary>
/// Represents an activator that can be used to instantiate components.
/// </summary>
public interface IComponentActivator
{
/// <summary>
/// Creates an component of the specified type using that type's default constructor.
/// </summary>
/// <param name="componentType">The type of component to create.</param>
/// <returns>A reference to the newly created component.</returns>
object CreateInstance(Type componentType);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
object CreateInstance(Type componentType);
IComponent CreateInstance(Type componentType);

Should this return an IComponent instead of just object? It seems that all components are IComponents, and having this returned here might save a cast higher up in the call stack.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're probably right, but I just wanted to make it behave the same as Activator.CreateInstance(componentType) which is returning an object.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we require component instances to implement IComponent, I think it only makes sense if the activator returns that type.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ public static IServerSideBlazorBuilder AddServerSideBlazor(this IServiceCollecti
services.AddScoped<IJSRuntime, RemoteJSRuntime>();
services.AddScoped<INavigationInterception, RemoteNavigationInterception>();
services.AddScoped<AuthenticationStateProvider, ServerAuthenticationStateProvider>();
services.AddScoped<IComponentActivator, ComponentActivator>();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be now removed since activator will fallback to default implementation anyway.


services.TryAddEnumerable(ServiceDescriptor.Singleton<IConfigureOptions<CircuitOptions>, CircuitOptionsJSInteropDetailedErrorsConfiguration>());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ internal static void AddViewServices(IServiceCollection services)
services.TryAddScoped<NavigationManager, HttpNavigationManager>();
services.TryAddScoped<IJSRuntime, UnsupportedJavaScriptRuntime>();
services.TryAddScoped<INavigationInterception, UnsupportedNavigationInterception>();
services.TryAddScoped<IComponentActivator, ComponentActivator>();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also can be removed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't the default activator be a singleton? It doesn't hold any state.


services.TryAddTransient<ControllerSaveTempDataPropertyFilter>();

Expand Down