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

[DI] Pass the requested type to the implementation factory to enable dynamic scenarios when the type isn't known statically #478

Closed
thomaslevesque opened this issue Nov 8, 2018 · 17 comments

Comments

@thomaslevesque
Copy link
Member

Is your feature request related to a problem? Please describe.

I'm trying to enable lazy injection of services, i.e. if I registered a service for IFoo, I'd like to be able to inject a Lazy<IFoo> or Func<IFoo>. This is useful to solve circular dependency issues; Unity does this automatically. So I'd like to be able to do this:

services.AddTransient(typeof(Lazy<>), provider => 
{
    ...
});

However, in the factory delegate, I don't have access to the requested type, so I can't dynamically create a Lazy<T> of the appropriate type.

Describe the solution you'd like

Having overloads of Add* that accept a Func<IServiceProvider, Type, object> would solve this. However it looks like it would be non-trivial, since the Func<IServiceProvider, object> signature is used all the way down to the low-level implementation...

Alternatively, implementing lazy injection like in Unity would solve my immediate problem, but would be less extensible.

Describe alternatives you've considered

Using a third-party container like Unity. But I'd rather keep using the built-in DI container.

Additional context

N/A

@dotnetchris
Copy link

+1

@pakrym
Copy link

pakrym commented Nov 14, 2018

Duplicate: dotnet/aspnetcore#3036

@pakrym pakrym closed this as completed Nov 14, 2018
@dotnetchris
Copy link

dotnetchris commented Nov 16, 2018

@pakrym how did you close this issue citing it as a duplicate of a closed issue? This is obviously not a closed issue.

Don't turn github issues into Microsoft connect!

@pakrym
Copy link

pakrym commented Nov 16, 2018

My bad, linked the wrong issue, it's dup of aspnet/DependencyInjection#474 aspnet/DependencyInjection#588 aspnet/DependencyInjection#498

In short, implementing this feature requires changing ServiceDescriptor type and provider specification and is a large breaking change. Other containers support this scenario natively too.

@dotnetchris
Copy link

@pakrym every single one of those is closed, one of these has to be open. Out of everything you linked aspnet/DependencyInjection#474 looks oldest.

This is clearly not a resolved issue. Unless the official answer is literally WILL NOT EVER SUPPORT, NO MATTER WHAT YOU THINK.

@thomaslevesque
Copy link
Member Author

Unless the official answer is literally WILL NOT EVER SUPPORT, NO MATTER WHAT YOU THINK.

From what I understand from the linked issues, that's exactly what the official answer is... I agree that it would be useful (or I wouldn't have opened the issue), but it's a fundamentally breaking change, and would also break compatibility with third party IoC containers.

@dotnetchris
Copy link

dotnetchris commented Nov 19, 2018

@thomaslevesque I don't understand why this wouldn't be trivial to do in compatible manner.

Can someone link to relevant source? i feel like this would be a 5 minute tweak. @pakrym

@pakrym
Copy link

pakrym commented Nov 19, 2018

It would require a change to https://github.com/aspnet/DependencyInjection/blob/master/src/DI.Abstractions/ServiceDescriptor.cs#L108 property type.

Which would lead to compiler error everywhere it's invoked. It's an easy change to do inside just this repo but the DependencyAbstraction.Package is used by every compatible service provider implementation out there and all of them would get broken,

@dotnetchris
Copy link

        public Func<IServiceProvider, object> ImplementationFactory { get; }

        public Func<IServiceProvider, Type, object> ImplementationFactory2 { get; }

        internal Type GetImplementationType()
        {
            if (ImplementationType != null)
            {
                return ImplementationType;
            }
            else if (ImplementationInstance != null)
            {
                return ImplementationInstance.GetType();
            }
            else if (ImplementationFactory2 != null)
            {
                var typeArguments = ImplementationFactory2 .GetType().GenericTypeArguments;

                Debug.Assert(typeArguments.Length == 3);

                return typeArguments[2];
            }
            else if (ImplementationFactory != null)
            {
                var typeArguments = ImplementationFactory.GetType().GenericTypeArguments;

                Debug.Assert(typeArguments.Length == 2);

                return typeArguments[1];
            }

            Debug.Assert(false, "ImplementationType, ImplementationInstance or ImplementationFactory must be non null");
            return null;
        }

etc

@pakrym
Copy link

pakrym commented Nov 19, 2018

I don't see what are you trying to say with this example.

@dotnetchris
Copy link

@pakrym You wouldn't have to change ImplementationFactory, you could have two and then there's no compatibility issues. Not necessarily the prettiest or optimal, but better than what we have today.

@pakrym
Copy link

pakrym commented Nov 19, 2018

But every other container would have to support it or it just won't work with new kind of ServiceDescriptor. It's not a compilation breaking change but a breaking change nevertheless

@dotnetchris
Copy link

dotnetchris commented Nov 19, 2018

@pakrym then it should get slated that v+1 should break and just modify the factory directly.

@pakrym
Copy link

pakrym commented Nov 19, 2018

We haven't broken other service provider implementation since 1.0 and this feature is certainly not worth it. What it's trying to accomplish already exists in other providers

@dotnetchris
Copy link

@pakrym this is literally one of the most critical features of a container

@mravko
Copy link

mravko commented Sep 5, 2019

Much needed feature. Please consider to implement it.

@thomaslevesque
Copy link
Member Author

Much needed feature. Please consider to implement it.

As much as I'd like it, I don't think it will ever be implemented. The team's position is quite clear: they don't want to introduce a breaking change, and it's already possible to do it by using a different container.

In the meantime, I have a solution for my initial problem (lazily resolved service):

public static class ServiceCollectionExtensions
{
    public static IServiceCollection AddLazyResolution(this IServiceCollection services)
    {
        return services.AddTransient(typeof(LazyService<>));
    }
}

public class LazyService<T>
{
    private readonly Lazy<T> _service;
    
    public LazyService(IServiceProvider serviceProvider)
    {
        _service = new Lazy<T>(() => serviceProvider.GetRequiredService<T>());
    }
    
    public T Value => _service.Value;
}

Usage:

public void ConfigureServices(IServiceCollection services)
{
    services.AddLazyResolution();
    services.AddSingleton<Foo>();
    services.AddSingleton<Bar>();
}

class Foo
{
    public Foo(Bar bar)
    {
        Bar = bar;
    }
    
    public Bar Bar { get; }
}

class Bar
{
    private readonly LazyService<Foo> _foo;
    
    public Bar(LazyService<Foo> foo)
    {
        _foo = foo;
    }
    
    public Foo Foo => _foo.Value;
}

@ghost ghost locked as resolved and limited conversation to collaborators Dec 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants