[GithubBrowserSample] Recommended way to inject dynamic parameters to ViewModel using Dagger 2

[radzio]

Hi everyone! Thanks for great examples. I'm just wondering what method you recommend to inject dynamic parameters into ViewModel using Dagger 2? Currently in the GithubBrowserSample such parameters are injected by hand using setters:

userViewModel = ViewModelProviders.of(this, viewModelFactory).get(UserViewModel.class);
userViewModel.setLogin(getArguments().getString(LOGIN_KEY));

I've tried to move login String to the UserViewModel constructor but unfortunately, it caused the problem:

Error:(35, 8) error: [dagger.android.AndroidInjector.inject(T)] @javax.inject.Named("login") java.lang.String cannot be provided without an @Provides-annotated method.
@javax.inject.Named("login") java.lang.String is injected at
com.android.example.github.ui.user.UserViewModel.<init>(…, loginString)
com.android.example.github.ui.user.UserViewModel is injected at
com.android.example.github.di.ViewModelModule.bindUserViewModel(userViewModel)
java.util.Map<java.lang.Class<? extends android.arch.lifecycle.ViewModel>,javax.inject.Provider<android.arch.lifecycle.ViewModel>> is injected at
com.android.example.github.viewmodel.GithubViewModelFactory.<init>(creators)
com.android.example.github.viewmodel.GithubViewModelFactory is injected at
com.android.example.github.di.ViewModelModule.bindViewModelFactory(factory)
android.arch.lifecycle.ViewModelProvider.Factory is injected at
com.android.example.github.ui.repo.RepoFragment.viewModelFactory
com.android.example.github.ui.repo.RepoFragment is injected at
dagger.android.AndroidInjector.inject(arg0)

So... what is your recommended way to pass dynamic parameters to the constructor instead of setters? I see one solution - using separate ViewModelFactory implementation per each ViewModel. What do you think about it? Do you have any other recommendations?

[AkshayChordiya]

One way is constructor injection using the following syntax: class YourViewModel @Inject constructor(var someInjection: String) : ViewModel()

[radzio]

@AkshayChordiya I know that this is one of the possible ways but it does not work with the Dagger 2, the @ContributesAndroidInjector annotation and the ViewModelFactory from the GithubBrowserSample

[AkshayChordiya]

@radzio Can you provide more details? Because it is working in my case with ViewModelFactory

[radzio]

@AkshayChordiya try this demo project: https://github.com/radzio/android-architecture-components/commit/f77b08ea895e71963980e48190b010663113eece

[magneticflux-]

@radzio After cloning and looking at your code, I believe that the issue is one of scope. I believe the ViewModel factories reside above the fragment scope. The end result requires that something derived from the Fragment resides in the ViewModel, which is a huge no-no.

Basically, imagine that instead of injecting a String login, you injected a Provider<String> login. Now, your ViewModel would possess a factory for generating Strings. This is fine, but now it depends on the UserFragment.UserFragmentModule method that gets a String from a UserFragment. That method then depends on the UserFragment, and suddenly you just leaked all of your views, fragments, etc.! Dagger injection (may) result(s) in a permanent connection from injector to injectee. That means that setters (or a similar system) must be used to enforce separation as much as possible.

Here is a comment from an earlier issue that explains the issue in a slightly different way: link.

[radzio]

@magneticflux-

I just want to know what is the recommended way to inject values from intent into ViewModel using Dagger. I just want to get rid of this: userViewModel.setLogin(getArguments().getString(LOGIN_KEY));

and use Dagger for injecting it.

[magneticflux-]

@radzio I feel like the best way to transfer data from View to ViewModel is by using those setter methods. In order for MVVM architecture to work, the View must push commands to the ViewModel and receive data from a binding of some sort. By allowing the ViewModel to "pull" data from the View (through a Dagger injection), the architecture is broken and you run into problems.

You also have to keep in mind the lifecycle of your components. ViewModels exist for a long time, so they can't reference any views. I think Dagger is not the right tool for the job, and that the userViewModel.setLogin(getArguments().getString(LOGIN_KEY)); that you want to get rid of is actually helping to clearly divide the architecture into Model, ViewModel, and View.

[radzio]

You don't have to explain to me that ViewModels can't reference any views and how MVVM works :)

IMHO passing "const" values to the ViewModel in the constructor is ok especially when ViewModel are persisted between orientation changes. Why do I need to call setLogin each time user rotates the screen?

[Maragues]

@radzio the idea is to inject the ViewModel.Factory that'll be responsible for creating the ViewModel

A possible approach is to provide the ViewModelProvider.Factory in each Activity/Fragment module, which you can specify in the BindingsModule. The problem is the verbosity needed to create a new screen: Activity + ActivityModule + ViewModel + ViewModelProvider.Factory with duplicated code (as you'll see).

As an example, this'd be the ActivityModule

@Module
public class MyActivityModule {
  @Provides
  @ActivityScope
  ViewModelProvider.Factory providesViewModelFactory(MyActivity activity, MyDependency dependency) {
    return new MyActivityViewModel.Factory(dependency, activity.getLogin());
  }
}

This is the ViewModel + ViewModelProvider.Factory

public class MyActivityViewModel extends ViewModel {

  private final MyDependency dependency;
  private final String login;

  MyActivityViewModel(MyDependency dependency, String login) {
    this.facade = facade;
    this.login = login;
  }

  static class Factory implements ViewModelProvider.Factory {
    private final MyDependency dependency;
    private final String login;

    public Factory(MyDependency dependency, String login) {
      this.facade = facade;
      this.login = login;
    }

    @SuppressWarnings("unchecked")
    @Override
    public MyActivityViewModel create(Class modelClass) {
      return new MyActivityViewModel(dependency, login);
    }
  }
}

And then, the Activity

class MyActivity ...{
  @Inject
  ViewModelProvider.Factory viewModelFactory;

  @Override
  protected void onCreate(Bundle savedInstanceState) {
    AndroidInjection.inject(this);
    super.onCreate(savedInstanceState);

    viewModel = ViewModelProviders.of(this, viewModelFactory).get(MyActivityViewModel.class);
}

 String getLogin(){
   return getIntent.getString(EXTRA_LOGIN);
  }
}

There must be a better way, but it's what I'm using.

Like this, it's easy to provide the dependencies when testing ViewModels in isolation.

[magneticflux-]

@radzio Sorry, I didn't mean to patronize, I was just making sure we were on the same page.

There is a very good reason why you must call setLogin every time the activity is recreated, and that is due to some peculiarities in the Android lifecycle. If you app is killed in the background to save memory, the activity is persisted using onSaveInstanceState and then it is destroyed along with the ViewModel. When you use the ViewModelProviders.of method, you may, but are not guaranteed to receive an already-used ViewModel.

If you, for instance, perform long-running tasks in setLogin, you can store the previous login value and only update the data if the previous login value is missing or different from the parameter.

[matejdro]

If you, for instance, perform long-running tasks in setLogin, you can store the previous login value and only update the data if the previous login value is missing or different from the parameter.

While that is true, method from @radzio seem to handle all of that without resorting to extra unnecessary if statements, as constructor will only be used when new ViewModel instance is created.

[O10]

I'm looking for a clean solution for this one for some time.

What do you think about simply moving ViewModelFactory from application scope to activity/fragment modules, and defining ViewModel bindings in those modules.

This way you don't have to define separate factory for each ViewModel. The view model is kept across configuration change, but the drawback is that you have to be careful what you are injecting into it to avoid leaks and that you are actually creating new factory object each time.

@Module
abstract class ActivityModule {

    @Binds
    abstract fun activity(activity: MainActivity): Activity

    @Binds
    abstract fun bindViewModelFactory(factory: MyViewModelFactory): ViewModelProvider.Factory

    @Binds
    @IntoMap
    @ViewModelKey(MainViewModel::class)
    abstract fun mainViewModel(mainViewModel: MainViewModel): ViewModel

    @Module
    companion object {

        @JvmStatic
        @Provides
        fun bundle(activity: Activity): Bundle = activity.intent.extras ?: Bundle.EMPTY

        @JvmStatic
        @Provides
        fun someParam(bundle: Bundle): String = bundle.getString("test", "abc")

    }
}

class MyViewModelFactory @Inject constructor(private val creators: Map<Class<out ViewModel>, @JvmSuppressWildcards Provider<ViewModel>>) : ViewModelProvider.Factory {

    override fun <T : ViewModel> create(modelClass: Class<T>): T {
        var creator: Provider<out ViewModel>? = creators[modelClass]
        if (creator == null) {
            for ((key, value) in creators) {
                if (modelClass.isAssignableFrom(key)) {
                    creator = value
                    break
                }
            }
        }
        if (creator == null) {
            throw IllegalArgumentException("unknown model class " + modelClass)
        }
        try {
            @Suppress("UNCHECKED_CAST")
            return creator.get() as T
        } catch (e: Exception) {
            throw RuntimeException(e)
        }
    }

}

[matejdro]

Issue https://github.com/googlesamples/android-architecture-components/issues/148 (which is kind of duplicate of this one) provides pretty good solution to this problem:

You can use AutoViewModelFactory to automatically generate factory for you with @Named parameters. After that you just define how can these parameters be retrieved from activity via Activity's module.

[aoben10]

I had issues in java until I used generics in my constructor like so

@Inject
public MyViewModelFactory(Map<Class<? extends ViewModel>, Provider<ViewModel>> viewModels){
   // Your code here
}

[Zhuinden]

Provider<ViewModel>

Ah so that's why you need the @JvmSuppressWildcards in Kotlin

[shredderskelton]

@radzio I understand your problem, it's very valid and ubiquitous: take a simple any simple list/details screen. A list of movies in a recyclerview, click one, opening a detail screen. This happens by sending the movieId to the DetailsFragment as an argument. So you want to create a DetailsViewModel with movieId as a constructor parameter. This makes the ViewModel nice and clean, no sloppy lateinit var, no complex setter with a bunch of clean up reset code, just a val movieId that can be used by all in the ViewModel to init properly and cleanly.

The problem is that with Android ViewModels they are kinda trying to do 2 things at once, be a kind of retained containers and a classic, pure Java/Kotlin view model where the view logic goes.

To get around this, we created a ViewHolder to separate these two concerns (the hashmap honours the retain concern, moving the actual view model logic down into a pure Kotlin class) and use Dagger Builder to pass the arguments to the Module itself and create a new ViewModel for each movieId.

abstract class ViewModelHolder : ViewModel()

class MovieDetailsViewModelHolder @Inject constructor() : ViewModelHolder() {

    private val builder = appComponent.createMovieDetailsComponent()

    private val viewModelMap = hashMapOf<String, MovieDetailsViewModel>()

    fun getViewModel(movieId: String) =
        viewModelMap.getOrPut(movieId) {
            builder
                .withModule(MovieDetailsModule(movieId))
                .build()
                .createViewModel()
        }
}

and the subcomponent to attach to your appComponent and scope it

@MovieDetailScope
@Subcomponent(modules = [MovieDetailModule::class])
interface MovieDetailComponent {
    fun createViewModel(): MovieDetailViewModel

    @Subcomponent.Builder
    interface Builder {
        fun withModule(module: MovieDetailModule): Builder
        fun build(): MovieDetailComponent
    }
}

@Scope
@Retention(AnnotationRetention.RUNTIME)
annotation class MovieDetailScope

It's a lot of fancy Dagger wizardry but it makes things a little cleaner where it counts - the View Model.

BTW with Koin this is much much easier: https://insert-koin.io/docs/2.0/documentation/reference/index.html#_declaring_injection_parameters

[Zhuinden]

Just use AssistedInjection see https://youtu.be/9fn5s8_CYJI?t=2088

don't need a subcomponent for this tbh

[iskuhis]

@radzio I am wondering did you find the best way to pass dynamic parameters to the constructor instead of setters?

[ghost]

Hi. I inject ViewModels with multibinding and have exactly the same issue. Before dagger I used a custom viemodelfactory with an argument, but don't know have to pass run time arguments with injection.

[Zhuinden]

I inject ViewModels with multibinding

Don't

[yurimachioni]

A lot of interesting takes here! I'd be interested to know how your opinions on this changed since https://developer.android.com/topic/libraries/architecture/viewmodel-savedstate went stable.

[Zhuinden]

They're working on the Dagger Hilt ViewModel Extension, which will encode the subcomponent multibinding anti-pattern - which will be an anti-pattern as it breaks scoping, and still won't support dynamic arguments, only injecting AndroidViewModel from Dagger (which they could already handle via reflection anyway).

You probably want to use AssistedInjection where you define a factory for each ViewModel, rather than hack around with multibinding (even though you are not actually confined to use 1 Factory).

[yurimachioni]

It is amazing how state preservation never seems to get completely solved, must be like the the 10th attempt by Google and still a lot of boilerplate, not 100% solution... I hope they go a completely different way with fuchsia about this.

[Zhuinden]

@yurimachioni personally I like that "too stupid to fail" approach where I just save everything to a bundle and then restore everything from bundle, as outlined here

But that's not Jetpack ViewModel + SavedStateHandle. :thinking:

Theoretically you could probably create some by state { if every ViewModel had a SavedStateHandle but the more you try to hide behind Kotlin delegates and auto-persisting .set/.get methods, the harder it becomes to customize in strange scenarios.

[bartekpacia]

I'm new to Dagger and I honestly think this is ridiculous, so much doubt and hassle about such a basic use case scenario.

[Zhuinden]

@bartekpacia i finally have a jetpack-based sample of it using AssistedInject: https://github.com/Zhuinden/Jetpack-Navigation-Fragment-NavGraph-Dagger-SavedStateHandle-FTUE-Experiment/blob/e184f00a2b4ba90f86b603dab128536042a937af/app/src/main/java/com/zhuinden/jetpacknavigationdaggersavedstatehandleftueexperiment/features/registration/RegistrationViewModel.kt#L31-L38

and

https://github.com/Zhuinden/Jetpack-Navigation-Fragment-NavGraph-Dagger-SavedStateHandle-FTUE-Experiment/blob/e184f00a2b4ba90f86b603dab128536042a937af/app/src/main/java/com/zhuinden/jetpacknavigationdaggersavedstatehandleftueexperiment/features/registration/CreateLoginCredentialsFragment.kt#L33-L35

which uses

https://github.com/Zhuinden/Jetpack-Navigation-Fragment-NavGraph-Dagger-SavedStateHandle-FTUE-Experiment/blob/e184f00a2b4ba90f86b603dab128536042a937af/app/src/main/java/com/zhuinden/jetpacknavigationdaggersavedstatehandleftueexperiment/utils/ViewModelUtils.kt#L41-L55