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 ability to have multiple service types per definition #360

Closed
davidfowl opened this Issue Feb 12, 2016 · 12 comments

Comments

Projects
None yet
8 participants
@davidfowl
Member

davidfowl commented Feb 12, 2016

I want to be able to get the same instance for this type of definition:

public class Bar : IBar, IBaz
{
}
services.AddSingleton<IBar, Bar>();
services.AddSingleton<IBaz, Bar>();
var sp = services.BuildServiceProvider();
var i1 = sp.GetService<IBar>();
var i2 = sp.GetService<IBaz>();
Assert.Same(i1, i2); // Fails

This works but is less efficient.

services.AddSingleton<Bar>();
services.AddSingleton<IBar>(sp => sp.GetService<Bar>());
services.AddSingleton<IBaz>(sp => sp.GetService<Bar>());
var i2 = sp.GetService<IBaz>();
var i1 = sp.GetService<IBar>();
var i2 = sp.GetService<IBaz>();
Assert.Same(i1, i2); // Passes

This also works but I have to know the entire dependency graph to construct bar:

var bar = new Bar(); // Hope Bar has no services
services.AddSingleton<IBar>(bar);;
services.AddSingleton<IBaz>(bar);
var i2 = sp.GetService<IBaz>();
var i1 = sp.GetService<IBar>();
var i2 = sp.GetService<IBaz>();
Assert.Same(i1, i2); // Passes

We'd have to look at how to support this, should it be a service descriptor?

services.AddSingleton<IBar, IBaz, Bar>();

We also have to see if other DI containers support this.

@dadhi

This comment has been minimized.

Show comment
Hide comment
@dadhi

dadhi Feb 12, 2016

Contributor

DryIoc supports this via RegisterMany and more specifically via RegisterMapping.

Contributor

dadhi commented Feb 12, 2016

DryIoc supports this via RegisterMany and more specifically via RegisterMapping.

@seesharper

This comment has been minimized.

Show comment
Hide comment
@seesharper

seesharper Feb 12, 2016

Contributor

With regards to the API I think it is important to specify all interfaces along with the implementing type (or factory delegate) in one single registration method.

Consider this scenario

services.AddSingleton<IBar>(provider => new Bar());
services.AddSingleton<IBaz>(provider => new Bar());

In this scenario we have no way to look at the implementing type at registration time to determine if both interfaces should resolve to the same instance or not. The intent here could just as well be that we want two singletons, one for IBar and one for IBaz.

In my opinion the key to a service instance is the service type and the actual need for multiple service types pointing to the same instance should be so rare that your "less efficient" solution would work just fine.

I also would like to point out that more and more "expected behavior" is now being baked into this abstraction making it harder for third party containers to integrate.

Contributor

seesharper commented Feb 12, 2016

With regards to the API I think it is important to specify all interfaces along with the implementing type (or factory delegate) in one single registration method.

Consider this scenario

services.AddSingleton<IBar>(provider => new Bar());
services.AddSingleton<IBaz>(provider => new Bar());

In this scenario we have no way to look at the implementing type at registration time to determine if both interfaces should resolve to the same instance or not. The intent here could just as well be that we want two singletons, one for IBar and one for IBaz.

In my opinion the key to a service instance is the service type and the actual need for multiple service types pointing to the same instance should be so rare that your "less efficient" solution would work just fine.

I also would like to point out that more and more "expected behavior" is now being baked into this abstraction making it harder for third party containers to integrate.

@Eilon Eilon added this to the Backlog milestone Mar 7, 2016

@Eilon

This comment has been minimized.

Show comment
Hide comment
@Eilon

Eilon Mar 7, 2016

Member

Backlog.

Member

Eilon commented Mar 7, 2016

Backlog.

@davidfowl

This comment has been minimized.

Show comment
Hide comment
@davidfowl

davidfowl Mar 8, 2016

Member

I also would like to point out that more and more "expected behavior" is now being baked into this abstraction making it harder for third party containers to integrate.

This repository has 2 things, the default container and the abstraction. The 2 aren't coupled. It's conceivable that features can be added to the container without breaking the DI abstraction. It's really up to us to decide how that works. Right now we don't add features that we aren't consuming ourselves so it's a good forcing function.

Member

davidfowl commented Mar 8, 2016

I also would like to point out that more and more "expected behavior" is now being baked into this abstraction making it harder for third party containers to integrate.

This repository has 2 things, the default container and the abstraction. The 2 aren't coupled. It's conceivable that features can be added to the container without breaking the DI abstraction. It's really up to us to decide how that works. Right now we don't add features that we aren't consuming ourselves so it's a good forcing function.

@herecydev

This comment has been minimized.

Show comment
Hide comment
@herecydev

herecydev May 25, 2016

Any update on this now RC2 is shipped? Would be a nice to have for me.

herecydev commented May 25, 2016

Any update on this now RC2 is shipped? Would be a nice to have for me.

@Eilon

This comment has been minimized.

Show comment
Hide comment
@Eilon

Eilon May 26, 2016

Member

@herecydev it's not a feature we're planning to implement for v1.

Member

Eilon commented May 26, 2016

@herecydev it's not a feature we're planning to implement for v1.

@NKnusperer

This comment has been minimized.

Show comment
Hide comment
@NKnusperer

NKnusperer Aug 23, 2016

Any schedule on this?

NKnusperer commented Aug 23, 2016

Any schedule on this?

@viniciusmelquiades

This comment has been minimized.

Show comment
Hide comment
@viniciusmelquiades

viniciusmelquiades Oct 19, 2016

@davidfowl I know this issue is old, why is your less efficient solution "less efficient"? Is it related to performance, or just because we have to write more code?

If its just about writing more code, couldn't we simply write a few extension methods for this?

viniciusmelquiades commented Oct 19, 2016

@davidfowl I know this issue is old, why is your less efficient solution "less efficient"? Is it related to performance, or just because we have to write more code?

If its just about writing more code, couldn't we simply write a few extension methods for this?

@davidfowl

This comment has been minimized.

Show comment
Hide comment
@davidfowl

davidfowl Feb 12, 2017

Member

@viniciusmelquiades Whenever you call GetService<T> manually, you basically start a new dependency graph. What I mean by that is, imagine you have an object graph that looks like this:

public class A
{
    public A(B b) {} 
}

public class B
{
    public B(C c) {} 
}

public class C
{
    public C() {} 
}

The DI container can build a dependency tree that ends up generating code like this:

new A(new B(new C())) (roughly). When you throw in GetService<T> calls, the container can't optimize because it cannot see the entire dependency graph.

public class A
{
    public A(IServiceProvider provider) 
    {
        var b  = provider.GetService<B>();
    } 
}

public class B
{
    public B(IServiceProvider provider) 
    {
        var c = provider.GetService<C>();
    } 
}

public class C
{
    public C() {} 
}

new A(GetService<B>())
new B(GetService<C>()) new C()`

Each explicit GetService<T> call basically resets dependency resolution (there's a new dictionary lookup etc).

Member

davidfowl commented Feb 12, 2017

@viniciusmelquiades Whenever you call GetService<T> manually, you basically start a new dependency graph. What I mean by that is, imagine you have an object graph that looks like this:

public class A
{
    public A(B b) {} 
}

public class B
{
    public B(C c) {} 
}

public class C
{
    public C() {} 
}

The DI container can build a dependency tree that ends up generating code like this:

new A(new B(new C())) (roughly). When you throw in GetService<T> calls, the container can't optimize because it cannot see the entire dependency graph.

public class A
{
    public A(IServiceProvider provider) 
    {
        var b  = provider.GetService<B>();
    } 
}

public class B
{
    public B(IServiceProvider provider) 
    {
        var c = provider.GetService<C>();
    } 
}

public class C
{
    public C() {} 
}

new A(GetService<B>())
new B(GetService<C>()) new C()`

Each explicit GetService<T> call basically resets dependency resolution (there's a new dictionary lookup etc).

@yufeih

This comment has been minimized.

Show comment
Hide comment
@yufeih

yufeih Mar 23, 2017

Any update to implement this post v1?

yufeih commented Mar 23, 2017

Any update to implement this post v1?

@Eilon Eilon removed this from the Backlog milestone Mar 23, 2017

@Eilon

This comment has been minimized.

Show comment
Hide comment
@Eilon

Eilon Mar 23, 2017

Member

We're having a discussion this afternoon (though not sure if we'll get to this issue today; if not, we'll get to it soon).

Member

Eilon commented Mar 23, 2017

We're having a discussion this afternoon (though not sure if we'll get to this issue today; if not, we'll get to it soon).

@Eilon

This comment has been minimized.

Show comment
Hide comment
@Eilon

Eilon Mar 27, 2017

Member

Closing because there are no plans to support this. This would be a fairly large breaking change for not-that-large value.

Member

Eilon commented Mar 27, 2017

Closing because there are no plans to support this. This would be a fairly large breaking change for not-that-large value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment