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
Closed

Add IComponentActivator #19642

wants to merge 13 commits into from

Conversation

stsrki
Copy link
Contributor

@stsrki stsrki commented Mar 6, 2020

Currently components are instantiated with Activator.CreateInstance that is located in the internal class ComponentFactory. There is no way for authors of third-party components to change how components will be created.

With this feature I introduce the IComponentActivator to allow components to be instantiated by custom implementation.

I think this PR will address these issues.

@stsrki stsrki requested a review from SteveSandersonMS as a code owner March 6, 2020 13:49
@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Mar 6, 2020
@dnfclas
Copy link

dnfclas commented Mar 6, 2020

CLA assistant check
All CLA requirements met.

@stsrki
Copy link
Contributor Author

stsrki commented Mar 6, 2020

A lot of test were falling because IComponentActivator was not specified. I'm not sure where to register IComponentActivator with ServiceCollection since there is a LOT of tests. So instead I have made some changes to my original idea and if IComponentActivator is not registered within DI it will fallback to Activator.CreateInstance(componentType).

Copy link
Contributor Author

@stsrki stsrki left a comment

Choose a reason for hiding this comment

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

@SteveSandersonMS After the last commit I think the commented lines can be safely removed but I wan't to see what you think before. tnx

@@ -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.

@@ -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.

@mkArtakMSFT
Copy link
Member

Thanks for your PR, @stsrki.
We plan to take a closer look at this after we're done with the Blazor WASM release in May.

@mkArtakMSFT
Copy link
Member

Update:
We're still busy wrapping up our upcoming Blazor WASM release, which is planned to go out in May. That forces us to delay some community PR reviews which target ASP.NET Core 5.0, so please expect some delays here. We will get to this as soon as we wrap up the Blazor work.

@krynium
Copy link

krynium commented Apr 28, 2020

@dotnetjunkie

/// </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.

stsrki and others added 2 commits June 30, 2020 11:29
@stsrki
Copy link
Contributor Author

stsrki commented Jun 30, 2020

I have updated this PR with latest changes from master branch!

@danroth27 I asked you couple of months ago when this PR will be given green light. The answer was probably for preview6 of .NET 5. Preview 6 is released but no info is given for this feature. Are there any plans soon for this?

@SteveSandersonMS You have been silent so far, but I would really like your feedback on this feature and how to proceed with it.

Sorry to bother you guys, I know you're already too busy! In case you need more info we can communicate over other channels, or we can proceed here. Just let me know what suits you best. Thanks!

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Jun 30, 2020

Thanks @stsrki, I appreciate your continued follow-up on this!

By now, we're all pretty convinced this is a good feature and should be included. I'm scheduling time for it later this week. If you don't hear any confirmation by the end of the week please feel free to ping again, but I do expect to be getting to it :)

This means we expect it to be included in the preview 8 release, so I'm tracking it on that milestone too.

@SteveSandersonMS SteveSandersonMS added this to the 5.0.0-preview8 milestone Jun 30, 2020
Comment on lines +29 to +33
var activator = serviceProvider.GetService<IComponentActivator>();

var instance = activator != null
? activator.CreateInstance(componentType)
: Activator.CreateInstance(componentType);
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.

/// <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?

@SteveSandersonMS
Copy link
Member

@stsrki For us to be able to consider this, can you please also add tests?

@stsrki
Copy link
Contributor Author

stsrki commented Jun 30, 2020

@stsrki For us to be able to consider this, can you please also add tests?

Sure, I will try to add them. There are some issue with the Components solution when I open it in VS, as I don't have all the references so the build is failing. Hopefully I will solve it.

@stsrki
Copy link
Contributor Author

stsrki commented Jun 30, 2020

@SteveSandersonMS I have made changes based on your input and also added some basic tests. If you need anything else please let me know.

@@ -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!

/// <inheritdoc />
public IComponent? CreateInstance(Type componentType)
{
return Activator.CreateInstance(componentType) as IComponent;
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: throw if null

@SteveSandersonMS
Copy link
Member

I'm following up on this in #23607, so will close this in favour of that.

@stsrki - your commits are over in #23607 now, so you will still get attribution for this contribution. Thanks very much!

@GTmAster
Copy link

Hi, any information when it will be released in asp.net Core 3.1?

@stsrki
Copy link
Contributor Author

stsrki commented Jul 27, 2020

@GTmAster As fa as I know the plan was to make it part of .Net 5 preview 8.

@GTmAster
Copy link

GTmAster commented Jul 27, 2020

@stsrki This is not good, we are really expecting this to be part of current asp.net version. So there is no way to use non-conforming DI libraries as Simple Injector now with blazor?

@egil
Copy link
Contributor

egil commented Jul 27, 2020

@GTmAster its only coming to .net 5. @SteveSandersonMS confirmed this in the related issue. And no, you cannot do it with other DI libraries, as the current way components are created is hardcoded inside Blazor's core.

@GTmAster
Copy link

@egil Thanks for clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants