adrielcafe / voyager

🛸 A pragmatic navigation library for Jetpack Compose
https://voyager.adriel.cafe
MIT License
2.6k stars 139 forks source link

Hilt integration issue #16

Closed Tolriq closed 3 years ago

Tolriq commented 3 years ago

Hi,

Testing this library and there's a small issue with current hilt integration if we do not want to use ScreenModel or are just testing before switching to them.

If we just add the voyager-hilt dependency then we end up with a dagger issue

error: [Dagger/MissingBinding] java.util.Map<java.lang.Class<? extends cafe.adriel.voyager.hilt.ScreenModelFactory>,javax.inject.Provider<cafe.adriel.voyager.hilt.ScreenModelFactory>> cannot be provided without an @Provides-annotated method.
  public abstract static class SingletonC implements Application_GeneratedInjector,
                         ^
      java.util.Map<java.lang.Class<? extends cafe.adriel.voyager.hilt.ScreenModelFactory>,javax.inject.Provider<cafe.adriel.voyager.hilt.ScreenModelFactory>> is requested at
          cafe.adriel.voyager.hilt.ScreenModelEntryPoint.screenModelFactories()

This can be worked around by just creating fake ScreenModel / ScreenModelFactory and a module to bind them but this should not be necessary or pointed in the doc if there's a better way to fix.

I'm still not fluent with multibinding so maybe there's something else I miss.

programadorthi commented 3 years ago

Hi @Tolriq. We need to use multibinding because this is the way that Dagger works with Androidx ViewModel. Hilt provides a @HitlViewModel annotation that remove all boilerplates required to use multibinding with AndroidxViewModel. This annotation doesn't works with ScreenModels. It's restricted to classes that is subclass of androidx.lifecycle.ViewModel. So putting @HiltViewModel on your ViewModel will generate something like that:

@Module
  @InstallIn(ViewModelComponent.class)
  public abstract static class BindsModule {
    private BindsModule() {
    }

    @Binds
    @IntoMap
    @StringKey("cafe.adriel.voyager.sample.hiltIntegration.HiltListViewModel")
    @HiltViewModelMap
    public abstract ViewModel binds(HiltListViewModel vm);
  }

We don't have an annotation to ScreenModel to generate multibinding to our. So we still need create our own multibinding structure manually:

// Apply `@Inject constructor` on your ScreenModel to have values injected by Hilt
class HiltListScreenModel @Inject constructor() : ScreenModel

// Create a module to have Multibinding declarations
@Module
@InstallIn(ActivityComponent::class) // Must be ActivityComponent at most. See https://dagger.dev/hilt/components
abstract class HiltModule {
    @Binds
    @IntoMap
    @ScreenModelKey(HiltListScreenModel::class) // A key to identify this instance on Multibinding Map
    abstract fun bindHiltListScreenModel(hiltListScreenModel: HiltListScreenModel): ScreenModel

    // Below is a version to have support to assisted injection because there is no support  by default. See https://github.com/google/dagger/issues/2287
    @Binds
    @IntoMap
    @ScreenModelFactoryKey(HiltDetailsScreenModel.Factory::class)
    abstract fun bindHiltDetailsScreenModelFactory(
        hiltDetailsScreenModelFactory: HiltDetailsScreenModel.Factory
    ): ScreenModelFactory
}

After this all boilerplate and setup, you can get a instance of your Screen doing:

class HiltListScreen : AndroidScreen() {
    @Composable
    override fun Content() {
        val screenModel: HiltListScreenModel = getScreenModel()
        // Or if you need a version with Assisted Injection do:
        val screenModel = getScreenModel<HiltDetailsScreenModel, HiltDetailsScreenModel.Factory> { factory ->
            factory.create(index)
        }
    }
}

There is no easy way. An alternative would to create our custom @HiltScreenModel annotation to avoid all multibinding boilerplate. But Hilt sources is out of our scope to learn and create a Kapt source generator for this purpose. Checkout our Hilt sample to have full insight.

Tolriq commented 3 years ago

@programadorthi yes I do understand why and how the binding works.

The issue here is that if we do not use nor want to use ScreenModel we are still forced to create 2 fake things (1 screenmodel + 1 screenmodelfactory) + the fake module + the fake binding to just have the app compile.

Even when just using screen model without factory we are forced to create a fake factory + bind it to compile.

To reproduce just create an empty project add, hilt + voyager-hilt and it fails.

If there's no way to have dagger work without providing the fake one (so be optional bindings), it should either be written in big red in the doc or be provided by the voyager-hilt module.

Adding the voyager-hilt to use viewmodel and have cryptic errors and create fake module is not a nice user experience in this case.

Edit: It's actually quite simple you just need to add

@Module
@InstallIn(ActivityComponent::class)
abstract class HiltModule {
    @Multibinds
    abstract fun screenModelMap(): Map<Class<out ScreenModel>, ScreenModel>?
    @Multibinds
    abstract fun screenModelFactoryMap(): Map<Class<out ScreenModelFactory>, ScreenModelFactory>?
}

to the voyager-hilt module to have them optional.

programadorthi commented 3 years ago

Thanks @Tolriq. We have implemented in #18

adrielcafe commented 3 years ago

Fixed on 1.0.0-beta13, feel free to reopen if the issue persists.