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

Container with parent assembly #50

Merged
merged 1 commit into from
Jan 12, 2016
Merged

Container with parent assembly #50

merged 1 commit into from
Jan 12, 2016

Conversation

Nikita2k
Copy link
Contributor

Container with parent assembly

This PR contains changes & tests for creating container with parent assembly.

Why?

Assume you have ApiManager, ViewController1 and ViewController2 in TabBar application. Both controllers asks API for some data. To implement this scheme we can create the following container hierarchy:

  • RootContainer (where ApiManager lives)
    • Controller1Container (all specific dependencies for VC1)
    • Controller2Container (all specific dependencies for VC2)

Later you decide to make a new feature: background upload (or tracking user location, etc). You have bunch of classes, that are responsible for that. Now you have two options:

  1. Create BackgroundUploadContainer and put all dependencies there
  2. Put all code to RootContainer

Both of them are not so good and shiny:

  1. You can't add it to current hierarchy, as you might want to add new "service" and it will lead you to multiple inheritance
  2. Your RootContainer will be soon a 10-screen-height file with lots of dependencies

Solutions?

We can have a RootAssembly = RootContainer + BackgroundUploadContainer. But with that solution it's impossible to have Controller1Container knowing about both of them (no container constructor accepts Assembler).

I propose to add such a constructor.

/// Instantiates a `Container` with its parent `Assembler`.
///
/// - Parameter parentAssembly: The parent `Assembler`.
public convenience init(parentAssembly: Assembler) {

Choose a reason for hiding this comment

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

Valid Docs Violation: Documented declarations should be valid. (valid_docs)

@yoichitgy
Copy link
Member

Thank you for your pull request with the enhancement and unit test👍 I would like to merge it.

Before merging the PR, I have a small suggestion. I think the requirement can be achieved by adding initializers to Assembler without modifiying Container like the following code.

public convenience init(parent: Assembler) {
    self.init(container: Container(parent: parent.container))
}

public convenience init(assemblies: [AssemblyType], propertyLoaders: [PropertyLoaderType]? = nil, parent: Assembler) throws {
    try self.init(assemblies: assemblies, propertyLoaders: propertyLoaders, container: Container(parent: parent.container))
}

As discussed in PR #35, I would like to keep Container independent from Assember for future source layout. What do you think about the suggestion?

@yoichitgy
Copy link
Member

This pull request relates to the following StackOverflow question:
http://stackoverflow.com/questions/34715871/swinject-migrating-to-assemblies

@yoichitgy yoichitgy added this to the v1.1.0 milestone Jan 12, 2016
@yoichitgy
Copy link
Member

@mowens, do you have any comment?

@mowens
Copy link
Contributor

mowens commented Jan 12, 2016

I like the idea of keeping the Container free of the Assembler. When I came up with the proposal for the Assembler the idea was to create a seamless way to organize components across multiple files. I think what @Nikita2k is looking for is a means to compose Assembler with another Assembler where their containers can have a child/parent relation. This can be done without the need to change the Container class. So I agree with @yoichitgy suggestion where you can compose an Assembler with another Assembler

This way access to the actual container is still restricted to only the Assembler which prevent manually adding service/component definitions outside of the scope of an AssemblyType. Then using @Nikita2k example, you could do something like this:

let rootAssembler = Assembler(assemblies: [
    RootAssembly(),
    BackgroundUploadAssembly()
])

let controllerAssembler = Assembler(assemblies: [
    Controller1Assembly(),
    Controller2Assembly()
], parent: rootAssembler)

@yoichitgy
Copy link
Member

Thank you @mowens😃

@Nikita2k
Copy link
Contributor Author

@yoichitgy @mowens Great idea, guys. Dagger (dependency injection for Java) has similar structure. I will reopen pull request (in fact, I have to revert my commits and start from scratch). Lets keep this one open if other people have suggestions :)

@yoichitgy
Copy link
Member

Thanks @Nikita2k for accepting our idea.

I think you can force-push to the same branch to update this PR without creating a new PR😀

git push --force origin feature_container_with_parent_assembly

/// - parameter parentAssembler: the baseline assembler
/// - parameter propertyLoaders: a list of property loaders to apply to the container
///
public init(assemblies: [AssemblyType], parentAssembler: Assembler, propertyLoaders: [PropertyLoaderType]? = nil) throws {

Choose a reason for hiding this comment

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

Valid Docs Violation: Documented declarations should be valid. (valid_docs)

Choose a reason for hiding this comment

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

Valid Docs Violation: Documented declarations should be valid. (valid_docs)

@Nikita2k
Copy link
Contributor Author

@yoichitgy I can't push to your repo:) We usually keep unsuccessful PR too, but if you like, I pushed changes here

@yoichitgy
Copy link
Member

Thank you @Nikita2k for updating the PR. Now I see your intention creating a new PR. I just don't have a policy to keep PRs before editing, so your update to this PR is ok.

I will investigate the messages from Hound CI, then will merge your PR. I've not updated my SwiftLint to 0.5.5 yet, and don't know why the code docs violating the lint.

@yoichitgy
Copy link
Member

Hound CI looks still using SwiftLint 0.4.0. This PR branch in my local environment has no violation with SwiftLint 0.4.0. It looks Hound CI incorrectly reported in this PR.

@yoichitgy yoichitgy merged commit 48a8504 into Swinject:master Jan 12, 2016
@yoichitgy
Copy link
Member

Thanks @Nikita2k for the enhancement. I was wondering whether the initializers should be convenience or not when I wrote the past comment. Your definitions are better because the initializers are not optional ones just for convenience. The argument order is also good regarding the default parameter 😃

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

Successfully merging this pull request may close these issues.

None yet

4 participants