google / dagger

A fast dependency injector for Android and Java.
https://dagger.dev
Apache License 2.0
17.44k stars 2.02k forks source link

Kotlin+Dagger best practices/documentation/pain points #900

Open ronshapiro opened 7 years ago

ronshapiro commented 7 years ago

Opening this as a tracking bug for all kotlin related documentation that we should be add/best practices that we should call out to make using Dagger w/ Kotlin easier.

One example: How to achieve the effect of static @Provides in Kotlin.

Feel free to comment new ideas, but don't make "me too" or "i agree with XYZ" comments.

ZacSweers commented 7 years ago

Edit: This resolved in Dagger 2.25+ by https://github.com/google/dagger/commit/646e0336cdbe454baa5fe73c0af11f211a92deeb


If you have injected properties (as "fields"), qualifiers must have field: designation.

Good

@Inject @field:ForApi lateinit var gson: Gson

Bad

@Inject @ForApi lateinit var gson: Gson // @ForApi is ignored!

Forgetting this can lead to subtle bugs if an unqualified instance of that type also exists on the graph at that scope, as that's the one that will end up being injected. This would make a great lint as well

ZacSweers commented 7 years ago

Edit: Objects are handled in Dagger 2.25+. Companion objects are handled in Dagger 2.26+.


Static provides can be achieved via @JvmStatic. There are two scenarios I see this come up:

top-level objects

@Module
object DataModule {
  @JvmStatic @Provides fun provideDiskCache() = DiskCache()
}

If you have an existing class module, things get a bit weirder

@Module
abstract class DataModule {
  @Binds abstract fun provideCache(diskCache: DiskCache): Cache

  @Module
  companion object {
    @JvmStatic @Provides fun provideDiskCache() = DiskCache()
  }
}

The way this works is as follows:

I sent Ron a braindump once of how dagger could better leverage this, i.e. no requirement for JvmStatic and just call through to the generated Companion class, but I think that's out of the scope of this issue :). I've been meaning to write up a more concrete proposal for how that would work.

ZacSweers commented 7 years ago

Another gotcha is inline method bodies. Dagger relies heavily on return types to connect the dots. In kotlin, specifying the return type is optional sometimes when you can inline the method body. This can lead to confusing behavior if you're counting on implicit types, especially since the IDE will often try to coerce you into quickfixing them away

That is, you could write in one of four ways

// Option 1
@Provides fun provideDiskCache() = DiskCache()

// Option 2
@Provides fun provideDiskCache(): DiskCache = DiskCache()

// Option 3
@Provides fun provideDiskCache(): DiskCache {
  return DiskCache()
}

// Option 4
@Provides fun provideDiskCache(): Cache {
  return DiskCache()
}

// Option 5
@Provides fun provideDiskCache(): Cache = DiskCache()

The first function is valid, but DiskCache is what's on the DI graph there because that's the inferred return type. The first three functions are functionally identical.

The fourth function is also valid, but now Cache is what's on the graph and DiskCache is just an implementation detail. The fifth function is functionally identical to the fourth.

The IDE will try to suggest inlining the fourth one. You can do so, but be mindful of potentially changing return types if you also drop the explicit return type.

JakeWharton commented 7 years ago

An expression body doesn't preclude the use of a return type.

On Mon, Oct 16, 2017, 5:10 PM Zac Sweers notifications@github.com wrote:

Another gotcha is inline method bodies. Dagger relies heavily on return types to connect the dots. In kotlin, specifying the return type is optional sometimes when you can inline the method body. This can lead to confusing behavior if you're counting on implicit types, especially since the IDE will often try to coerce you into quickfixing them away

That is, you could take write in one of two ways

@Provides fun provideDiskCache() = DiskCache()

@Provides fun provideDiskCache(): Cache { return DiskCache() }

The first function is valid, but DiskCache is what's on the DI graph there because that's the inferred return type.

The second function is also valid, but now Cache is what's on the graph and DiskCache is just an implementation detail.

The IDE will try to suggest inlining the second one. You can do so, but be mindful of potentially changing return types.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/google/dagger/issues/900#issuecomment-337044489, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEEd-hmcPrT3JVf03HxMXNZWvOSOJuks5ss8ZQgaJpZM4P5yKC .

ZacSweers commented 7 years ago

Good point, tweaked the wording to make it clear that it's only if you drop the return type and added more examples

tasomaniac commented 7 years ago

@JvmSuppressWildcards is incredibly useful when you are injecting classes with generics. Can be handy when using multimap injection.

jhowens89 commented 7 years ago

What a great thread! I was fighting this fight this last weekend. Here is the syntax for injection that's both qualified and nullable:

@field:[Inject ChildProvider] @JvmField var coordinatorProvider: CoordinatorProvider? = null

janheinrichmerker commented 7 years ago

@tasomaniac #807 has also cost me quite some time to debug. Doesn't look nice but at least it works:

@Inject lateinit var foo: Set<@JvmSuppressWildcards Foo>

(taken from https://stackoverflow.com/a/43149382/2037482)

charlesdurham commented 6 years ago

As an add-on from @heinrichreimer, the same thing is required when injecting functions that have parameters:

... @Inject constructor(val functionalThing: @JvmSuppressWildcards(true) (String) -> String)
InvisibleGit commented 6 years ago

With Kotlin 1.2 we can use array literals in annotations, ditching arrayOf() call in them

@Component(modules = [LibraryModule::class, ServicesModule::class])

guelo commented 6 years ago

As far as I can tell Dagger is unable to inject Kotlin functions. This fails

@Module
class Module() {
  @Provides fun adder(): (Int, Int) -> Int = {x, y -> x + y}
}

class SomeClass {
  @Inject lateinit var adder:  (Int, Int) -> Int

  fun init() {
    component.inject(this)
    println("${adder(1, 2)} = 3")
  }
}

error: kotlin.jvm.functions.Function2<? super java.lang.Integer,? super java.lang.Integer,java.lang.Integer> cannot be provided without an @Provides- or @Produces-annotated method.

Theoretically you could provide a kotlin.jvm.functions.Function2 but I failed to make it work. Even with this

@Provides
fun adder(): kotlin.jvm.functions.Function2<Integer, Integer, Integer> {
  return object : kotlin.jvm.functions.Function2<Integer, Integer, Integer> {
    override fun invoke(p1: Integer, p2: Integer): Integer {
      return Integer(p1.toInt() + p2.toInt())
    }
  }
}

it still says a Function2 cannot be provided.

JakeWharton commented 6 years ago

You need to add @JvmSuppressWildcards at the injection site to ensure the signature matches.

On Mon, Jan 22, 2018 at 7:55 PM Miguel Vargas notifications@github.com wrote:

As far as I can tell Dagger is unable to inject Kotlin functions. This fails

@Module class Module() { @Provides fun adder(): (Int, Int) -> Int = {x, y -> x + y} }

class SomeClass { @Inject lateinit var adder: (Int, Int) -> Int

fun init() { component.inject(this) println("${adder(1, 2)} = 3") } }

error: kotlin.jvm.functions.Function2<? super java.lang.Integer,? super java.lang.Integer,java.lang.Integer> cannot be provided without an @Provides- or @Produces-annotated method.

Theoretically you could provide a kotlin.jvm.functions.Function2 but I failed to make it work. Even with this

@Providesfun adder(): kotlin.jvm.functions.Function2<Integer, Integer, Integer> { return object : kotlin.jvm.functions.Function2<Integer, Integer, Integer> { override fun invoke(p1: Integer, p2: Integer): Integer { return Integer(p1.toInt() + p2.toInt()) } } }

it still says a Function2 cannot be provided.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/google/dagger/issues/900#issuecomment-359629878, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEEWWIjIzLpOQBN_IbTjP239JfGh-Vks5tNS4LgaJpZM4P5yKC .

gildor commented 6 years ago

I understand that this issue about "best practices", but on Reddit AMA @ronshapiro mentioned that this issue the place to collect kotlin-specific features. There is a couple possible kotlin-specific features that require work with Kotlin Metadata, but would be very useful for Kotlin projects and make dagger usage even more pleasant

  1. Support typealias as an alternative for @Named and custom @Qualifier annotations. It's completely compiler feature, but you can get typealias name from @Metadata annotation, so can use it to detect different qualifiers of the same class
  2. Support nullability as an alternative for Optional value. But I see some drawbacks: you cannot distinguish between non-initilized nullable and inilized one, but it's still can be a usefult and efficient replacement for Optional dependencies.
  3. Native support of lambda injection that doesn't require to use @JvmSuppressWildcards. This proposal can be extended to other generics, like for Lists and so on, but it not always required behavior, but in the case of lambdas I don't see any good reason do not consider any lambda (kotlin.jvm.functions.* interface) as generic without a wildcard. It will make lambda injection much less wordy. Also, works pretty well with typealias as qualifier.
ronshapiro commented 6 years ago

It would probably be good to make sure that no binding method used default parameters. It wouldn't make sense, and it definitely would be misleading.

tobeylin commented 6 years ago

The method injection works well on Kotlin setter.

@set: Inject
lateinit var booleanProvider: Provider<Boolean>

If you need the qualifier, must write the custom setter.

var booleanProvider: Provider<Boolean>? = null
        @Inject set(@MyQualifier value){
            field = value
}
sophiataskova commented 6 years ago

@hzsweers re: your comment on static provides methods in Kotlin, does the Kotlin implementation offer the same performance wins as "true" Java static methods? Does it provide any performance wins? What's your main reason for making your modules this way?

Static provides can be achieved via @JvmStatic. There are two scenarios I see this come up:

top-level objects

@Module object DataModule { @JvmStatic @Provides fun provideDiskCache() = DiskCache() } If you have an existing class module, things get a bit weirder

@Module abstract class DataModule { @Binds abstract fun provideCache(diskCache: DiskCache): Cache

@Module companion object { @JvmStatic @Provides fun provideDiskCache() = DiskCache() } } The way this works is as follows:

the companion object must also be annotated as @Module under the hood, the kotlin compiler will duplicate those static provides methods into the DataModule class. Dagger will see those and treat them like regular static fields. Dagger will also see them in the companion object, but that "module" will get code gen from dagger but be marked as "unused". The IDE will mark this as such, as the provideDiskCache method will be marked as unused. You can tell IntelliJ to ignore this for annotations annotated with @Provides via quickfix I sent Ron a braindump once of how dagger could better leverage this, i.e. no requirement for JvmStatic and just call through to the generated Companion class, but I think that's out of the scope of this issue :). I've been meaning to write up a more concrete proposal for how that would work.

ZacSweers commented 6 years ago

static provides allow you to make your modules truly stateless and allows you to simplify plumbing (no instance wiring required). The biggest win there is basically architecture. The "performance" wins of invokestatic always seemed kind of a reach. 25% of something measured in nanoseconds on a non-hotpath seems a nice-to-have rather than a strong motivating factor.

ronshapiro commented 6 years ago

The invokestatic might not be the true benefit - not needing the module instance allows all static provides to get horizontally merged, eliminating the class loading of all of the module classes.

sophiataskova commented 6 years ago

@ronshapiro same question -- does that same behavior occur with Kotlin's "static" implementations? In the case of top-level Kotlin object, an instance of that class still gets created; in the case of companion object, the "static" methods are only accessible through an instance of the companion object. (I am learning both Kotlin and Dagger right now so please correct me if anything I said was false)

arekolek commented 6 years ago

@sophiataskova See the discussion I had with Jeff Bowman on this topic, maybe you'll find it useful.

tasomaniac commented 6 years ago

My 2 cents:

If somebody put a gun to your head to do 100% Kotlin, sure this is a solution. But I really prefer Java Modules to this. It is totally fine for me to 1 or 2 Java abstract classes in my feature. No inner classes, single flat file of provisions. Much simpler 🎉

sophiataskova commented 6 years ago

Should I take it that Kotlin does not offer any of the advantages of static @Provides that you get in Java?

Ron says that not needing the module instance allows all static provides to get horizontally merged, eliminating the class loading of all of the module classes but both Kotlin implementations offered here seem to still involve class loading for each module. Again, I kinda hope I'm wrong, and can make my app faster without converting to Java.

JakeWharton commented 6 years ago

First of all, this speed benefit will be extraordinarily minor. If you are looking to make your app faster then use a profiler. You will find 100 easier optimization targets.

Beyond that, don't use companion object for modules. Use object. In that case the instance will be unused and its initialization code will be removed by R8 and the methods will be truly static and can also be inlined just like Java.

tasomaniac commented 6 years ago

It is unfortunately not possible to have abstract methods in object. You would need 2 modules. I try not to mix abstract and static provisions anyways. But if you need to, object is not a solution. This JvmStatic trick allows you to have them in 1 module.

ronshapiro commented 6 years ago

Recording a thought from Droidcon: someone mentioned that @Inject on constructors is awkward in Kotlin because the constructor is often implicit via the properties list. Using the constructor keyword is not idiomatic.

Could we support an annotation on the class, detect that it's a kotlin class, and treat the sole constructor as having @Inject? Or could we have an annotation in modules that adds a binding to a type's sole constructor as if it had @Inject (which may also make adding bindings for code that you don't own easier).

JakeWharton commented 6 years ago

I think you should ignore that person!

The last point isn't tied to any language and should probably be considered separately.

On Wed, Aug 29, 2018, 7:22 PM Ron Shapiro notifications@github.com wrote:

Recording a thought from Droidcon: someone mentioned that @Inject on constructors is awkward in Kotlin because the constructor is often implicit via the properties list. Using the constructor keyword is not idiomatic.

Could we support an annotation on the class, detect that it's a kotlin class, and treat the sole constructor as having @Inject? Or could we have an annotation in modules that adds a binding to a type's sole constructor as if it had @Inject (which may also make adding bindings for code that you don't own easier).

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/google/dagger/issues/900#issuecomment-417138971, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEEfuOXjQQGhJ7dwT0Yf1rX8nHiXAQks5uVyIbgaJpZM4P5yKC .

fat-fellow commented 6 years ago

@ronshapiro It would be nice to have at least second one. But both sounds nice. It is very common case to have the only one constructor in kotlin code. We don't use bindings because at least it looks awkward (imposes requirement to have @inject in a constructor we need to declare first only for that purpose).

class Rectangle @Inject constructor(
        @Named("length") private val length: Int,
        private val width: Int
    )

vs

class Rectangle (
        @Named("length") private val length: Int,
        private val width: Int
    )

And furthermore there are too much work to change @Provides to @Binds, when you need to find all the classes and change them for using inject. It would be much better if all I need to do is to get done the work only by changing modules mostly (if I understand correctly). In general we very rarely use @Named or any other qualifiers, so the samples could look even clearer.

tasomaniac commented 6 years ago

It is easier to have only 1 constructor in Kotlin since they have default value support. Dagger does not understand that and that's why often times I find myself having secondary constructor with Inject annotation to provide defaults.

If Dagger is going to get a Kotlin specific features, I think understanding the default values provided would be my favorite.

cgruber commented 6 years ago

The problem with @Inject on the type is that it isn't JSR-330. I like the idea, but I think it breaks the language. (Not that Dagger couldn't stray, but it should be considered carefully).

An alternative would be to just accept single-constructors as implicitly @Inject, which I think @warabei14 refers to above, but that walks back a rather significant decision to not support implicit constructors (mostly to avoid stuff like injecting a newly constructed empty String because it has a default constructor). There are subtleties here, and I like the possible feature set for Kotlin, but we should make sure we represent those earlier concerns properly in the discussion, so we don't trample older design decisions that may still be meaningful.

On Thu, 30 Aug 2018 at 03:27 Said Tahsin Dane notifications@github.com wrote:

It is easier to have only 1 constructor in Kotlin since they have default value support. Dagger does not understand that and that's why often times I find myself having secondary constructor with Inject annotation to provide defaults.

If Dagger is going to get a Kotlin specific features, I think understanding the default values provided would be my favorite.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/google/dagger/issues/900#issuecomment-417271069, or mute the thread https://github.com/notifications/unsubscribe-auth/AAUN4s5wC3pUS5n7Ar6FKX-cJV-N2Aegks5uV74OgaJpZM4P5yKC .

ronshapiro commented 6 years ago

I don't think @Inject on a class would even work. But we could add an annotation on the type declaration (not saying we should, just ideating)

On Fri, Aug 31, 2018, 2:34 PM Christian Edward Gruber < notifications@github.com> wrote:

The problem with @Inject on the type is that it isn't JSR-330. I like the idea, but I think it breaks the language. (Not that Dagger couldn't stray, but it should be considered carefully).

An alternative would be to just accept single-constructors as implicitly @Inject, which I think @warabei14 refers to above, but that walks back a rather significant decision to not support implicit constructors (mostly to avoid stuff like injecting a newly constructed empty String because it has a default constructor). There are subtleties here, and I like the possible feature set for Kotlin, but we should make sure we represent those earlier concerns properly in the discussion, so we don't trample older design decisions that may still be meaningful.

On Thu, 30 Aug 2018 at 03:27 Said Tahsin Dane notifications@github.com wrote:

It is easier to have only 1 constructor in Kotlin since they have default value support. Dagger does not understand that and that's why often times I find myself having secondary constructor with Inject annotation to provide defaults.

If Dagger is going to get a Kotlin specific features, I think understanding the default values provided would be my favorite.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/google/dagger/issues/900#issuecomment-417271069, or mute the thread < https://github.com/notifications/unsubscribe-auth/AAUN4s5wC3pUS5n7Ar6FKX-cJV-N2Aegks5uV74OgaJpZM4P5yKC

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/google/dagger/issues/900#issuecomment-417753950, or mute the thread https://github.com/notifications/unsubscribe-auth/AAwY3VZA_eW2Dm7rbU9hVvM79hVhM5Z2ks5uWYHNgaJpZM4P5yKC .

ZacSweers commented 6 years ago

You could have a separate artifact of kotlin-specific annotations that, when used, require Kotlin @Metadata annotations to be present too. In the case of @Inject on a class, kotlin metadata annotations can tell you what the primary constructor is of a given class, and you'd then just want to wire that up in Dagger as a fake @Inject constructor. That'd allow you to have something like @KotlinInject that doesn't tread on the existing JSR330 annotations. That would also allow you to be a bit more intentional about Kotlin support in dagger as well, like expectations of a primary constructor or expectation of metadata annotations.

JakeWharton commented 6 years ago

I'm still not convinced we need to invent both a problem and a solution. Is @Inject constructor really that bad? Why would someone describe a feature of the language as unidiomatic unless they were just trying to code golf character count?

On Fri, Aug 31, 2018, 4:28 PM Zac Sweers notifications@github.com wrote:

You could have a separate artifact of kotlin-specific annotations that, when used, require Kotlin @Metadata annotations to be present too. In the case of @Inject on a class, kotlin metadata annotations can tell you what the primary constructor is of a given class, and you'd then just want to wire that up in Dagger as a fake @Inject constructor. That'd allow you to have something like @KotlinInject that doesn't tread on the existing JSR330 annotations.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/google/dagger/issues/900#issuecomment-417780573, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEEYXqapw5gdI6XU8QIBVjFKCDV_VOks5uWZxbgaJpZM4P5yKC .

cgruber commented 6 years ago

I'm with Jake here. I don't accept that it isn't idiomatic kotlin either. If you need to see the constructor, this is how you do it. I've done this several times and it reads just fine to me.

On Fri, Aug 31, 2018, 5:03 PM Jake Wharton notifications@github.com wrote:

I'm still not convinced we need to invent both a problem and a solution. Is @Inject constructor really that bad? Why would someone describe a feature of the language as unidiomatic unless they were just trying to code golf character count?

On Fri, Aug 31, 2018, 4:28 PM Zac Sweers notifications@github.com wrote:

You could have a separate artifact of kotlin-specific annotations that, when used, require Kotlin @Metadata annotations to be present too. In the case of @Inject on a class, kotlin metadata annotations can tell you what the primary constructor is of a given class, and you'd then just want to wire that up in Dagger as a fake @Inject constructor. That'd allow you to have something like @KotlinInject that doesn't tread on the existing JSR330 annotations.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/google/dagger/issues/900#issuecomment-417780573, or mute the thread < https://github.com/notifications/unsubscribe-auth/AAEEEYXqapw5gdI6XU8QIBVjFKCDV_VOks5uWZxbgaJpZM4P5yKC

.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/google/dagger/issues/900#issuecomment-417817187, or mute the thread https://github.com/notifications/unsubscribe-auth/AAUN4r-dhtlWjZvscpAUEHn43sRvmRG4ks5uWc7NgaJpZM4P5yKC .

tasomaniac commented 6 years ago

Here is how you would add Dagger's annotation processor options with Kotlin's kapt. It looks pretty nice actually.

kapt {
  arguments {
    arg('dagger.formatGeneratedSource', 'disabled')
  }
}

You can use ☝️ for all the options mentioned here: https://google.github.io/dagger/compiler-options.html

ultraon commented 5 years ago

I created interesting the config for kapt with different optimization options in this way:

kapt {
    useBuildCache = false
    // If you have some issues with kapt to get more meaningful message in terminal set to "true" "mapDiagnosticLocations" and "correctErrorTypes" below
    mapDiagnosticLocations = false
    correctErrorTypes = false

    javacOptions {
        // Dagger2 APT optimizations, see https://google.github.io/dagger/compiler-options
        option('-Adagger.fastInit=enabled')
        if (isCiBuild) {
            option('-Adagger.moduleBindingValidation=WARNING')
            // Increase the max count of errors from annotation processors, default is 100,
            // see https://kotlinlang.org/docs/reference/kapt.html#java-compiler-options
            option("-Xmaxerrs", 500)
        } else {
            option('-Adagger.formatGeneratedSource=disabled')
            option('-Adagger.gradle.incremental')
            option('-Adagger.moduleBindingValidation=ERROR')
        }
    }
}
tasomaniac commented 5 years ago

You should consider enabling the build cache with kapt to improve the build speed. This is a huge speed up especially in highly modularized apps. We have been using it with Dagger for quite some time with no problem.

kapt {
    useBuildCache = true
}
matejdro commented 5 years ago

Does option('-Adagger.gradle.incremental') even make any difference considering kapt is not incremental?

vRallev commented 5 years ago

I found another little workaround. If you have an internal function for multi bindings, then Dagger can't find the method, because the Kotlin compiler mangles the name.

@Module
abstract class MyModule {
  // methods

  @Module
  companion object {
    @Provides @IntoSet @JvmStatic
    internal fun contributeInternalObject(object: InternalClass): InterfaceClass = object
  }
}

This will fail with error: cannot find symbol.

The trick is to use @PublishedApi. That tells the Kotlin compiler to skip the name mangling and makes Dagger happy.

@Provides @IntoSet @JvmStatic @PublishedApi
internal fun contributeInternalObject(object: InternalClass): InterfaceClass = object
prasanthmurugan commented 5 years ago

Hi, i am learning dagger dependency injection. But got stuck with the below error for 2 days. I have created StackOverflow post regarding this error.

I created a ViewModel with contributeAndroidInjection annotation in ViewModelModule but the view model contructor has an integer value. So i am providing an integer value at runtime from my activity intent through ItemDetailActivityModule class. Problem is dagger throwing me an error "integer already exist in the scope" but not able to inject in the ViewModel contructor.

Any help or suggestion could be great !!

StackOverflow post link

ZacSweers commented 5 years ago

@prasanthmurugan that doesn't have anything to do with Kotlin. Let's keep this issue on topic

@matejdro kapt supports incremental apt as of kotlin 1.3.30 (though theres a Gradle flag you need to enable)

ZacSweers commented 5 years ago

@ronshapiro @gildor Another thought on default parameters: this could allow for otherwise "missing" dependencies when composing modules. Consider this:

class HttpModule {
  @Provides
  fun provideHttpClient(cache: Cache? = Cache.inMemoryCache()): HttpClient {
    return HttpClient(cache)
  }
}

@Module(includes = [HttpModule::class]
class TwitterModule {
  @Provides
  fun provideTwitterClient(httpClient: HttpClient): TwitterClient {
    return TwitterClient(httpClient)
  }
}

//
// TwitterModule could be used with any of the below AppModule variants
//

// Cache is defined and not null, HttpClient will end up with RealCache
@Module(includes = [TwitterModule::class]
class ProductionAppModule {
  @Provides fun provideCache(): Cache {
    return RealCache()
  }
}

// No cache is defined, HttpClient will end up with default inMemoryCache()
@Module(includes = [TwitterModule::class]
class IntegrationTestAppModule {

}

// Cache is defined but null, HttpClient will end up with no cache
@Module(includes = [TwitterModule::class]
class ConstrainedEnvironmentAppModule {
  @Provides fun provideCache(): Cache? = null
}

You can model absence and nullability in Kotlin at least. I'm not familiar enough with dagger to say off the top of my head if it differentiates between null and absent, but I'd assume it does with Optional. Then the above just boils down to invoking the defaults method and indicating if the cache parameter is present, rather than modeling with something like Optional. Invoking the synthetic defaults method would require reflection with today's build tooling, but could be achieved with bytecode generation once gradle and kapt properly support Filer-generated bytecode. The above approach is definitely advanced usage, but I think a really powerful affordance of the language. This can also be done with constructors for constructor injection.

tasomaniac commented 5 years ago

In a Dagger component, an instance is only ever available once. You don't have 2 different ways of injecting the same instance. If I'm not wrong, Kotlin default value support would introduce that. That means, it would not be clear which instance is injected in a given graph.

gmale commented 4 years ago

Should these best practices be summarized into a document, perhaps as part of the initiative to improve Dagger on Android? That might allow this issue to be closed, rather than remain open indefinitely.

ZacSweers commented 4 years ago

The purpose of this issue is to be a living doc, I don’t think moving it into a frozen documentation that the community can’t contribute to is good

kvithayathil commented 4 years ago

Definitely like the idea of keeping this issue open for all contributors. It would be great to transpose of these best practises somewhere indexable/searchable

dmitry-ryadnenko-izettle commented 4 years ago

Can we expect at some point to be able to add @Inject annotations to top-level factory functions like we can do with class constructors? Example.

@Inject
fun makeMyDependency(): MyDependency = object : MyDependency {

}

So, in Dagger, you can tell Dagger how to resolve a dependency by dropping an @Inject annotation on the class constructor.

class MyDependencyImpl @Inject constructor() : MyDependency {

}

What I'm missing really badly is a way to do the same thing but with top-level factory functions.

@Inject
fun makeMyDependency(): MyDependency = object : MyDependency {

}

One of the most important scenarios where I need this is to be able to tell Dagger how to provide a function.

@Inject
fun makeMyFunction(): () -> Something = { /* implement function */ }

Functions doesn't have constructors, so when using a factory function like the one above there is no way to benefit from boilerplate generation that we have with @Inject in class constructors. So when providing functions we always have to write boilerplate like this.

@Module
class MyFunctionModule {
    @Binds
    fun provideMyFunction(
            dependency1: Dependency1,
            ...
            dependencyN: DependencyN
    ): () -> Something = makeMyFunction(
            dependency1: Dependency1,
            ...
            dependencyN: DependencyN
    )
}

We can of course move implementation of makeMyFunction() to MyFunctionModule but then we introduce strong coupling of our DI agnostic code with Dagger. makeMyFunction() implementation can hold lets say an important business logic and it would be nice to not not mix this function with DI details. Adding @Inject annotation to it would still be "mixing with DI details" but to a much lesser extent than surrounding it with a module class.

The added benefit of being able to add @Inject to top level functions is that instead of doing something like this.

class MyDependencyImpl @Inject constructor() : MyDependency {

}

@Module
interface MyDependencyModule {
    @Binds
    fun provideMyDependency(myDependency: MyDependencyImpl): MyDependency
}

We can now do it like this instead.

@Inject
fun makeMyDependency(): MyDependency = object : MyDependency {

}

So, there is no longer need to come up with a name for the implementation class and there is no longer need for a module with @Binds annotations.

Basically the code bellow is no longer needed.

@Module
interface MyDependencyModule {
    @Binds
    fun provideMyDependency(myDependencyImpl: MyDependencyImpl): MyDependency
}

And for us it's a pretty common scenario when we have to do something like this. I think it would be nice to be able to not write this boilerplate anymore.

JakeWharton commented 4 years ago

Those seem far more like module-less providers than injection points. I can argue semantics, but I actually don't think it's that useful of a distinction in practice.

I'm extremely skeptical of how well this would work in a general sense. As soon as you get into things like scoping annotations you need the ability to associate a function with the corresponding component–especially since the function could live entirely separately from the component definition.

tasomaniac commented 4 years ago

Isn't that the same with constructor injection. When putting @Inject annotation to a class constructor you don't really associate it with a component either. Scopes are done via putting a scope annotation at class level.

tbroyer commented 4 years ago

What you're looking for is something akin to Guice's @ProvidedBy, but that annotation goes on the type being injected, which I don't think is possible here; or like CDI's @Produces. In any case, that would be something extra to JSR 330, not @Inject; so you cannot achieve "DI agnostic code", at least in the sense that it could work with another JSR 330 implementation than Dagger.

Fwiw, the way Dagger works, if it finds that it needs to inject a type A, it will look into the component's modules whether there's a specific binding for it, and will fall back to A's @Inject-annotated constructor. It wouldn't be able to find your provider/factory method without somehow scanning the classpath (at runtime or compile-time, but still) for anything that would provide the expected type (before falling back to using its constructor). This is actually how CDI works by design, classpath scanning, autodiscovery, etc. the opposite of Dagger's mantra that things should be explicit (hey, it even requires you to annotate a zero-argument constructor with @Inject!)

You could probably have your own annotation and annotation processor (or Kotlin compiler plugin) to generate that module for you based on your annotated function (and possibly then a build plugin, like Hilt, for even fewer boilerplate). This is actually not much different from AutoFactory or AssistedInject, except specific to Kotlin. Or maybe when Dagger will have native support for factories (#1825) this would be somehow included?

tasomaniac commented 4 years ago

That definitely makes sense. Thanks @tbroyer for detailed explanation.

Btw, I feel like this can already be possible with Hilt by having @file:InstallIn and @file:Module annotations. I've not tried myself.