[swift-users] Feedback for Dependency Injection Framework?

[Ian Terrell]

Hi Tadeas,

I don't generally use the word "dependency" to cover types that are used by
another type; those are simply implementation details of the outer type.
There is no VC_B until VC_A decides to present one, so I do not it a
dependency; there's nothing to inject. The creation and presentation of
VC_B is just "what happens" when showNestedViewController() is called.

That is perhaps a better phrasing for my point, thank you. The creation of
VC_B is an implementation detail of VC_A, which is why VC_A's true
dependencies are so numerous.

As an aside, in apps I have found that sometimes navigation wants to be
considered as a separate concern. In that case injecting a protocol based
VC factory or a "router" is the way to go in my opinion.

Thanks,
Ian

On Thu, Jun 16, 2016 at 5:10 PM, Tadeas Kriz <tadeas at brightify.org> wrote:

> Hi Ian,
>
> Shouldn't the `ViewController_B` be a dependency and not be instantiated
> inside a `ViewController_A`? Or ViewControllers/Controllers are not
> considered dependencies?
>
> Thanks,
> Tadeas
>
> On Thu, Jun 16, 2016 at 10:50 PM Ian Terrell via swift-users <
> swift-users at swift.org> wrote:
>
>> Hi Mike,
>>
>> Thanks for your response. :) Let me address your points. I apologize for
>> the length; this is a topic I care about and I know my co-workers are
>> reading it. :)
>>
>> I'm going to edit the message history for brevity.
>>
>> My first point of confusion is that these libraries seem to be less about
>>>> dependency injection and more about service location. For instance, in your
>>>> GitHub example in the playground, we see the following:
>>>>
>>>> struct GithubListMembersServiceImpl : GithubListMembersService {
>>>>>     let githubURL: TaggedProvider<GithubBaseURL>
>>>>
>>>>
>>>>>
>>>>     func listMembers(organizationName: String, handler: [String] -> ())
>>>>> {
>>>>>         let url = githubURL.get()
>>>>
>>>>
>>>> To my mind it doesn't look like githubURL was injected. It looks like
>>>> it was looked up in a service locator.
>>>>
>>> I'm not very familiar with the service locator pattern to be honest,
>>> however, I can assure you that the base URL is being injected. I think
>>> there may be some confusion on either how the GithubListMembersServiceImpl
>>> is being constructed, or tagged providers in general.
>>>
>>
>> I don't believe I'm confused over those features, and I do understand
>> that githubURL is being injected from your point of view. What I mean
>> though is that it's being injected from a global location, not from the
>> main call site.
>>
>> In service location code that needs a service asks for it, and the
>> service locator provides it. In this case you are asking for a
>> GithubBaseURL. The (in my opinion problematic) pattern occurs even if you
>> rewrite the service implementation as you did:
>>
>> struct GithubListMembersServiceImpl : GithubListMembersService {
>>>     let githubURL: *NSURL*
>>
>>
>>>
>>     func listMembers(organizationName: String, handler: [String] -> ()) {
>>>         let url = githubURL
>>>
>>> .URLByAppendingPathComponent("orgs/\(organizationName)/public_members")
>>>         let dataTask = urlSession.dataTaskWithURL(url) ...
>>
>>
>> Definitely from the perspective of GithubListMembersServiceImpl the
>> githubURL is injected. That makes it nicely testable and well designed.
>> However in this case the service location is merely once removed (and is
>> already present in the prior version as well): now your code is asking a
>> service locator for the GithubListMembersService (client code will now have
>> to call GithubListMembersService.get()).
>>
>> I see dependency injection as being given your dependencies. I see
>> service location as asking for your dependencies. In Cleanse and Dagger and
>> Guice I feel like the dependencies are being asked for (the get() method is *asking
>> something else* to be provided the call site with an instance).
>>
>> In this case, I believe your "component" is the service locator.
>>
>> and then change the binding statement to be
>>>
>>>
>>>>       binder
>>>>        .bind()
>>>>        .to(value: NSURL(string: "https://api.github.com")!)
>>>
>>>
>>> The downside of this in an actual application is that you may want to
>>> inject other URLs. These "Type Tags"  TaggedProviders are essentially
>>> just a ...
>>>
>>
>> And this is why I think Dagger and Guice and Cleanse are improperly
>> categorized as dependency injection frameworks. I think in reality they are
>> service locators and factories. Dependency injection needs no framework!
>>
>> With manual dependency injection, when different services want different
>> URLs you simply inject them normally.
>>
>> call site A: MembersServiceImpl(baseURL: productionURL)
>> call site B: MembersServiceImpl(baseURL: stagingURL)
>>
>> That requires that the call sites manage that dependency in order to know
>> what to pass (feature not a bug).
>>
>>
>>> I've always thought of dependency injection as being a tool for avoiding
>>>> global state. But in this case it seems to me like the service locator is
>>>> globally managed. The language of service locators is even present in
>>>> Guice's excellent motivation page
>>>> <https://github.com/google/guice/wiki/Motivation> (my emphasis):
>>>>
>>>>
>>> The only "global" state are things that are declared singletons. (which
>>> are still only singletons in the scope of each time one calls build). They
>>> have very similar semantics to singletons in Guice
>>> <https://github.com/google/guice/wiki/Scopes> (however no custom scopes
>>> yet as mentioned above).
>>>
>>
>> Global is not quite accurate, but it is not terribly close. Your
>> Component object tree is what I mean, and I view it (perhaps wrongly) as
>> "semi-global." I get the sense that in most applications there will end up
>> being exactly one. A better word is "external". Now your objects rely on
>> external state, whether it is global or semi-global.
>>
>>
>>> For the former, there was feedback regarding the constructors being
>>> implicitly made for structs in this reddit thread
>>> <https://www.reddit.com/r/swift/comments/4o2lno/squarecleanse_swift_dependency_injection/d49pu6c>
>>> .
>>>
>>
>> You pointed out in that thread that these frameworks make it easier to
>> keep up with new dependencies, reordered initializer parameters, etc — but
>> all that hiding I fear would lead to poor architectural decisions in
>> practice. Too many objects end up depending on too many things, because
>> those dependencies are hidden from them. That means that real refactors
>> (trying to use these objects in un-forethought contexts, etc) become more
>> difficult, and your app actually becomes more tightly coupled (it's so easy
>> to .get() that other object!).
>>
>>  Guice will inspect the annotated constructor, and *lookup* values for
>>> each parameter.
>>>
>>> Cleanse actually achieves the same thing, but without reflection. I
>>> attempted to explain it in this part of the README
>>> <https://github.com/square/Cleanse/blob/master/README.rst#terminating-methods-top1factory-p1---e-1st-arity>,
>>> but I'll be honest, I struggled while attempting to make it both simple and
>>> detailed enough to convey what's going on.
>>>
>>
>> I think I get what's going on. I only meant that the words "look up"
>> imply service location to me. In my mind you're registering (via bind())
>> service factory instructions to the service locator. At runtime, the
>> service locator/factory *looks up *how to build it and returns you an
>> instance.
>>
>> Big snippet below for all the context:
>>
>> Another point I've heard people talk about is that these libraries help
>>> as dependencies grow. I hope this isn't a straw man, as I interpret the
>>> following line in your README in that way.
>>>
>>>>
>>>> As the functionality of this app grows, one may add arguments to
>>>>> RootViewController and its dependencies as well as more modules to satisfy
>>>>> them.
>>>>>
>>>>
>>>> The argument seems to go that this is easy to maintain:
>>>>
>>>> init(a: A)
>>>>
>>>>
>>>> But this is difficult:
>>>>
>>>> init(a: A, b: B, c: C, d: D, e: E, ...)
>>>>
>>>>
>>>> The argument goes further by saying that it becomes especially
>>>> difficult as you may need to pass the dependencies through the some nodes
>>>> of the object graph that seem not to need them. For instance, in the object
>>>> graph A -> B -> C, if C has a dependency on Foo but B does not, why should
>>>> B know about Foo?
>>>>
>>>> But I find that argument unconvincing on two grounds. First, I believe
>>>> an object's dependencies are the union of its and its children's
>>>> dependencies. In the example above, B has an implicit dependency on Foo.
>>>> "Skipping" passing them around or instantiating them in the owning class is
>>>> actually only hiding an object's true dependencies by using global state.
>>>> Second, as a single object's dependencies become more unwieldy it seems in
>>>> practice to indicate an architectural problem where objects are doing too
>>>> much. These problems are related, as the architectural problem may not be
>>>> as visible when the true dependencies are hidden!
>>>>
>>>
>>> So this is an interesting point. I'm a strong believer that DI solves
>>> this issue really well.
>>>
>>> Let's take this example (I think its similar to the straw man you are
>>> referring to):
>>>
>>> https://gist.github.com/mikelikespie/c54a017677265322df7eb785d9f36345
>>>
>>> Basically, VC_A, needs serviceA, and a VC_B to do its job, however, for
>>> it to create VC_B it needs VC_B's dependencies + its transitive
>>> dependencies which happen to be serviceA, serviceB, serviceC.
>>>
>>> So I agree with the arguments made in the straw man, e.g. don't need to
>>> change the constructor hierarchy all the way down when VC_D needs a new
>>> service or needs to start depending on something. It takes a pretty large
>>> example to start to see these benefits, but its kind of like how you don't
>>> really see a benefit from a quicksort over a bubble sort until you have a
>>> lot of items to sort.
>>>
>>> So if we refactor as is (without turning anything into protocols) to use
>>> Cleanse, we come up with something like:
>>>
>>> https://gist.github.com/mikelikespie/3abe371bf7f7ab67f71d6cfa22c0145d
>>>
>>> Now, say serviceD incurred a new dependency, one would just add it to
>>> its constructor and make sure the dependency is configured... don't have to
>>> change any of the other view controllers in the hierarchy.
>>>
>>
>> Ok! So, we're on the exact same page with the code and the problem, and
>> perhaps even the same conclusion, but the exact opposite solutions.
>>
>> I'm a strong believer that DI solves this issue really well. ... It takes
>>> a pretty large example to start to see these benefits
>>>
>>
>> I believe that true dependency injection is the solution, but I think
>> that Cleanse/Dagger/Guice aren't really providing that in a useful way. I
>> think the refactored version using Cleanse is significantly worse than the
>> non-refactored version. Additionally, I think the larger the project (in
>> your view where it shines), the worse the problem gets (in my view where it
>> hinders).
>>
>> Concretely: In both examples, ViewController_A has dependencies on
>> services A, B, C, and D. But only in the non-Cleanse version is that
>> visible. This means that it has hidden, non-explicit dependencies, that
>> have to be looked up somewhere else. And they're looked up from the service
>> locator/factory VC_A_Component. We cannot use ViewController_A outside of
>> the context of that special knowledge and setup.
>>
>> These frameworks aren't removing dependencies, they're hiding them. This
>> makes it less easy to reason about the code.
>>
>> As an example of reasoning, a reader of the component/service locator
>> setup in the Cleanse example has to wonder: Why am I binding Services A, B,
>> C, and D and VCs A, B, C, and D just to create a single VC_A? Now the
>> reader has to inspect all of the view controllers to find out! After which
>> she learns, well, VC_A uses S_A and can create a VC_B, which uses S_B and
>> can create a VC_C, which uses S_C and can create a VC_D, and VC_D uses S_D.
>> All of that information must be discovered and tied together just to get()
>> a single VC_A!
>>
>> In the non-Cleanse example, the question is: Why do I need to instantiate
>> VC_A with Services A, B, C, and D? And the answer is simple and comes from
>> a single view controller (the one we care about): because VC_A uses S_A can
>> create a VC_B and VC_B uses services B, C, and D.
>>
>> That's a huge contrast in reasoning, and it's because
>> Cleanse/Dagger/Guice are hiding information in external service locators.
>>
>> I suppose I might phrase my point of view as saying that I don't believe
>> in transitive dependencies; and definitely not in treating them
>> differently. You have your dependencies, and by depending on something else
>> that has dependencies, those transitive dependencies are de facto yours,
>> directly. They get no special treatment, because they cannot; treating them
>> specially relies on external state. That is breaking encapsulation.
>>
>>
>>> This gets me to the next point, testing. VC_A's purpose is to call
>>> serviceA and present a view controller. To properly unit test its
>>> functionality, it may not be necessary to provide a concrete implementation
>>> of the VC it wants to present. In general if one can test a component and
>>> its functionality correctly with less dependencies that's better.
>>>
>>
>> I would never advocate anything that wasn't properly and nicely testable.
>> I take your point that it seems simpler to test in this setup, but I don't
>> really believe it's simpler. Again, that's because the true dependencies
>> are hidden.
>>
>> In the case of manual dependency injection as I advocate, the solution is
>> protocols and empty implementations. I agree that empty implementations of
>> protocols can be annoying to maintain, but I find they solve the problem
>> very well in a clean and "dumb" manner.
>>
>> For instance, to test VC_A, assuming all the services were protocols, you
>> could create an instance solely to test its service A functionality like so:
>>
>> let testedVC = ViewController_A(serviceA: TestingServiceA(), serviceB:
>> DummyServiceB(),
>>                                                      serviceC:
>> DummyServiceC(), serviceD: DummyServiceD())
>>
>> https://gist.github.com/ianterrell/c86aa1ad64453a214966ac81b31f75e9
>>
>> I do think there is value in aiding with property injection for the cases
>>>> where we can't use initializer injection (we do often use storyboards at my
>>>> company, although we try to keep them small and unwieldy!), but I think
>>>> those cases are solved with a few clever extensions. See post-script if
>>>> you're curious.
>>>>
>>>
>>> Cleanse does support property injection
>>> <https://github.com/square/Cleanse#property-injection>. My examples in
>>> the readme only really demonstrate using it for the AppDelegate, but it
>>> also makes sense for storyboard injection. It is very explicit though (less
>>> magic than other DI frameworks). I also think there would be room for a
>>> cleanse extension that specifically deals with storyboards. (I kinda like
>>> what RxSwift did with RxCocoa).
>>>
>>
>> Yes, I saw it. I think I only meant that I do see manual property
>> injection with UIKit classes as sometimes cumbersome, but I believe there
>> are better solutions than frameworks like these.
>>
>> I don't believe I've misinterpreted your project or it's documentation. I
>> anticipate that we may simply believe in different solutions to the hard
>> problem or dependency management. In that case I leave this conversation
>> happily agreeing to disagree, just as I do with some of my other colleagues
>> here. :) But at least I hope I've well explained why I personally view
>> these solutions as detrimental.
>>
>> Thanks,
>> Ian
>>
>> _______________________________________________
>> swift-users mailing list
>> swift-users at swift.org
>> https://lists.swift.org/mailman/listinfo/swift-users
>>
> --
> Hello majk!
> das
> d
> asd
> as
> dasd
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-users/attachments/20160617/82027a2c/attachment.html>