amzn / kotlin-inject-anvil

Extensions for the kotlin-inject dependency injection framework
Apache License 2.0
182 stars 2 forks source link

Enforce scopes on all `@Contribute*` annotations #36

Open vRallev opened 6 days ago

vRallev commented 6 days ago

kotlin-inject initially didn't support arguments for scopes https://github.com/evant/kotlin-inject/issues/377. After #1 and #16 have been resolved kotlin-inject-anvil now supports arguments for scopes, too. That makes the API awkward, because there are two ways to contribute and merge classes depending on the scope.

With arguments:

@ContributesTo(AppScope::class)
interface Abc

@Inject
@SingleIn(AppScope::class)
@ContributesBinding(AppScope::class)
class Impl : Api

@Component
@MergeComponent(AppScope::class)
@SingleIn(AppScope::class)
abstract class Component : ComponentMerged

Without arguments:

@ContributesTo
@Singleton
interface Abc

@Inject
@Singleton
@ContributesBinding
class Impl : Api

@Component
@MergeComponent
@Singleton
abstract class Component : ComponentMerged

Scopes with an argument a preferred as the annotations are reusable across scopes and the API aligns with the original Anvil implementation for Dagger 2. However, the API for the preferred scenario is problematic, because all scope parameters on the @Contributes* annotations are optional. They're easy forget, which leads to surprising results and errors. The compiler cannot enforce them due to the nature of how scopes without arguments work.

The proposal is to remove support for scope annotation without arguments and make the scope parameter required similar to the original Anvil.

Note that this doesn't enforce to use the annotations from the :runtime-optional module and users could still bring their custom scope annotations, e.g. the example with arguments could be transformed into:

@ContributesTo(Singleton::class)
interface Abc

@Inject
@Singleton
@ContributesBinding(Singleton::class)
class Impl : Api

@Component
@MergeComponent(Singleton::class)
@Singleton
abstract class Component : ComponentMerged
ZacSweers commented 6 days ago

Def in favor of aligning on how the original anvil annotations worked in this sense (requiring scope params) 👍. I like the separation of scope parameters and actual scope annotations as well.

chrisbanes commented 6 days ago

+1 to Zac's comment.

The fact that we can set the binding scope and the contributed scope separately, is a big win.