Skip to content

Ability to resolve multiple implementations of same type#154

Open
kmaschke85 wants to merge 9 commits into
hmlongco:masterfrom
kmaschke85:master
Open

Ability to resolve multiple implementations of same type#154
kmaschke85 wants to merge 9 commits into
hmlongco:masterfrom
kmaschke85:master

Conversation

@kmaschke85

Copy link
Copy Markdown

This PR adds the ability to resolve all registered implementations for a given type, where the implementations are differentiated by names.
Please let me know what I can optimize.

@hmlongco

Copy link
Copy Markdown
Owner

So the first question is why? I'm not sure of the use case or what problem this will solve.

Secondarily, if you're nesting containers for mocking or modularization, then the current code only gets the first set of items it finds. It doesn't get "all" of them.

@kmaschke85

kmaschke85 commented Apr 28, 2022

Copy link
Copy Markdown
Author

Valid question!

To your first one: being able to resolve multiple instances of the same registered type is something I am used to when working with .Net. Microsofts Dependency Injection framework and Autofac provide options for that.
In my specific case I have multiple classes acting as handlers for some task and all of them need to perform their task. Makes it simple using DI for it so I can easily add new handlers.

Good catch on the nested containers, will look into that.

@kmaschke85

Copy link
Copy Markdown
Author

@hmlongco I addressed your feedback about the nested containers. Can you please review?

@hmlongco

hmlongco commented May 8, 2022

Copy link
Copy Markdown
Owner

Went through this again and found that it still suffers from a bug when handling multiple containers.

Functionally this mechanism gathers up all of the named containers and stuffs them into an array so you can call them as needed. Right?

So what happens if container B is a child of container A... and container A has a named registration of "Fred" that was supposed to override the Fred in container B? In the current implementation both Freds will be gathered, returned and executed... and I don't think that's the desired result.

…orrectly respecting the container hierarchy.
@kmaschke85

Copy link
Copy Markdown
Author

Didn't consider that. Made some additions and adjustments. @hmlongco

@hmlongco

Copy link
Copy Markdown
Owner

Terribly sorry for the delay. Been working on getting Factory, my latest DI system, out the door.

I'll put this in if you still want it, but one more change is needed; Throwing a fatal error if nothing is found doesn't make sense. Just return an empty array.

Comment thread Sources/Resolver/Resolver.swift Outdated
Comment thread Sources/Resolver/Resolver.swift Outdated
if let registrations = root.lookupAll(type) {
return registrations.compactMap { reg in return reg.resolve(resolver: root, args: args) }
}
fatalError("RESOLVER: '\(Service.self)' types not resolved.")

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

return []

Comment thread Sources/Resolver/Resolver.swift Outdated
if let registrations = lookupAll(type) {
return registrations.compactMap { reg in return reg.scope.resolve(registration: reg, resolver: self, args: args) }
}
fatalError("RESOLVER: '\(Service.self)' types not resolved.")

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

return []

…e found for looking up multiple implementations and fixed registration key
@kmaschke85

Copy link
Copy Markdown
Author

Addressed your PR review comments @hmlongco

Sorry for my delay this time (was on PTO).

@kostisJP

Copy link
Copy Markdown

Hello, we need that too! @hmlongco did you have time to review the PR?

@majie776

majie776 commented Oct 27, 2022 via email

Copy link
Copy Markdown

@grinloc grinloc left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No actually

@rahimlis

Copy link
Copy Markdown

any update on this? @grinloc @hmlongco

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants