[swift-users] Feedback for Dependency Injection Framework?

[Ian Terrell]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-users/attachments/20160616/4d5268b8/attachment.html>