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

Parent child container support for Dip #171

Merged
merged 1 commit into from
Dec 12, 2017

Conversation

john-twigg-ck
Copy link
Contributor

Support for Parent Child containers

First off, I have to say I love Dip, and you've done an amazing job at the API. I heavily vetted Dip along with Swinject, Cleanse and a few other containers. I've used Unity .Net for over a decade, Guice for years. But yours was perfect for us especially around syntax.

Motivation

This PR takes its inspiration from (Dagger Subcomponents)[https://google.github.io/dagger/subcomponents]

The motivations is to provide support for Container to have a hierarchy, such that some containers can be denoted as Parent containers and others as Child and sibling containers.

Child containers will be isolated from each other, and parent containers will be isolated from the child containers.

Rules

  • Parent containers cannot directly access the dependencies of a child container.
  • Child containers can only have one single parent
  • Child containers can access all the dependencies of a parent container, but not any of the dependancies of a sibling container.
  • Reuse : Dependencies that are resolved in a parent container are reused during the lifecycle of the overall resolve() invocation, similar to existing behavior
  • Overriding: All nested calls to resolve start over with the container with whom the resolve call was invoked If both a child and a parent include a registry, the child's registry will always be attempted first, failing up the hierarchy. EVEN if the dependency is auto-wired from within a parent class, all new resolve calls are re-initiated with the child container from which the call originated.
  • Collaboration containers are attempted first, before parent containers.

Changes to existing behavior

Much effort was made to maintain and only extend the existing behavior, however there were knock on effects with respect to resolving ambiguities that I believe are an overall improvement, making resolution more deterministic and easier to reason about.

Attached is a screenshot of a Unit Test I added before/after this change that focuses on Collaborators.

  • Before: I believe the collaboration of the containers prior to this change were somewhat ambiguously resolved. In the example resolving the dependency from either collaborated container would bias the results towards the container that contained the AutoWire. In fact order of associating collaborators can have a big impact on the results.

  • After: With the addition of the behavior that all nested resolves are reinitiated with the originating container, I believe the behavior is now more deterministic and predictable. In this example, the container2 has registered a SerivceImpl2 under its Service protocol. When you resolve using conatiner2, anything requesting a Service will receive a ServiceImpl2 implementation which seems reasonable since you just went to the trouble of registering it and requesting it.

screen shot 2017-08-02 at 9 52 55 pm

Further Improvements

Perhaps we should restrict the mixture of collaboration and parent child relationships.

@ilyapuchka
Copy link
Collaborator

@john-twigg-ck thanks for your work!
I think if we go with this we should deprecate collaboration in favour of parent-child.
@jtwigg can you please look if this implementation will solve the issues you had with collaboration?

@john-twigg-ck
Copy link
Contributor Author

@ilyapuchka @jtwigg and @john-twigg-ck are in fact the same people! Sorry for the confusion. I launched this PR from our corp account.

It does solve the problems I had with collaboration.

@ilyapuchka
Copy link
Collaborator

I thought so 😂

@jtwigg
Copy link

jtwigg commented Aug 3, 2017

@ilyapuchka I'll try and maintain come consistency here from this account.

So thats great news that you are in favor of the change.

If you want to deprecate collaboration then a few points need to be discussed

  1. There's the possibility that a child could have multiple parents. There's nothing excluding it. I would avoid using it in my implementations because it would degrade my ability to understand how the dependancies are being resolved, but it would open up ability to aggregate containers together to create a behavior. You'd have order sensitive initialization.
//Always tries rootContainerFirst
let container  = DependencyContainer(parents: [rootContainer,sideEffectContainer])
  1. I think you should consider creating an actual "Module" concept, since thats what collabs were trying to do at some level. Here. Swinject Assemblies or Guice Modules actually do a really good job of this and I'm using this concept in our production app. Something like
let rootContainer = DependancyContainer(module: [RootModule(), LoginModule(),AdsModule()])
let childContainer = DependancyContainer(module : [ChildModule()],parent: rootContainer)

class RootModule : Module {
     register(container: DependancyContainer ) {
         container.register(.singleton) { ...} 
     }
}
  1. Getting more specific, I think that your
self.resolvedInstances.resolvedInstances 
self.resolvedInstances.resolvableInstances 

should actually be

self.context.resolvedInstances 
self.context.resolvableInstances  

since context shares the lifecycle of the resolve() call and resolvedInstances shares the lifecycle of the container (good for singletons). ** Thoughts?**

@ilyapuchka
Copy link
Collaborator

I'll be able to get back to this and really discuss this only on a weekend, unfortunately, so keep this thought =)

@ilyapuchka ilyapuchka merged commit 12738a6 into AliSoftware:develop Dec 12, 2017
@jtwigg
Copy link

jtwigg commented Dec 13, 2017

@ilyapuchka Awesome. Thanks for merging.
I'll post another PR with our latest updates and fixes.
We've been using it in anger exponentially more over the last half year.

@ilyapuchka ilyapuchka mentioned this pull request Apr 26, 2018
ilyapuchka added a commit that referenced this pull request Apr 28, 2018
* Revert "fix merge conflict"

This reverts commit aff01c5.

* Revert "Merge branch 'develop' into develop"

This reverts commit a8dd47c, reversing
changes made to 9e7bd51.
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.

None yet

3 participants