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

Register / Resolve T or IEnumerable<T> #124

Closed
dansiegel opened this issue Jun 28, 2020 · 8 comments
Closed

Register / Resolve T or IEnumerable<T> #124

dansiegel opened this issue Jun 28, 2020 · 8 comments
Labels
bug Something isn't working

Comments

@dansiegel
Copy link
Owner

Description

As initially discussed in issue #115 we have an issue where IEnumerable may need to be resolved. This is a particular issue with Shiny integration where multiple delegates may be registered and will need to be resolved.

Problem

Prism registers services with an expectation that we will internally only want to resolve the last registration for any particular service. However it is possible for developers, or 3rd party plugins such as Shiny to require registering multiple services as is the case for Delegates. In this scenario it would be normal to see:

c.Register<ISomeDelegate, DelegateA>();
c.Register<ISomeDelegate, DelegateB>();

Expectations

What we need is a proper ruleset for the Container so that:

  • When we Resolve<ISomeDelegate>() we should get DelegateB
  • When we Resolve<IEnumerable<ISomeDelegate>>() we should get back a collection with both DelegateA and DelegateB.

NOTE: The registrations cannot directly reference the underlying container since this should transparently work with the Prism Container Abstractions and the integration where we convert from IServiceCollection to register each Service Descriptor with the Prism Container.

Observations

Currently we use the default IfAlreadyRegistered.Replace, this satisfies the first requirement, but fails on the second as we only get a collection with DelegateB.

If we do not set the IfAlreadyRegistered behavior or set it explicitly to Append, we succeed on the second requirement but fail on the first.

cc: @dadhi... any thoughts?

@dadhi
Copy link

dadhi commented Jun 29, 2020

@dansiegel Likely you'll need

rules.WithFactorySelector(Rules.SelectLastRegisteredFactory())

instead of IfAlreadyRegistered.Replace, which is rather destructive policy anyway (because you overriding the regisrations).

Here is the docs: https://github.com/dadhi/DryIoc/blob/master/docs/DryIoc.Docs/RulesAndDefaultConventions.md#resolving-from-multiple-default-services

@dansiegel
Copy link
Owner Author

perfect... that does indeed solve the issue. Thanks @dadhi

@dansiegel
Copy link
Owner Author

@dadhi sorry to keep bugging...

As was being discussed in @dahlbyk's PR... there is a related issue here that in another place we have a check for IsRegistered...

Since we assume IsRegistered should return true whether we are checking for T or IEnumerable. The following change was proposed, but I'm not sure if this is ideal or if it can be simplified.

return Instance.IsRegistered(type) ||
                Instance.IsRegistered(type, factoryType: FactoryType.Wrapper);

Also in further testing I realized that we do have an issue with keyed services not appending... is there a way to get keyed services to append instead of throwing?

The only work around I've found there is to force a Replace for keyed services

@dadhi
Copy link

dadhi commented Jun 29, 2020

The only work around I've found there is to force a Replace for keyed services

Yes, for keyed this is the only way I imagine at the moment, because the keyed services are supposed to be unique.

IsRegistered should return true whether we are checking for T or IEnumerable.

IEnumerable wrapper is always registered whether or not you are registered a service type, so that the enpty collection will be returned for the non registered type.
So you may just check for the registered type or check for the type.IsGeneric && type.GetGenericTypeDefinition() == typeof(IEnumerable<>)

@dansiegel
Copy link
Owner Author

Thanks @dadhi... we could certainly check if it's an IEnumerable<T> in the resolve. I feel that it would potentially create some bugs though if we did that as part of our IsRegistered check since you really don't have anything registered for it.

@dahlbyk
Copy link

dahlbyk commented Jun 29, 2020

we could certainly check if it's an IEnumerable<T> in the resolve. I feel that it would potentially create some bugs though if we did that as part of our IsRegistered check since you really don't have anything registered for it.

Since DryIoc supports resolving a few different collection types (everything implemented by object[], if I understand the code correctly), it seems preferable to avoid repeating that logic.

Is there a downside to checking if a wrapper is registered for the specified type? Expected behavior for resolving a collection for an unregistered type is an empty collection, which seems consistent with considering IEnumerable<> or whatever to indeed be registered.

@dadhi
Copy link

dadhi commented Jun 30, 2020

@dahlbyk

DryIoc supports resolving a few different collection types (everything implemented by object[]

It is correct. Plus its own LazyEnumerable.

Is there a downside to checking if a wrapper is registered for the specified type?

No downside I am aware of. That way you will check for whatever wrapper suported by DryIoc.

@dansiegel
Copy link
Owner Author

fixed by #126

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants