collect icon indicating copy to clipboard operation
collect copied to clipboard

Add Hilt to the project

Open grzesiek2010 opened this issue 2 years ago • 10 comments

Problem description

We use Dagger for dependency injection. Hilt is built on top of the Dagger and it is a recommended tool now and it is everywhere (documentation, tutorials, articles). The goal of Hilt is:

  • To simplify Dagger-related infrastructure for Android apps.
  • To create a standard set of components and scopes to ease setup, readability, and code sharing between apps.
  • To provide an easy way to provision different bindings to various build types, such as testing, debug, or release.

Some time ago I started a discussion on slack about it but we haven't done anything. Personally I'm a fan of this tool although I don't have hand-on experience with it, it feels like it makes life easier.

We can migrate from Dagger to Hilt (see https://dagger.dev/hilt/migration-guide.html) but It would require a lot of refactoring so for us maybe a better option would be to add Hilt to the project and support both gradually switching to Hilt.

What do you think @seadowg

grzesiek2010 avatar Dec 20 '21 12:12 grzesiek2010

I was just thinking about this! We should definitely have a proper discussion about this as part of our next release cycle and do some investigation, but I wanted to reply before you're away for the holidays. In short, I think we should look at and evaluate a few different options such as:

  • Keeping Dagger2
  • Moving to Hilt
  • Moving to a Kotlin DI tool like Kodein or Koin

Whatever we decide, I think we could make the switch in our non collect_app modules first (as they have independent setup) and then maybe leave Dagger2 in collect_app until the job becomes small enough or we feel we have the time to do a full conversion there.

Cards on the table: I'm not a fan of Dagger and whenever I've looked at Hilt I've not really felt like it improves on the problems I have (code generation, confusing compile time errors, need for dependency injection mechanism to be aware of components, trend towards tying yourself into Android specifics etc). It could be that that's changed over time though. I've found success in happiness previously by using less powerful but simpler (IMO) alternatives in Android such as (the now defunct) RoboGuice and Kodein DI. As I said at the beginning of this, I reckon we can probably compare different routes and come up with a good plan for the future.

seadowg avatar Dec 20 '21 16:12 seadowg

I have never used Koin or Kodein so I can't tell anything about my personal experiences with them vs Dagger. I can agree that dagger and its setup was complex and I think Hilt makes it a bit easier maybe not enough for you @seadowg but there is an improvement I think.

I would vote for sticking to dagger (I mean Hilt of course) because:

  • it's the most popular DI tool that was used in ~95% of courses I watched/read in last 2 years + it's from Google
  • it resolves dependencies at compile-time (but Koin and Kodein do that at run-time) that means app should work a bit faster (see https://github.com/Sloy/android-dependency-injection-performance) and it should help us to catch bugs better
  • it's based on dagger so everything looks similar and should be easier for us to use

I would rather choose between Hilt and Koin since Kodein seems like the worst option:

  • the smallest developer community
  • worse documentation and samples
  • the slowest runtime performance

but as I said my vote goes to Hilt.

grzesiek2010 avatar Jan 17 '22 16:01 grzesiek2010

I would vote for sticking to dagger (I mean Hilt of course) because

Can't disagree with any of your points here: Dagger is the standard for Android development, and it is pretty unique (within the JVM world) in providing performance advantages given it does most of it's work at compile time. I guess for me personally, I've never really felt like that is a worth trade-off for the things that really annoy me:

  • Coupled injection and container mechanism - to generate code the "component" needs to know what it's injecting into and vice versa
  • Slower compile times because of annotation processing
  • Generated code confusing refactoring tools

Again though, this is all personal between Dagger and I 😆. That battle has clearly been lost in the Android world.

I would rather choose between Hilt and Koin since Kodein seems like the worst option

That seems right - Koin seems to have become far more dominant in the last few years. They are very similar in the way setup etc works anyway.

I reckon as a test we should try replacing Dagger2 in the geo module with Hilt and Koin and then compare those examples with our Dagger2 one. We need to see how these things look in multimodule (not just a monolithic single module setting) and geo provides a good example of a module that uses DI as a way to receive dependencies from it's dependents (collect_app) and to manage its own internal dependencies. Sound like a good plan?

seadowg avatar Jan 18 '22 10:01 seadowg

I reckon as a test we should try replacing Dagger2 in the geo module with Hilt and Koin and then compare those examples with our Dagger2 one. We need to see how these things look in multimodule (not just a monolithic single module setting) and geo provides a good example of a module that uses DI as a way to receive dependencies from it's dependents (collect_app) and to manage its own internal dependencies. Sound like a good plan?

Yeah that sounds fine I can give it a try (Koin) why not. Please give me some time I will read/watch something about Koin so that I'm not completly new and we can start.

grzesiek2010 avatar Jan 18 '22 12:01 grzesiek2010

Yeah that sounds fine I can give it a try (Koin) why not.

What about I look at Koin and you look at Hilt? We could do draft PRs of each to compare. I've used Kodein a bunch in the past and Koin is pretty similar where as I haven't had a chance to look at Hilt properly.

seadowg avatar Jan 18 '22 13:01 seadowg

Ok a tangible example of Dagger being frustrating (IMO) that I just ran into: I'm unable to make GeoPointDialogFragment and internal class (encapsulate it within the geo module) because I need to have an inject method on GeoDependencyComponent to support injection into the class (and that interface has to be public). This is an example of the coupling actively getting in the way of doing other things (as well as being inelegant in my mind).

I believe this might be possible to work around Dagger's subcomponents, but that adds a lot more complexity than I feel like I should need to indulge in to get this working.

seadowg avatar Jan 19 '22 11:01 seadowg

As part of the work on #4893, I had to set up a Gradle module (mapbox) to be able to be provided dependencies from its dependent module (collect_app) without the dependent module having an explicit class reference (mapbox is only included if the developer configures a Mapbox download token locally). With Dagger we currently have the following (abstracted) setup for providing dependencies to modules from collect_app (apologies for the overloaded terminology of "module" here):

In the module

interface MyModuleComponentProvider {
    val myModuleDependencyComponent: MyModuleDependencyComponent
}

@Component(modules = [MyModuleDependencyModule::class])
@Singleton
interface MyModuleDependencyComponent {
...
}

@Module
open class MyModuleDependencyModule {
...
}

In a module class

class MyModuleActivity : ComponentActivity() {

    @Inject
    lateinit var someDep: SomeDep

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        (application as MyModuleDependencyComponent).getMyModuleDependencyComponent.inject(this)
    }
}

In collect_app

class Collect : Application, MyModuleComponentProvider  {

    override val myModuleDependencyComponent =
        DaggerOsmDroidDependencyComponent.builder()
        .myModuleDependencyModule(CollectMyModuleDependencyModule())
        .buidl()
    }
}

This setup runs into a few problems because of:

  1. We extend MyModuleDependencyModule in collect_app
  2. We build (using its interface's builder) a MyModuleDependencyComponent
  3. Our application class implements MyModuleDependencyComponentProvider

In the Mapbox case, collect_app won't be able to reference any of the classes in mapbox. We can work around 1 and 2 by using a class loader and some nasty reflection to call methods. With 3 we need to have a shared interface (in a third module).

Trying Hilt

This all felt like too much work, so I ended up investigating using Hilt to see if it let me provide dependencies without the explicit references (or implicit through reflection) that Dagger needs. And in fact, it does solve the problem. However, there are some other problems I want to lay out here (which will explain why I didn't end up using it there).

As a comparison to the example Dagger setup, here's the Hilt setup for MyModule:

In the module

// Nothing! No dependency setup needed to compile the module

In a module class

@AndroidEntryPoint
class MyModuleActivity : ComponentActivity() {

    @Inject
    lateinit var someDep: SomeDep
}

In collect_app

@HiltAndroidApp
class Collect : Application {

}

@Module
@InstallIn(SingletonComponent::class)
object HiltModule {
    ...
}

Simple! And, it works pretty intuitively - Hilt grabs dependencies (in classes with @AndroidEntryPoint) from our Application (which works because it's marked with @HiltAndroidApp and we've made a module available to be used with @InstallIn) without a lot of the boilerplate we needed for Dagger. Most importantly, collect_app doesn't need to know anything about our module to provide dependencies to it.

This started to fall apart when I played around with adding tests. There were two major problems:

1. Replacing dependencies in tests is a pain

Hilt provides ways to do this (@UninstallModules, @TestInstallIn, BindsValue etc) with as long as you're using its test rule and annotating your test with HiltAndroidTest (which gives me a bad feeling about having to move away from it ever), but you can only replace the whole module with another one. This would work OK for a project with dependencies split across many modules, but we've specifically avoided that as it creates a huge architectural though burden on how to split those modules and is only really useful when using the more advanced features of Dagger/Hilt (multiple scopes/components). This would mean that any test that wants to replace a dependency for another has to care about defining a module (or @BindsValue) fields for every dependency that is injected in the current classpath.

I tried replicating the trick we use in Dagger where we allow our application module to simply be extended in tests, overriding the provider method for the dependency we want to sub out, but this didn't work - I got an error telling me that overriding bindings is not allowed. Eventually I ended up with a solution like this:

@HiltAndroidApp
class RobolectricApplication : Application()

@Module
@InstallIn(SingletonComponent::class)
object TestModule {
    
    var someDep: SomeDep? = null

    @Provides
    fun proviesSomeDep(): SomeDep {
        return someDep ?: mock()
    }
}

That allows you to switch dependencies statically in and out of the module but would need clean up code. Adding to that, I just feel uncomfortable not being able to use the standard test tooling out of the box without making massive changes (which I don't think we want) to our dependency setup.

2. Hilt doesn't work with Fragments

Well it does unless you want tests for Fragments. Basically, Hilt doesn't work with FragmentScenario because the Activity that uses under the hood is not an @AndroidEntryPoint. There's some custom code for resolving this floating around (and even referenced from the official Android Hilt docs), but you still end up testing a Fragment using an ActityScenario whihc is not ideal and feels like a step backwards. There are two solutions to this (discounting the Android team fixing this by making FragmentScenario more versatile/configurable:

  1. Stop using a DI framework with Fragments and use FragmentFactory and constructors instead
  2. Use Hilt's @EntryPoint feature in Fragments (basically Dagger light for things that @AndroidEntryPoint doesn't support)

To be honest, I'm a fan of 1, but I'm not confident enough that it's a direction that will work for everything (we've only used FragmentFactory in a couple of places) to say that it solves this.

Conclusion

For me, I'm not sure that we actually get that much benefit from moving to Hilt. It does remove some boilerplate, but I feel like we'd be working around it unless we wanted to embrace a heavier Dagger/Hilt setup (split modules) which I'm still against. I'm going to look into Koin as well, but I know it's going to be hard to justify making a change to that due to performance concerns. For the moment, it's feeling to me like Dagger might still be the best fit. That said, it might be that the problems I've run into disappear as Hilt matures.

seadowg avatar Jun 14 '22 14:06 seadowg

To follow up, here's the same example using Koin:

In the module

// Again, nothing! No dependency setup needed to compile the module

In a module class

class MyModuleActivity : ComponentActivity() {

    private val someDep: SomeDep by inject()
}

In collect_app

class Collect : Application {

    private val module = module {
        factory<SomeDep> { SomeDepImpl() }
    }

    @Override
    public void onCreate() {
        super.onCreate();

        startKoin {
            androidContext(collect)
            modules(module)
        }
    }
}

There's some lovely under the hood reified magic making that inject property work - pretty much every Kotlin focused DI framework ends up doing something like that. I imagine signature is something like inline fun <reified T> Activity.inject(): InjectDelegate<T>. That aside, the setup is pretty simple, and I like that, like Hilt, we don't end up needing any boilerplate in the Activity's onCreate.

One thing I definitely prefer (as opposed to Hilt), is that the dependency "module" is just data and not a static entity. This makes switching it out in tests much easier:

class RobolectricApplication : Application() {
    fun overrideModule(module: Module) {
        stopKoin()
        startKoin {
            androidLogger()
            androidContext(this@RobolectricApplication)
            modules(module)
        }
    }
}

With some experimentation, I also found that you can also layer modules:

Test application

class RobolectricApplication : Application() {
    fun overrideModule(otherModule: Module) {
        stopKoin()
        startKoin {
            androidLogger()
            androidContext(this@RobolectricApplication)
            modules(baseModule, otherModule)
        }
    }
}

If baseModule and otherModule contain a binding of the same class, otherModule will override the binding in baseModule. This makes it simple to override a single or multiple dependencies in a test without needing to define all the dependencies. It's definitely not perfect though. Here are a few things that bother me:

startKoin/stopKoin

Koin holds dependencies at a process singleton level (which is why we have those suspicious stopKoin/startKoin paired calls). This isn't the end of the world, but it feels a little off to me. I tried to use it in a setup where I could invert the relationship and have Koin hosted inside the Application object, but that basically forced me to write my own version of inject. Interestingly, this is how a rival framework Kodein works. From their own examples:

class MyApplication : Application(), KodeinAware {
    override val kodein = Kodein.lazy {
        import(businessModule)
        import(androidModule(this@MyApplication))

        bind<Controller>()
            with scoped(ActivityRetainedScope).singleton {
                Controller(instance())
            }
    }
}

class MyActivity : Activity(), KodeinAware {
    override val kodein by closestKodein()

    // retained between activity restarts
    val ctrl: Controller by instance()
}

Kodein definitely feels nicer to me on the surface, but it now seems to be trying to be a full application framework (like Spring for instance) and really doesn't have that much serious adoption. I'd be tempted to write my own leaner DI framework as opposed to using it if I'm being honest.

Performance

Not something I noticed. But we know there will be some kind of performance hit on grabbing object out of a big map vs using generated methods (which is how Dagger and therefore Hilt works). That said, how big that hit is feels pretty unknown to me. As far as I've seen, there's only one performance writeup that's out there, and it uses a pretty deeply nested dependency graph (see the example Koin module) whereas we have a comparatively flat structure (declared dependencies don't often rely on other declared dependencies in our modules).

Documentation

Compared to Dagger/Hilt, the docs are very sparse. The nice thing about Dagger is that it's a very sharp tool which people go really deep on and it's been adopted by Google as the official DI tool for Android. Those two facts combine for a good combination of basic docs and SO posts.

Conclusion

Thoughts on Koin? Seems pretty nice. Should we switch to it? I don't know.

I think Dagger is mostly doing the job we need it to (other than in the one special case of the mapbox module), but I do think it's important for us to keep in mind that we mostly use it as a "service locator" rather than a dependency injection container (obligatory Martin Fowler post). For some, that's a dirty word, but I tend to think that if you really want to be inversion of control because you're writing tests and letting shape your architecture, then using constructors is pretty much all you need to be doing. The only time we need a "tool" or a "framework" is when we're dealing with objects that are constructed outside our control (like an Activity or Service), and in those cases we're not using dependency injection really - we're just grabbing dependencies from a shared singleton and slamming them into fields using some annotation magic. It does feel like Dagger is a little over the top for that (killing a cockroach with an atom bomb style), maybe that's OK because it hopefully means that the majority of Android devs would be able to understand our dependency setup faster than they would if we were using Koin or something custom. As Martin Fowler concludes in the attached article:

The choice between Service Locator and Dependency Injection is less important than the principle of separating service configuration from the use of services within an application.

I'd be interested to know if anyone really likes the look of Hilt or Koin based on my findings. I'd be happy to switch to Koin, but I'd also be happy to stick with Dagger and keep a keen eye on whether Hilt develops into something that doesn't have the same blockers it does today (that's probably something we can file issues or berate people at Andorid Dev Summit about).

seadowg avatar Jun 16 '22 14:06 seadowg

I still kind of like Hilt even with the drawback you described. However my feelings are not strong enough to force switching to that tool. We can keep this issue or close and get back to it in the future.

I'm only thinking about the issue we have with injecting objects into AudioRecorderFactory https://console.firebase.google.com/u/1/project/api-project-322300403941/crashlytics/app/android:org.odk.collect.android/issues/39d9044314abc57e2885c3e308514179?time=last-ninety-days&types=crash&versions=v2022.2.3%20(4444)&sessionEventKey=62E7D54F032F0001253D01727A8DFA7B_1705345319511332820

Maybe it would make sense to try hilt in that module to see if it fixes that mysterious issue...

grzesiek2010 avatar Aug 01 '22 16:08 grzesiek2010

I'm only thinking about the issue we have with injecting objects into AudioRecorderFactory https://console.firebase.google.com/u/1/project/api-project-322300403941/crashlytics/app/android:org.odk.collect.android/issues/39d9044314abc57e2885c3e308514179?time=last-ninety-days&types=crash&versions=v2022.2.3%20(4444)&sessionEventKey=62E7D54F032F0001253D01727A8DFA7B_1705345319511332820

We could look at using the ObjectProvider system we introduced for Mapbox? I feel less happy with having Hilt and Dagger in the app given the lines are a little blurred between them. On the other hand, maybe that's a good argument for having Hilt in some places, and Dagger in others. We could also put Hilt in place for Mapbox - it wouldn't run into the test problems there as that module doesn't have any.

seadowg avatar Aug 01 '22 19:08 seadowg

@grzesiek2010 thoughts on the last comment?

seadowg avatar Oct 24 '22 16:10 seadowg

@grzesiek2010 thoughts on the last comment?

Yes, sorry I didn't respond but actually yesterday and today I was playing with hilt (mixing it with plain dagger to see if it doesn't cause any big problems and to understand hilt more). I'm not like totally in favor of one of those options but I'm leaning towards trying hilt at least in that one module. If you are not against I can try adding hilt to the audiorecorder module to check if it work well and then we can release it to see if it fix that weird bug we had with injecting things in that module.

grzesiek2010 avatar Oct 25 '22 14:10 grzesiek2010

If you are not against I can try adding hilt to the audiorecorder module to check if it work well and then we can release it to see if it fix that weird bug we had with injecting things in that module.

Yeah that sounds like a good resolution to this. The more I play around with the newer FragmentFactory (especially when combined with the Navigation Component), the more I can see Hilt fitting in well in the future. Let's give it a try in that one module and see if we can come up with a setup we like.

seadowg avatar Oct 25 '22 16:10 seadowg

Closing this as I think we have a nice resolution around what we're doing next.

seadowg avatar Oct 25 '22 16:10 seadowg