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

added ManualResolver #365

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

Rivukis
Copy link

@Rivukis Rivukis commented Aug 13, 2018

ManualResolver can be used for types where the initializer can not be manually defined and used. (i.e. UIViewControllers, UIViews, etc.)

I realize that there is already a solution to this problem for UIViewControllers using UIStoryBoards. However, this solution only works for that particular case. If the user wants to use a XIB or inject properties into a UIView then a different solution will be needed. Also, since that solution is using advanced topics in Objective-C, it can be difficult to extend it to cover these other similar scenarios.

ManualResolver is one solution that I've used personally. After using it in a couple projects, I thought that others might benefit from this solution as well. Please let me know what you think and/or how this solution could be improved.

Tests/SwinjectTests/ManualResolverSpec.swift Outdated Show resolved Hide resolved
Tests/SwinjectTests/BasicViewController.swift Outdated Show resolved Hide resolved
Tests/SwinjectTests/BasicViewController.swift Outdated Show resolved Hide resolved
Sources/ManualResolver.swift Outdated Show resolved Hide resolved
Sources/ManualResolver.swift Outdated Show resolved Hide resolved
Sources/ManualResolver.swift Outdated Show resolved Hide resolved
Sources/ManualResolver.swift Outdated Show resolved Hide resolved
Sources/ManualResolver.swift Outdated Show resolved Hide resolved
Sources/ManualResolver.swift Outdated Show resolved Hide resolved
Sources/ManualResolver.swift Outdated Show resolved Hide resolved
@Rivukis
Copy link
Author

Rivukis commented Sep 6, 2018

Can somebody please take a look at this PR?

@felipeflorencio
Copy link

Hey Rivukis would you mind to explain why this solution? I'm mean, looking to the implementation the purpose is after you resolve your instance you want to get the reference to this instance and pass / inject any other variable property right?

But you implementation already can be done using:
let container = Container() container.register(Animal.self) { _ in Cat() } container.register(Person.self) { _ in PetOwner2() } .initCompleted { r, p in let owner = p as! PetOwner2 owner.pet = r.resolve(Animal.self) }

Is this what you are looking for or it's different?

@felipeflorencio
Copy link

@Rivukis

@Rivukis
Copy link
Author

Rivukis commented May 13, 2019

Hey @felipeflorencio,
What I'm doing with ManualResolver is a little different. The main issue is with classes that are not initialized manually. This includes UIViewController and UIView. As I stated there is a solution for UIViewController, but only if it is inflated from a storyboard. I've seen other architectural designs that more heavily use UIViews instead of making each view component a UIViewController. Also using XIBs for UIViewControllers is beneficial in certain scenarios. In those cases Swinject will not be the one that initializes the object and won't know that it has been initialized. This is where Manual Resolver comes in. This allows you to tell Swinject that it is time to inject all the dependencies. The other nice thing is that it is not dependent on Objective-C specific solutions (Swinject-Storyboards requires swizzling which is not supported in Swift) which means it does not require the Objective-C runtime, bridging, etc. which can increase the binary size. I hope that answers your question. If not, please let me know.

@felipeflorencio
Copy link

Yep, but you still can do the same using init completed, because the initCompleted you receive the initialised instance the same way that you are doing in your implementation.

The difference is that swinject does this in two steps, first initialising, and the reason this is to make sure that the instance was fully loaded, for example.

Your implementation does this:

container.registerManualConstruction(BasicViewController.self) { (resolver, basicViewController) in

        let food = resolver.resolve(Food.self)!

        basicViewController.inject(food: food)

}

The same with that swinject has today would be:

container.register(BasicViewController.self) { _ in BasicViewController() } .initCompleted { (resolver, basicViewController) in

let food = resolver.resolve(Food.self)!

(basicViewController as! BasicViewController).inject(food: food)

}

And this work's with all scenarios, if you use NIB, if you need has the same name as your viewcontroller your viewcontroller will initialise your need and will maintain the same reference, but even is not a problem as initCompleted always return the instance that was completely initialised.

One of the down sizes, but it's how its works iOS is that we can't have a custom initialiser to inject our dependencies, that's why we need to inject on initCompleted, using method injection or variable injection.

Sorry if i'm not seeing the same, but from the source-code that you added I could reproduce the same behaviour using initCompleted
For the storyboard the same can be done but I would recommend use another method to register that is in the extension SwinjectStoryboard, will guarantee that your storyboard will be initialised having the save viewcontroller reference, so if you implement the initCompleted you will have 100% sure that is the instance that you are looking for and be able to inject again :)

@jakubvano What do you think? Can you give us your insigne ?

@Rivukis
Copy link
Author

Rivukis commented May 13, 2019

The scenarios you described are cases when you are the one calling init. If something else calls it (aka UIKit) then Swinject will not be invoked and therefore neither will initCompleted.

For instance if you have a custom UIView (i.e. MyView) that is being used in a storyboard for a UIViewController (i.e. MyViewController). When MyViewController is initialized, UIKit will create a MyView on its behalf. Since UIKit does not know about Swinject it will not ask Swinject to create it. Therefore, Swinject is out of the creation loop and has no way of getting a reference to the instance.

You can try this yourself if you set up the scenario I just described. Create a subclass of UIViewController and a subclass of UIView then reference the view in the storyboard for the view controller. Then register the view and add an initCompleted that calls a method that prints something out. When you run the app you will notice that nothing gets printed. You can also put a print statement in awakeFromNib() to make sure that the view is being inflated even though the injection method is not being called.

@Rivukis
Copy link
Author

Rivukis commented May 13, 2019

As a side note, I'm a big fan of Swinject and still use it. However, I'm a little disappointed that 9 months has gone by without any feedback on my pull request. At this point I feel like the developers are no longer maintaining it and it's in a state of "use at your own risk".

@codeman9
Copy link

codeman9 commented Jun 3, 2019

@Rivukis Maybe you should update your tests to use the scenario you described with a view and a view controller? Then it may be more clear that when something changes, the test fails?

@jakubvano
Copy link
Member

Thanks for the PR 👍 I'm sorry for taking this long to respond 😓

Even though this kind of functionality is achievable without an explicit Swinject API, IMO it does make sense to bake it in.

I would propose a couple of changes though:

  • I don't think we should provide an explicit global container in ManualResolver - it might restrict certain usages (e.g. when app uses more than a single container). I would probably just add the API for defining the "manual construction" and requesting injection, and leave the Container instance management to the application code.
  • We could probably ditch the ManualResolver entirely and implement methods directly as extensions on Container / Resolver
  • Maybe change the method names to mirror existing API?

Usage I have in mind could look something like this:

container.manualInitCompleted(Foo.self) { resolver, foo in  // maybe `externalInitCompleted`?
    foo.dep1 = resolver.resolve(Dep1.self)
    foo.dep2 = resolver.resolve(Dep2.self) 
    // alternatively, but we don't need to make this idiomatic - leave this on the developer instead
    foo.inject(dep1: resolver.resolve(Dep1.self)!, dep2: resolver.resolve(Dep2.self)!)
}

let foo = Foo()
container.inject(foo)

What are your thoughts?

@jakubvano jakubvano mentioned this pull request Jul 25, 2019
8 tasks
@Rivukis
Copy link
Author

Rivukis commented Aug 28, 2019

Hi @jakubvano,

Thank you for responding! I think those are all great ideas for improving this PR. Unfortunately, I have many projects I'm managing right now and do not have the time to make the updates. If you want to go ahead and make them feel free. Otherwise, I'll get to them "eventually", but fair warning it may be a while.

I'd also like to encourage others to make the suggested changes as well, if you have the cycles.

@BProg
Copy link
Contributor

BProg commented Jan 10, 2024

Hi there,

Based on the examples give in the comments, programmer can reference the container from the ViewController and resolve the required dependencies from there.

If the reason to not reference the container is to be able to use mocked dependencies from tests, then programmer should be able to register mocked dependencies in the container before running tests.
There is no code smell at using a container from the View if there are dependencies needed, the View will only resolve dependencies, not register. The container will be configured with before the class is created.

class Foo {
  lazy var dep1 = SharedUIContainer.resolve(Dep1.self)!
  lazy var dep2 = SharedUIContainer.resolve(Dep2.self)!
}

Would this work for the View and ViewController use cases?

@Rivukis
Copy link
Author

Rivukis commented Apr 1, 2024

Hi @BProg, I gave up on this after 9 months of waiting with no response and decided to move on. It became too difficult and time consuming to try and improve Swinject (i.e. this PR). Since then I treat Swinject as "use at your own risk" since it doesn't seem to be actively maintained (only based on this PR; didn't look at commit/bug history). I have used this solution in my personal projects using a helper cocoapod that I made for myself (thus not having to rely on Swinject maintainers).

It's now been another 5 years. I forgot this PR still existed. I don't have any opinions on what happens to it: update it, merge it, delete it. etc. No offense to you (in fact it's nice of you to try and catch up on things long forgotten), but I've given up any hope on contributing to Swinject.

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

6 participants