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

Dynamically resolve differently for same protocl #96

Open
jakubvano opened this issue Apr 11, 2018 · 5 comments
Open

Dynamically resolve differently for same protocl #96

jakubvano opened this issue Apr 11, 2018 · 5 comments

Comments

@jakubvano
Copy link
Member

From @gilgol on April 9, 2018 11:40

I'm trying to dynamically resolve a protocol ViewModeling that 2 different view models are conforming to..
I am able to do it on the service level (one protocol that 2 services are conforming to, and 2 different viewModel that each consume a different service can resolve the required one using a specific name)

I'm struggling with doing this on the viewController level, trying to inject the same view controller a specific viewModel (both conforms as mentioned to the same protocol).

I believe the issue is with SwinjectStoryboard that doesn't let me instantiate a view controller using its storyboard id (like I would normally do), and in addition define few different names that will be resolved on runtime.

Am I missing something?

Just realised it might be more appropriate as a stackoverflaw post, so here is a link to the post:
https://stackoverflow.com/questions/49732300/how-to-inject-the-right-viewmodel-to-a-single-viewcontroller

Copied from original issue: Swinject/Swinject#345

@jakubvano
Copy link
Member Author

Indeed this functionality is currently not quite supported by SwinjectStoryboard - I have answered more in depth in the SO issue.

@jakubvano
Copy link
Member Author

From @gilgol on April 10, 2018 11:33

First of all thanks for the quick response.
I've read your answer on SO, and after that I've checked the open PR and had an idea of an alternative approach I would love hearing your thoughts on it:

Currently when calling instantiateViewController(withIdentifier: "identifier") you will instantiate the relevant viewController that has storybaordID equals to the given identifier from the storyboard.
You than inject it with the right dependancy, and if there are few resolving available you distinguish using the name which is, as far as I understand, being compared to the swinjectRegistrationName runtime attribute.

What if you would specify in the storyboard the available swinjectRegistrationNames (in a form of array) and extend instantiateViewController(withIdentifier: "identifier") to have another String variable (something like: instantiateViewController(withIdentifier: "identifier", name: "aName").
This way, you could run over the entire list of available names (taken from swinjectRegistrationNames) and if one matches with the requested name just resolve and inject it...

@jakubvano
Copy link
Member Author

Thanks for the idea 👍

If we wanted to add the name parameter to the instantiateViewController(withIdentifier:) we wouldn't even need to use array of names in the storyboard - swinjectRegistrationName parameter is used to "select" the correct storyboardInitCompleted definition (simplifying here a bit 😉). We could simply use the method parameter instead.

However, I'm not quite sure whether this is the right path to take: Initial idea behind the SwinjectStoryboard was to allow users relying on "hidden" instantiation of view controllers from storyboard (e.g. segues, child containers, ..) to inject dependencies. Once you need to use names in instantiation you cannot rely on these mechanisms anyway, so you might as well instantiate the view controllers manually:

container.register(MyViewController.self, name: "name") {
    let vc = SwinjectStoryboard.create(name: "MyStoryboard", bundle: nil).instantiateViewController(withIdentifier: "identifier")
    vc.viewModel = $0.resolve(MyViewModel.self)!
    return viewModel
}

let vc = SwinjectStoryboard.defaultContainer.resolve(MyViewController.self, name: "name")

My fear is that once we start extending instantiateViewController method we might end up reimplementing more and more of a "native" Swinject API. Which requires a lot of work and IMHO doesn't add much value compared to the usage described above.

BTW: this argument works for adding resolution arguments (SwinjectStoryboard #62), which is taking the framework in the same direction.

@jakubvano
Copy link
Member Author

From @gilgol on April 10, 2018 14:54

Well, according to your answer and a brief look on the implementation in SwinjectStoryboard.swift, it looks like swinjectRegistrationName is behaving as the name parameter which "select" the right
storyboardInitCompleted definition.

Will adding a default value nil for this potential name: String? extra param and choosing it over the swinjectRegistrationName param (or, alternatively, to remove the need of this attribute completely) still make you feel it might end up reimplementing more and more of a "native" Swinject API?

Because as I see it - from the moment you've instantiated the view controller from the storyboard, all you do is injecting the dependancy and return the injected viewController.
Maybe I'm missing something, but will something like that:

 public override func instantiateViewController(withIdentifier identifier: String) -> UIViewController {
        SwinjectStoryboard.pushInstantiatingStoryboard(self)
        let viewController = super.instantiateViewController(withIdentifier: identifier)
        SwinjectStoryboard.popInstantiatingStoryboard()

        injectDependency(to: viewController, nil)

        return viewController
    }


  private func injectDependency(to viewController: UIViewController, name: String?) {
        guard !viewController.wasInjected else { return }
        defer { viewController.wasInjected = true }

        // This could be omitted, or alternatively be used only if name is nil?
        let registrationName = name ?? viewController.swinjectRegistrationName

        // Xcode 7.1 workaround for Issue #10. This workaround is not necessary with Xcode 7.
        // If a future update of Xcode fixes the problem, replace the resolution with the following code and fix storyboardInitCompleted too.
        // https://github.com/Swinject/Swinject/issues/10
        if let container = container.value as? _Resolver {
            let option = SwinjectStoryboardOption(controllerType: type(of: viewController))
            typealias FactoryType = ((Resolver, Container.Controller)) -> Any
            let _ = container._resolve(name: registrationName, option: option) { (factory: FactoryType) in factory((self.container.value, viewController)) as Any } as Container.Controller?
        } else {
            fatalError("A type conforming Resolver protocol must conform _Resolver protocol too.")
        }

        for child in viewController.childViewControllers {
            injectDependency(to: child, name)
        }
    }

public func instantiateViewController(withIdentifier identifier: String, name: String? = nil) -> UIViewController {
        SwinjectStoryboard.pushInstantiatingStoryboard(self)
        let viewController = super.instantiateViewController(withIdentifier: identifier)
        SwinjectStoryboard.popInstantiatingStoryboard()

        injectDependency(to: viewController, name: name)

        return viewController
    }

could cause any issues and will move the library to the direction you mentioned is wrong?

@jakubvano
Copy link
Member Author

jakubvano commented Apr 11, 2018

@gilgol We are on the same page, the example implementation you've came up with is more or less what I had in mind when thinking about this feature 😄

I'm not so much worried about the issues with the implementation itself - IMO it would work just fine - but with extending the SwinjectStoryboard API in general. Unfortunately, I haven't considered this when we were discussing the API for arguments.. Any extension will introduce more complexity to the codebase, and we should weigh that against the benefits it brings.

From my point of view this kind of usage diverges from the original purpose of SwinjectStoryboard, and can be implemented with just the Swinject API. On the other hand I can see the argument for maintaining consistency with the cases where you need to use the SwinjectStoryboard.

However, I don't have much real world experience with SwinjectStoryboard - I am not using it in my bigger projects.
It would be useful to know what other users think about this API / how they solve similar issues.

Thoughts, anyone?

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

No branches or pull requests

1 participant