nickbutcher / plaid

An Android app which provides design news & inspiration as well as being an example of implementing material design.
Apache License 2.0
16.26k stars 3.16k forks source link

[About][Dagger] ❓Possibly redundant @Inject annotation on AboutStyler constructor #832

Open geaden opened 4 years ago

geaden commented 4 years ago

I noticed, that AboutStyler constructor has @Inject annotation, however at the same time AboutStyler is provided via @Provides from AboutActivityModule with @FeatureScope. Having looked at generated by Dagger source code, AboutStyler_Factory is not used anywhere and DaggerAboutComponent uses AboutActivityModule_ProvideAboutStylerFactory instead.

I removed @Inject annotation from AboutStyler and run ./gradlew :about:assembleDebug --profile. It seems everything is working, DaggerAboutComponent remained the same, but AboutStyler_Factory is not generated. Rough measurement on clean about module on my machine shew, that task :about:kaptDebugKotlin took 2.686s with @Inject and 2.299s without.

Moreover AboutActivity is already bounded in AboutComponent.Builder, so one can remove provideContext and provideAboutStyler from AboutActivityModule, keeping @Inject on AboutStyler. I also added @FeatureScope to AboutStyler class. This time generated AboutStyler_Factory is used by DaggerAboutComponent.

Diff

```diff --- about/build/generated/source/kapt/debug/io/plaidapp/about/dagger/DaggerAboutComponent.java 2020-05-15 22:03:28.000000000 +0300 +++ about/build/generated/source/kapt/debug/io/plaidapp/about/dagger/DaggerAboutComponent.java 2020-05-15 22:03:34.000000000 +0300 @@ -2,11 +2,13 @@ package io.plaidapp.about.dagger; import dagger.internal.DoubleCheck; +import dagger.internal.InstanceFactory; import dagger.internal.Preconditions; import in.uncod.android.bypass.Markdown; import io.plaidapp.about.ui.AboutActivity; import io.plaidapp.about.ui.AboutActivity_MembersInjector; import io.plaidapp.about.ui.AboutStyler; +import io.plaidapp.about.ui.AboutStyler_Factory; import io.plaidapp.about.ui.model.AboutViewModelFactory; import io.plaidapp.core.dagger.MarkdownModule; import io.plaidapp.core.dagger.MarkdownModule_ProvideMarkdownFactory; @@ -19,14 +21,16 @@ public final class DaggerAboutComponent implements AboutComponent { private final AboutActivityModule aboutActivityModule; - private Provider provideAboutStylerProvider; + private Provider activityProvider; + + private Provider aboutStylerProvider; private Provider provideMarkdownProvider; private DaggerAboutComponent(AboutActivityModule aboutActivityModuleParam, - MarkdownModule markdownModuleParam, AboutActivity activity) { + MarkdownModule markdownModuleParam, AboutActivity activityParam) { this.aboutActivityModule = aboutActivityModuleParam; - initialize(aboutActivityModuleParam, markdownModuleParam, activity); + initialize(aboutActivityModuleParam, markdownModuleParam, activityParam); } public static AboutComponent.Builder builder() { @@ -34,12 +38,13 @@ } private AboutViewModelFactory getAboutViewModelFactory() { - return new AboutViewModelFactory(provideAboutStylerProvider.get(), AboutActivityModule_ProvideResourcesFactory.provideResources(aboutActivityModule), provideMarkdownProvider.get());} + return new AboutViewModelFactory(aboutStylerProvider.get(), AboutActivityModule_ProvideResourcesFactory.provideResources(aboutActivityModule), provideMarkdownProvider.get());} @SuppressWarnings("unchecked") private void initialize(final AboutActivityModule aboutActivityModuleParam, - final MarkdownModule markdownModuleParam, final AboutActivity activity) { - this.provideAboutStylerProvider = DoubleCheck.provider(AboutActivityModule_ProvideAboutStylerFactory.create(aboutActivityModuleParam)); + final MarkdownModule markdownModuleParam, final AboutActivity activityParam) { + this.activityProvider = InstanceFactory.create(activityParam); + this.aboutStylerProvider = DoubleCheck.provider(AboutStyler_Factory.create(activityProvider)); this.provideMarkdownProvider = DoubleCheck.provider(MarkdownModule_ProvideMarkdownFactory.create(markdownModuleParam)); } ```

The question is: is it safe to remove @Inject annotation from AboutStyler or it's better to remove provide* methods from AboutActivityModule and keep @Inject annotation on AboutStyler constructor? Or we have to keep everything as it is, but we'll have generated unused AboutStyler_Factory. This should not be a problem since R8/Proguard will strip such classes, but I suppose it might decrease build time during development, if there are more such cases.

I can make all required changes if needed.

Thanks.

geaden commented 4 years ago

I found this 3 years old article by @pyricau where @Inject is favored over @Provides. Probably, this should be taken into consideration.

pyricau commented 4 years ago

You can safely remove the @Inject, unfortunately Dagger doesn't error out when there are two ways to provide a binding. Creating AboutStyler requires an activity instance, which isnt available in the graph and is passed in as a module param.

geaden commented 4 years ago

You can safely remove the @Inject, unfortunately Dagger doesn't error out when there are two ways to provide a binding. Creating AboutStyler requires an activity instance, which isnt available in the graph and is passed in as a module param.

Great! Thanks for clarification!