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

Integration with Swift 5.2 Features? #164

Closed
corban123 opened this issue Aug 18, 2020 · 20 comments
Closed

Integration with Swift 5.2 Features? #164

corban123 opened this issue Aug 18, 2020 · 20 comments

Comments

@corban123
Copy link

corban123 commented Aug 18, 2020

Is there a way to make use of Swift 5.1/2's PropertyWrappers to allow something along the lines of

    @Injected var userStateMachine: UserStateMachine
    @Injected var keyValueStore: KeyValueStore
    @Injected var bundle: BundleProviding
    @Injected var touchIdService: TouchIDManaging
    @Injected var status: SystemStatusProviding
    ...
}

rather than requiring all parameters for a class to go through the initializer?

@sebastianv1
Copy link
Collaborator

@corban123 That could be possible. Cleanse also supports property injection so that you don't have to require all the parameters to go through the initializer.

We've internally had some discussions whether or not we should make use of property wrappers but haven't found a solid use case. I'd love to hear more about how property wrappers might help you with Cleanse.

@corban123
Copy link
Author

In my mind, the main use case for us is a cleanliness factor for instantiating and presenting view controllers that require multiple services if not relying on a View Model for making network calls. That, or being able to avoid requiring an assisted factory for adding the View Model to the graph(Be it unscope) to be able to access the services that exist in graph. To make sure I clarify, what I'm speaking about is having

class VCA { 
  let model: VCAModel

 init(model: VCAModel) {
  ...
}
... 
}

struct VCAModel { 
let serviceA: ServiceA
let serviceB: ServiceB
let serviceC: ServiceC
}

Where Services A-C all exist in the graph. Requiring an assistedFactory(If i'm correct) for VCA due to the fact that VCAModel requires those three services(As it needs to be initialized with them which means it has to be placed in a module) is kind of a pain in the butt.

Personally, it would be easier if it looked more like

struct VCAModel { 
@Injected let serviceA: ServiceA
...
}

as you would then be able to initialize the model without the services being in the initializer or using a factory

@corban123
Copy link
Author

Property injection works, however, it requires its own function, and as per your own documentation, is a second class citizen.

@corban123
Copy link
Author

This would also allow usage with SwiftUI as custom initializers aren't a big thing for it

@sebastianv1
Copy link
Collaborator

In the example you posted, the only way to make @Injected grab an instance for any of the services (ServiceA, ServiceB, or ServiceC) would require a global container object. This is an antipattern for DI frameworks as it breaks scoping, the idea of building a unique graph every time we construct a RootComponent, and makes testability very difficult.

In order to avoid a global container, we could expose a service locator with each graph you construct, and utilize referencing the enclosed self with property wrappers (https://github.com/apple/swift-evolution/blob/master/proposals/0258-property-wrappers.md#referencing-the-enclosing-self-in-a-wrapper-type), but Cleanse has taken a hard stance against publicly exposing service locator objects. We've held this opinion because it leaks the idea of a DI framework into your objects/dependencies, making reusability in a manual DI world difficult.

This would also allow usage with SwiftUI as custom initializers aren't a big thing for it

I know utilizing environmentObject is common, but is it true writing initializers aren't common? Let me play around with ways to improve property injection and its API. We've always considered it to be more of a second-class API because constructor injection is generally a better practice (but understand its need for storyboards and other objects iOS constructs for us), but it seems as though property injection might serve a valid purpose within the realm of SwiftUI.

@corban123
Copy link
Author

That all makes sense, and I'm unsure if I'd die on the hill saying that SwiftUI doesn't use custom initializers as the language is so young and unused that common patterns aren't common yet, but it's not something I see often.

While I've got you here, what is the expectation of usage with a Coordinator pattern where an object is expected to initialize and present 4+ view controllers? Is there an expectation of quite a few assisted a factories or have I misunderstood something

@sebastianv1
Copy link
Collaborator

That all makes sense, and I'm unsure if I'd die on the hill saying that SwiftUI doesn't use custom initializers as the language is so young and unused that common patterns aren't common yet, but it's not something I see often.

Agreed, and total valid to bring up. We'd like Cleanse to evolve with the standard patterns that might emerge with SwiftUI.

Assisted factories are helpful for objects that require "just in time" dependencies but don't need to be bound into the graph. One common example is a list view and an assisted factory for a detail view that requires an id upon selection. So with the coordinator pattern, it mostly depends on what your views require. I'll also point out now that the coordinator pattern with it's parent -> child bidirectional relationship will require utilizing Cleanse's WeakProvider type to resolve the cyclical dependency.

@corban123
Copy link
Author

corban123 commented Aug 19, 2020

I've been meaning to ask as I've been curious, but what's the main reason for having a ViewController get added to the graph? I can't imagine a situation where the view controller is considered a "dependency" for another object, at least not in such a way that I'd compare it to a networking object.

Also, what's the preferred way of registering mocks such that if you run a unit test, an object you're testing will use the mock service(or whatever has been registered) rather than the real object?

@sebastianv1
Copy link
Collaborator

I guess I would flip the question and ask why not have a ViewController in the graph? If the object does not exist in the graph then it becomes more difficult swap out implementations since another object lower in the dependency tree is constructing your view controller. Every object, view controller, or even view model are nodes in your dependency graph at the end of the day. Which is a nice segue into your second question about swapping for tests.

There are a number of ways to do this, and at the core it's simply using a different binding for your unit test graph.

One approach would be having a separate unit test component from your main app component. Consider a service protocol NetworkService {}:

struct AppRootComponent: Cleanse.RootComponent {
    static func configure(binder: Binder<Unscoped>) {
        binder.bind(NetworkService.self).to(factory: RealNetworkService.init)
    }
    // ...
}

struct TestRootComponent: Cleanse.RootComponent {
    static func configure(binder: Binder<Unscoped>) {
        binder.bind(NetworkService.self).to(factory: FakeNetworkService.init)
    }
}

Another way is if we put those bindings above into separate modules, we could make the component generic.

struct RealNetworkModule: Cleanse.Module {
    static func configure(binder: Binder<Unscoped>) {
        binder.bind(NetworkService.self).to(factory: RealNetworkService.init)
    }
}

struct FakeNetworkModule: Cleanse.Module {
    static func configure(binder: Binder<Unscoped>) {
        binder.bind(NetworkService.self).to(factory: FakeNetworkService.init)
    }
}

struct AppRootComponent<Config: Cleanse.Module>: Cleanse.RootComponent {
    static func configure(binder: Binder<Unscoped>) {
        binder.include(module: Config.self)
    }
}

And then to build your real root component you could do:
let factory = ComponentFactory.of(AppRootComponent<RealNetworkModule>.self)
or for your fake
let factory = ComponentFactory.of(AppRootComponent<FakeNetworkModule>.self)

@corban123
Copy link
Author

This makes sense! One last question. When you're adding a ViewController to the graph, where are you expected to bind it to? Generally the examples have surrounded binding a model/service to a VC/Model's init, but generally you don't initialize ViewControllers with a view controller.

@sebastianv1
Copy link
Collaborator

I'm not sure if I totally follow what you're asking, but a binding is telling Cleanse how to construct some object type. For instance, binder.bind(Int.self).to(value: 3) is telling Cleanse, "here's how to construct an integer." So when you want to add a view controller to the dependency graph, you would add a binding for your view controller. And then as you're alluding to, when we want to construct a ViewController, it may have a dependency on another view controller that we can inject. Maybe this code sample below will help:

struct MyModule: Cleanse.Module {
    static func configure(binder: Binder<Unscoped>) {
        // This is a "binding" for `ViewControllerA`. We're telling Cleanse _how_ to construct `ViewControllerA`.
        binder
            .bind(ViewControllerA.self)
            .to { 
                return ViewControllerA()
            }
        // This is a "binding" for `ViewControllerB`. 
        binder
            .bind(ViewControllerB.self)
            .to {  (viewControllerA: ViewControllerA) in
                // This is how we construct `ViewControllerB`. Notice how it has a dependency on `ViewControllerA`.
                // Cleanse internally looks for a "binding" of `ViewControllerA` to resolve it, which we created above.
                return ViewControllerB(vcA: viewControllerA)
            }

    }
}

As for where you are expected to bind it, you can put the binding into your root component configure function, or break it up into distinct Cleanse.Module instances like in the example above. As long as you binder.include(module: MyModule.self into your root component, then Cleanse can find each of the bindings. Does that answer your question?

@corban123
Copy link
Author

Close, but not entirely. This actually brings up my point. I can't imagine a situation where you initialize a ViewController(ViewControllerB here) with another ViewController, generally those get initialized in the view controller at the necessary time. Lets say in this example, ViewControllerA looks like

class ViewControllerA { 

...
let serviceA: ServiceA
}

In the configure you include whatever module contains ServiceA, and then bind ViewControllerA to the constructor. My question is then how do you access ViewControllerA from ViewControllerB without requiring it to go through the initializer.

Say ViewControllerB does

class ViewControllerB { 


...

func tappedButton() { 
...
present(viewControllerA) 
}

Now I don't want to initialize with ViewControllerA because it might not get loaded since the user might not press the button and I don't want to waste the memory on it, but I do want it in the graph. How do I initialize ViewControllerA right before presenting it, unless the answer is "Use and AssistedFactory".

@sebastianv1
Copy link
Collaborator

Now I don't want to initialize with ViewControllerA because it might not get loaded since the user might not press the button and I don't want to waste the memory on it, but I do want it in the graph.

Ah so in this case you would use Provider<ViewControllerA> as a dependency. Cleanse will create this type for you in the dependency graph when create a binding, so you simply add it as:

class ViewControllerB: UIViewController {
    let viewControllerA: Provider<ViewControllerA>
    func tappedButton() {
        // `ViewControllerA`'s `init` function will not get called until you invoke `get()`.
        // This is effectively a lazy instantiation.
        let vc = viewControllerA.get()
    }
}

@corban123
Copy link
Author

That Provider still needs to be initialized though in the initializer correct?

@sebastianv1
Copy link
Collaborator

Correct, but that's a very cheap initialization. To take a small step back, having a view controller accept another view controller in its initializer follows the good pattern of practicing dependency injection (with or without a framework). Instead of having ViewControllerB be responsible for constructing ViewControllerA, pushing that concern above helps make testing and swapping implementations much easier. I think this piece by Martin Fowler is considered a great reference for explaining why we want to do this.

@corban123
Copy link
Author

The worry I have are for things that present a bunch of different view controllers, such as Coordinators. At that point your initializer can balloon quite a bit.

@sebastianv1
Copy link
Collaborator

sebastianv1 commented Aug 21, 2020

Right, your initializer will grow to include all the objects you depend upon. However, reducing this initializer by just constructing the objects inside won't reduce your object's complexity. If you're concerned about growing initializers, I'd suggest looking into how to break up your coordinators into smaller pieces that are easier to reason about (and thus require less direct dependencies to be injected). Does the coordinator pattern support breaking pieces up into sub-coordinators and composing them into a root coordinator?

@corban123
Copy link
Author

That's a valid point. I guess as a developer, reading objects being initialized in the initializer feels less on-time than having what's being injected described in the file, but not getting fully resolved prior to when it's needed. Honestly, it's the same thing, but I worry that as developers, we see things in the initializer as "ready to go" rather than "this can still set things up"

@sebastianv1
Copy link
Collaborator

we see things in the initializer as "ready to go" rather than "this can still set things up"

Exactly, which is where the idea of providers and factories come into play when you want certain dependencies to still set things up, or not be constructed right away on init.

@sebastianv1
Copy link
Collaborator

@corban123 I'm going to close this issue for now, but thanks for the discourse! If you want to continue this discussion, don't hesitate to reach out personally.

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

No branches or pull requests

2 participants