Review Public API

[marcellogalhardo]

Our current API is working fine for our needs, but we may be able to improve it aiming to reduce cognitive load of developers using the library for the first time. For example, instead of multiple annotations we could come up with a couple of annotations and make the compiler smarter enough to understand what type is annotating.

The following proposal recommends:

// Members Injection.

@ContributesAndroidInjection
class MyApplication : Application() {
    fun onCreate() {
        Whetstone.inject(application = this)
    }
}

@ContributesAndroidInjection
class MyActivity : Activity() {
    fun onCreate() {
        Whetstone.inject(activity = this)
    }
}

@ContributesAndroidInjection
class MyService : Service() {
    fun onCreate() {
        Whetstone.inject(service = this)
    }
}

// Constructor Injection.

@ContributesAndroidBinding
class MyFragment @Inject constructor() : Fragment()

@ContributesAndroidBinding
class MyViewModel @Inject constructor() : ViewModel()

@ContributesAndroidBinding
class MyWorkManager @Inject constructor() : WorkManager()

For cases where "injection" is not required:

Whetstone.install(application = this)
Whetstone.install(activity = this)

As you can see, we can pretty much infer the target type and reduce all features to a couple of annotations: ContributesAndroidInjection for members injection and ContributesAndroidBinding for constructor injection (by using assisted injection).

We could also validate by type on the compiler level and by including a custom lint rule.

[kingsleyadio]

Update:
Currently, we have specialized annotations for different components. E.g ContributesFragment, ContributesViewModel, etc
We also recently switched to automatic discovery of these annotations, so we don't have to hard code them in the compiler
The approach to discovery is as follows (simplified):

• Collect all project classes
• Retrieve their annotations
• Check if any of these annotations have the AutoScopedBinding meta-annotation
• Read all necessary configuration from there and proceed with codegen accordingly

Checking for meta-annotation requires loading the annotation class type, which is a little more expensive that simply reading the AST of the annotated class

Now, the proposal in this issue takes a different direction, and follows this (simplified):

• Collect all project classes
• Filter for those that have specific annotations: ContributesAndroidInjection, ContributesAndroidBinding
• Recursively find the supertype of these classes by loading each intermediate one, until we find known types: Fragment, Activity, Service, etc. This is the most important step to be able to figure out to which scope/component the generated code should belong
• Collect predefined configuration for these types and proceed with codegen accordingly

This makes all the difference
For (1), there's only a finite set of unique annotations, and we can cache the necessary info from any previously loaded one. However, loading full classes and their super types doesn't lend themselves so much to caching (again loading types is more expensive that reading source AST). Best case, no need to do any loading if a class directly extends any of the known types. Worst case, a significant amount of super class loading until we finally get to the known types or nothing (we should always fail in case of nothing)

It's a classic performance vs UX problem 😄

Given these concerns, we'd like some external opinions to give insight into potential use cases so that we can have a wholistic view that would help us make the most informed decision

[kingsleyadio]

Regarding ContributesAndroidBinding, ContributesAndroidInjection:
An option here is adding an extra boundType parameter to both annotations in a case where an implementation doesn't extend the known type directly, then we'd be able to avoid the whole recursive class loading. This is similar to how anvil disambiguates types that extend multiple super types

[gent-ahmeti]

What do you think about using scope instead of boundType? We're using this boundType to figure out the scope anyway
For some reason having @ContributesAndroidBinding(scope = ActivityScope::class) feels more natural than @ContributesAndroidBinding(boundType = Activity::class) to me

[kingsleyadio]

This is interesting
I took some time to think about it: While scope immediately sounds interesting, and relatively more familiar since we use it more than boundType in most cases. It seems to me that scope alone might not be sufficient, and it might even introduce some semantic conflict

Take a look at this code

@ContributesAndroidBinding
class MyActivity: Activity()

The compiler can immediately figure out from this, that Activity is the super class, and we can already start doing something about that. If the compiler doesn't recognize this super type, we can supply a hint for it to understand what type this class is related to. Scope only really comes much later during actual codegen
If we had used scope instead, the compiler still needs to start from the super type, as that is the only information we have

Keep in mind that we're actually generating a BindingModule for the type, and scope only informs where we should install that module

Consider this from another perspective. Usually, scope as a parameter is never optional. Scope annotations (SingleIn, ForScope, etc), Contributes annotations (ContributesTo, ContributesBinding, etc). Even though with ContributesBinding, compiler can already figure out the type, we still have to supply a scope because they serve different purposes
Using this context, using scope in our case breaks that pattern. So it might introduce some extra cognitive overload

Lastly, in an error case where we can't figure out the super type, and user also hasn't supplied a "scope", we fail with an error that asks them to help us because we couldn't figure out the scope because we were looking for it by the type

What do you think?

[kingsleyadio]

I went ahead to give this a try: #18
It really does feel much nicer. And it also allows me to add some auto-discovery logic

I still feel a bit of doubt though, for reasons I haven't been able to properly articulate, besides what I already mentioned
Please go through it anyway and let me know what you think

[kingsleyadio]

Closing as won't do. An alternative solution is now implemented in #60