From 87f870d65900f1d7bb0f54efae9e5f75b7b4a9dd Mon Sep 17 00:00:00 2001 From: Pranav K Date: Thu, 3 Mar 2022 12:11:01 -0800 Subject: [PATCH] Revert "Activate components by default" (#40526) Reverting type activation based on discussion in #40521 This reverts commit 197d527b62f2dc1474d115220531ab80fc9b43ae. This reverts commit 0e7409e8cb59b21c762b20ef63702a0026fd18a8. --- .../Components/src/ComponentFactory.cs | 87 ++++--------------- .../src/DefaultComponentActivator.cs | 22 +++++ ...NetCore.Components.WarningSuppressions.xml | 6 +- .../Components/src/RenderTree/Renderer.cs | 13 +-- .../Components/test/ComponentFactoryTest.cs | 49 +---------- ...e.Components.Forms.WarningSuppressions.xml | 2 +- ...ore.Components.Web.WarningSuppressions.xml | 14 ++- ...bly.Authentication.WarningSuppressions.xml | 4 +- ...icrosoft.JSInterop.WarningSuppressions.xml | 2 +- 9 files changed, 71 insertions(+), 128 deletions(-) create mode 100644 src/Components/Components/src/DefaultComponentActivator.cs diff --git a/src/Components/Components/src/ComponentFactory.cs b/src/Components/Components/src/ComponentFactory.cs index 4eab2e5f0872..5cbf330db9b4 100644 --- a/src/Components/Components/src/ComponentFactory.cs +++ b/src/Components/Components/src/ComponentFactory.cs @@ -5,7 +5,6 @@ using System.Diagnostics.CodeAnalysis; using System.Reflection; using Microsoft.AspNetCore.Components.Reflection; -using Microsoft.Extensions.DependencyInjection; using static Microsoft.AspNetCore.Internal.LinkerFlags; namespace Microsoft.AspNetCore.Components; @@ -15,71 +14,46 @@ internal sealed class ComponentFactory private const BindingFlags _injectablePropertyBindingFlags = BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic; - private static readonly ConcurrentDictionary _cachedInitializers = new(); - private readonly IComponentActivator? _componentActivator; + private static readonly ConcurrentDictionary> _cachedInitializers = new(); - public ComponentFactory(IComponentActivator? componentActivator) + private readonly IComponentActivator _componentActivator; + + public ComponentFactory(IComponentActivator componentActivator) { - _componentActivator = componentActivator; + _componentActivator = componentActivator ?? throw new ArgumentNullException(nameof(componentActivator)); } public static void ClearCache() => _cachedInitializers.Clear(); public IComponent InstantiateComponent(IServiceProvider serviceProvider, [DynamicallyAccessedMembers(Component)] Type componentType) { - if (_componentActivator is not null) - { - return InstantiateWithActivator(_componentActivator, serviceProvider, componentType); - } - - return InstantiateDefault(serviceProvider, componentType); - } - - private static IComponent InstantiateDefault(IServiceProvider serviceProvider, [DynamicallyAccessedMembers(Component)] Type componentType) - { - // This is thread-safe because _cachedInitializers is a ConcurrentDictionary. - // We might generate the initializer more than once for a given type, but would - // still produce the correct result. - if (!_cachedInitializers.TryGetValue(componentType, out var initializer)) - { - if (!typeof(IComponent).IsAssignableFrom(componentType)) - { - throw new ArgumentException($"The type {componentType.FullName} does not implement {nameof(IComponent)}.", nameof(componentType)); - } - - initializer = new(CreatePropertyInitializer(componentType), ActivatorUtilities.CreateFactory(componentType, Type.EmptyTypes)); - _cachedInitializers.TryAdd(componentType, initializer); - } - - return initializer.CreateDefault(serviceProvider); - } - - private static IComponent InstantiateWithActivator(IComponentActivator componentActivator, IServiceProvider serviceProvider, [DynamicallyAccessedMembers(Component)] Type componentType) - { - var component = componentActivator.CreateInstance(componentType); + var component = _componentActivator.CreateInstance(componentType); if (component is null) { - // A user implemented IComponentActivator might return null. + // The default activator will never do this, but an externally-supplied one might throw new InvalidOperationException($"The component activator returned a null value for a component of type {componentType.FullName}."); } - // Use the activated type instead of specified type since the activator may return different/ derived instances. - componentType = component.GetType(); + PerformPropertyInjection(serviceProvider, component); + return component; + } + private static void PerformPropertyInjection(IServiceProvider serviceProvider, IComponent instance) + { // This is thread-safe because _cachedInitializers is a ConcurrentDictionary. // We might generate the initializer more than once for a given type, but would // still produce the correct result. - if (!_cachedInitializers.TryGetValue(componentType, out var initializer)) + var instanceType = instance.GetType(); + if (!_cachedInitializers.TryGetValue(instanceType, out var initializer)) { - initializer = new(CreatePropertyInitializer(componentType)); - _cachedInitializers.TryAdd(componentType, initializer); + initializer = CreateInitializer(instanceType); + _cachedInitializers.TryAdd(instanceType, initializer); } - initializer.ActivateProperties(serviceProvider, component); - return component; + initializer(serviceProvider, instance); } - private static Action CreatePropertyInitializer([DynamicallyAccessedMembers(Component)] Type type) + private static Action CreateInitializer([DynamicallyAccessedMembers(Component)] Type type) { // Do all the reflection up front List<(string name, Type propertyType, PropertySetter setter)>? injectables = null; @@ -119,29 +93,4 @@ void Initialize(IServiceProvider serviceProvider, IComponent component) } } } - - private readonly struct ComponentInitializer - { - private readonly Action _propertyInitializer; - - private readonly ObjectFactory? _componentFactory; - - public ComponentInitializer(Action propertyInitializer, ObjectFactory? componentFactory = null) - { - _propertyInitializer = propertyInitializer; - _componentFactory = componentFactory; - } - - public IComponent CreateDefault(IServiceProvider serviceProvider) - { - var component = (IComponent)_componentFactory!(serviceProvider, Array.Empty()); - ActivateProperties(serviceProvider, component); - return component; - } - - public void ActivateProperties(IServiceProvider serviceProvider, IComponent component) - { - _propertyInitializer(serviceProvider, component); - } - } } diff --git a/src/Components/Components/src/DefaultComponentActivator.cs b/src/Components/Components/src/DefaultComponentActivator.cs new file mode 100644 index 000000000000..9ce7781a9235 --- /dev/null +++ b/src/Components/Components/src/DefaultComponentActivator.cs @@ -0,0 +1,22 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Diagnostics.CodeAnalysis; + +namespace Microsoft.AspNetCore.Components; + +internal class DefaultComponentActivator : IComponentActivator +{ + public static IComponentActivator Instance { get; } = new DefaultComponentActivator(); + + /// + public IComponent CreateInstance([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type componentType) + { + if (!typeof(IComponent).IsAssignableFrom(componentType)) + { + throw new ArgumentException($"The type {componentType.FullName} does not implement {nameof(IComponent)}.", nameof(componentType)); + } + + return (IComponent)Activator.CreateInstance(componentType)!; + } +} diff --git a/src/Components/Components/src/Microsoft.AspNetCore.Components.WarningSuppressions.xml b/src/Components/Components/src/Microsoft.AspNetCore.Components.WarningSuppressions.xml index 7596eb1bb2c1..ccde9a2ffa24 100644 --- a/src/Components/Components/src/Microsoft.AspNetCore.Components.WarningSuppressions.xml +++ b/src/Components/Components/src/Microsoft.AspNetCore.Components.WarningSuppressions.xml @@ -1,6 +1,6 @@  - + ILLink IL2026 @@ -41,7 +41,7 @@ ILLink IL2072 member - M:Microsoft.AspNetCore.Components.ComponentFactory.InstantiateWithActivator(Microsoft.AspNetCore.Components.IComponentActivator,System.IServiceProvider,System.Type) + M:Microsoft.AspNetCore.Components.ComponentFactory.PerformPropertyInjection(System.IServiceProvider,Microsoft.AspNetCore.Components.IComponent) ILLink @@ -59,7 +59,7 @@ ILLink IL2077 member - M:Microsoft.AspNetCore.Components.ComponentFactory.CreatePropertyInitializer(System.Type) + M:Microsoft.AspNetCore.Components.ComponentFactory.CreateInitializer(System.Type) ILLink diff --git a/src/Components/Components/src/RenderTree/Renderer.cs b/src/Components/Components/src/RenderTree/Renderer.cs index ca89362d42b6..51550c2b9957 100644 --- a/src/Components/Components/src/RenderTree/Renderer.cs +++ b/src/Components/Components/src/RenderTree/Renderer.cs @@ -64,7 +64,7 @@ public event UnhandledExceptionEventHandler UnhandledSynchronizationException /// The to be used when initializing components. /// The . public Renderer(IServiceProvider serviceProvider, ILoggerFactory loggerFactory) - : this(serviceProvider, loggerFactory, GetComponentActivatorOrDefault(serviceProvider)!) + : this(serviceProvider, loggerFactory, GetComponentActivatorOrDefault(serviceProvider)) { // This overload is provided for back-compatibility } @@ -87,8 +87,10 @@ public Renderer(IServiceProvider serviceProvider, ILoggerFactory loggerFactory, throw new ArgumentNullException(nameof(loggerFactory)); } - // Even though IComponentActivator is not marked as nullable, we do allow null because that's how the framework internally indicates - // that we should use default activation logic. + if (componentActivator is null) + { + throw new ArgumentNullException(nameof(componentActivator)); + } _serviceProvider = serviceProvider; _logger = loggerFactory.CreateLogger(); @@ -97,9 +99,10 @@ public Renderer(IServiceProvider serviceProvider, ILoggerFactory loggerFactory, internal HotReloadManager HotReloadManager { get; set; } = HotReloadManager.Default; - private static IComponentActivator? GetComponentActivatorOrDefault(IServiceProvider serviceProvider) + private static IComponentActivator GetComponentActivatorOrDefault(IServiceProvider serviceProvider) { - return serviceProvider.GetService(); + return serviceProvider.GetService() + ?? DefaultComponentActivator.Instance; } /// diff --git a/src/Components/Components/test/ComponentFactoryTest.cs b/src/Components/Components/test/ComponentFactoryTest.cs index 6a2675c21e7d..653ed8d1e7e3 100644 --- a/src/Components/Components/test/ComponentFactoryTest.cs +++ b/src/Components/Components/test/ComponentFactoryTest.cs @@ -12,7 +12,7 @@ public void InstantiateComponent_CreatesInstance() { // Arrange var componentType = typeof(EmptyComponent); - var factory = new ComponentFactory(null); + var factory = new ComponentFactory(new DefaultComponentActivator()); // Act var instance = factory.InstantiateComponent(GetServiceProvider(), componentType); @@ -22,30 +22,12 @@ public void InstantiateComponent_CreatesInstance() Assert.IsType(instance); } - [Fact] - public void InstantiateComponent_CreatesInstance_WithTypeActivation() - { - // Arrange - var componentType = typeof(ComponentWithConstructorInjection); - var factory = new ComponentFactory(null); - - // Act - var instance = factory.InstantiateComponent(GetServiceProvider(), componentType); - - // Assert - Assert.NotNull(instance); - var component = Assert.IsType(instance); - Assert.NotNull(component.Property1); - Assert.NotNull(component.Property2); - Assert.NotNull(component.Property3); // Property injection should still work. - } - [Fact] public void InstantiateComponent_CreatesInstance_NonComponent() { // Arrange var componentType = typeof(List); - var factory = new ComponentFactory(null); + var factory = new ComponentFactory(new DefaultComponentActivator()); // Assert var ex = Assert.Throws(() => factory.InstantiateComponent(GetServiceProvider(), componentType)); @@ -113,7 +95,7 @@ public void InstantiateComponent_IgnoresPropertiesWithoutInjectAttribute() { // Arrange var componentType = typeof(ComponentWithNonInjectableProperties); - var factory = new ComponentFactory(null); + var factory = new ComponentFactory(new DefaultComponentActivator()); // Act var instance = factory.InstantiateComponent(GetServiceProvider(), componentType); @@ -147,31 +129,6 @@ public Task SetParametersAsync(ParameterView parameters) } } - public class ComponentWithConstructorInjection : IComponent - { - public ComponentWithConstructorInjection(TestService1 property1, TestService2 property2) - { - Property1 = property1; - Property2 = property2; - } - - public TestService1 Property1 { get; } - public TestService2 Property2 { get; } - - [Inject] - public TestService2 Property3 { get; set; } - - public void Attach(RenderHandle renderHandle) - { - throw new NotImplementedException(); - } - - public Task SetParametersAsync(ParameterView parameters) - { - throw new NotImplementedException(); - } - } - private class ComponentWithInjectProperties : IComponent { [Inject] diff --git a/src/Components/Forms/src/Microsoft.AspNetCore.Components.Forms.WarningSuppressions.xml b/src/Components/Forms/src/Microsoft.AspNetCore.Components.Forms.WarningSuppressions.xml index cd43d81a8f0f..e19838cbe120 100644 --- a/src/Components/Forms/src/Microsoft.AspNetCore.Components.Forms.WarningSuppressions.xml +++ b/src/Components/Forms/src/Microsoft.AspNetCore.Components.Forms.WarningSuppressions.xml @@ -1,6 +1,6 @@  - + ILLink IL2026 diff --git a/src/Components/Web/src/Microsoft.AspNetCore.Components.Web.WarningSuppressions.xml b/src/Components/Web/src/Microsoft.AspNetCore.Components.Web.WarningSuppressions.xml index 750434411f3c..db0f1efbb1b1 100644 --- a/src/Components/Web/src/Microsoft.AspNetCore.Components.Web.WarningSuppressions.xml +++ b/src/Components/Web/src/Microsoft.AspNetCore.Components.Web.WarningSuppressions.xml @@ -1,6 +1,18 @@  - + + + ILLink + IL2026 + member + M:Microsoft.AspNetCore.Components.Web.Infrastructure.JSComponentInterop.SetRootComponentParameters(System.Int32,System.Int32,System.Text.Json.JsonElement,System.Text.Json.JsonSerializerOptions) + + + ILLink + IL2026 + member + M:Microsoft.AspNetCore.Components.Web.WebEventData.ParseEventArgsJson(Microsoft.AspNetCore.Components.RenderTree.Renderer,System.Text.Json.JsonSerializerOptions,System.UInt64,System.String,System.Text.Json.JsonElement) + ILLink IL2062 diff --git a/src/Components/WebAssembly/WebAssembly.Authentication/src/Microsoft.AspNetCore.Components.WebAssembly.Authentication.WarningSuppressions.xml b/src/Components/WebAssembly/WebAssembly.Authentication/src/Microsoft.AspNetCore.Components.WebAssembly.Authentication.WarningSuppressions.xml index bbb730763fa6..358d7fae2dda 100644 --- a/src/Components/WebAssembly/WebAssembly.Authentication/src/Microsoft.AspNetCore.Components.WebAssembly.Authentication.WarningSuppressions.xml +++ b/src/Components/WebAssembly/WebAssembly.Authentication/src/Microsoft.AspNetCore.Components.WebAssembly.Authentication.WarningSuppressions.xml @@ -1,4 +1,4 @@ - + @@ -14,4 +14,4 @@ M:Microsoft.Extensions.DependencyInjection.WebAssemblyAuthenticationServiceCollectionExtensions.<>c__1`1.<AddAuthenticationStateProvider>b__1_0(System.IServiceProvider) - \ No newline at end of file + diff --git a/src/JSInterop/Microsoft.JSInterop/src/Microsoft.JSInterop.WarningSuppressions.xml b/src/JSInterop/Microsoft.JSInterop/src/Microsoft.JSInterop.WarningSuppressions.xml index 2ac15203d51b..a1c7043de593 100644 --- a/src/JSInterop/Microsoft.JSInterop/src/Microsoft.JSInterop.WarningSuppressions.xml +++ b/src/JSInterop/Microsoft.JSInterop/src/Microsoft.JSInterop.WarningSuppressions.xml @@ -1,6 +1,6 @@  - + ILLink IL2026