Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Hilt] Multiple viewmodel instances with different keys crash #2328

Closed
chpetersen opened this issue Jan 28, 2021 · 30 comments
Closed

[Hilt] Multiple viewmodel instances with different keys crash #2328

chpetersen opened this issue Jan 28, 2021 · 30 comments

Comments

@chpetersen
Copy link

chpetersen commented Jan 28, 2021

Hello

I have recently swapped to Hilt. Before we used dagger and had the following code which would allow us to have multiple instance of the same viewmodel, but with different keys:

fun <T : ViewModel> getUniqueViewModel(modelClass: Class<T>, key: String): T {
    val viewModel = ViewModelProvider.get(key, modelClass)
    return viewModel
}

Example of use:

val firstViewModel = getUniqueViewModel(TestViewModel.class, "key1")
val secondViewModel = getUniqueViewModel(TestViewModel.class, "key2")

But when I'm using Hilt I get the following error when I try to get my second instance:

java.lang.IllegalArgumentException: SavedStateProvider with the given key is already registered
        at androidx.savedstate.SavedStateRegistry.registerSavedStateProvider(SavedStateRegistry.java:111)
        at androidx.lifecycle.SavedStateHandleController.attachToLifecycle(SavedStateHandleController.java:50)
        at androidx.lifecycle.SavedStateHandleController.create(SavedStateHandleController.java:70)
        at androidx.lifecycle.AbstractSavedStateViewModelFactory.create(AbstractSavedStateViewModelFactory.java:67)
        at androidx.lifecycle.AbstractSavedStateViewModelFactory.create(AbstractSavedStateViewModelFactory.java:84)
        at dagger.hilt.android.internal.lifecycle.HiltViewModelFactory.create(HiltViewModelFactory.java:108)
        at androidx.lifecycle.ViewModelProvider.get(ViewModelProvider.java:187)
        at com.test.BaseActivity.getUniqueViewModel(BaseActivity.kt:162)

Doesn't Hilt allow multiple viewModels with different keys?

@frett
Copy link

frett commented Jan 28, 2021

I've run into the same issue when switching from the AndroidX hilt-lifecycle-viewmodel to the dagger hilt view model.

I believe the core of the issue is that HiltViewModelFactory implements ViewModelProvider.Factory and doesn't extend ViewModelProvider.KeyedFactory which provides support for defining custom keys for ViewModels.

@danysantiago
Copy link
Member

danysantiago commented Jan 28, 2021

Yup - Part of the problem is that ViewModelProvider.KeyedFactory is not a public API in androidx.lifecycle so we can't make HiltViewModelFactory a keyed factory so that the ViewModelProvider propagates the key.

We are working on a long term solution to have 'extras' be propagated via ViewModelProvider such that during ViewModel creation you get access to the SavedStateRegistryOwner, Application and ViewModel keys if available instead of having to create custom factories with tight inheritance.

Sadly, I don't have a workaround recommendation for this issue, I can only say to make distinct ViewModel classes with a shared parent since the ViewModel class is the key used.

We could try to make HiltViewModelFactory extend AbstractSavedStateViewModelFactory like it was in the androidx.hilt package but that has its own set of problems, specifically it eagerly creates a SavedStateHandle that might be thrown out if your ViewModel does not use it and it might cause conflicting key issues too when the ViewModel is not Hilt injected and is just a vanilla ViewModel.

@frett
Copy link

frett commented Jan 28, 2021

in my current use-case distinct ViewModels won't work, I'm currently using the ViewModel framework to load additional complex data about individual items in a recycler view. Each item gets its own distinct instance of the ViewModel and the key is based off the unique id of the recycler view item.

I'll probably just rework my use-case to store all the individual item ViewModels in a single parent ViewModel when I have a chance, until then I'll hold off on upgrading

@Zhuinden
Copy link

Zhuinden commented Jan 28, 2021

Doesn't Hilt allow multiple viewModels with different keys?

This isn't actually a Hilt issue. If you weren't using AbstractSavedStateViewModelFactory, only one of your ViewModels would be stored in the store, because the default key in the ViewModelStore is the canonical name of the ViewModel.

IIRC the code says "//TODO: log a warning".

https://android.googlesource.com/platform/frameworks/support/+/androidx-main/lifecycle/lifecycle-viewmodel/src/main/java/androidx/lifecycle/ViewModelProvider.java#181

So N-1 viewModels would actually not even be stored across config changes.

Anyway, the AbstractSavedStateViewModelFactory is a KeyedFactory

https://android.googlesource.com/platform/frameworks/support/+/refs/heads/androidx-main/lifecycle/lifecycle-viewmodel-savedstate/src/main/java/androidx/lifecycle/AbstractSavedStateViewModelFactory.java?source=post_page---------------------------%2F%2F%2F%2F%2F%2F%2F%2F%2F%2F%2F%2F%2F%2F%2F#35

So try ViewModelProvider(this, hiltViewModelFactory).get("viewModel1", MyViewModel::class.java) and ViewModelProvider(this, hiltViewModelFactory).get("viewModel2", MyViewModel::class.java)

We could try to make HiltViewModelFactory extend AbstractSavedStateViewModelFactory like it was in the androidx.hilt package

Oh.

@frett
Copy link

frett commented Jan 28, 2021

So try ViewModelProvider(this, hiltViewModelFactory).get("viewModel1", MyViewModel::class.java) and ViewModelProvider(this, hiltViewModelFactory).get("viewModel2", MyViewModel::class.java)

This is the problem this issue is surfacing, using that method call with 2 different keys but the same ViewModel class doesn't work with the current hilt ViewModel support because it is implementing ViewModelProvider.Factory and is not a descendant of KeyedFactory.

@Zhuinden
Copy link

Zhuinden commented Jan 28, 2021

Oh.... It stopped being an AbstractSavedStateViewModelFactory. I knew it used to be one.

Yeah, I'd consider that a Hilt issue. 🤔

@chpetersen
Copy link
Author

chpetersen commented Jan 29, 2021

So try ViewModelProvider(this, hiltViewModelFactory).get("viewModel1", MyViewModel::class.java) and ViewModelProvider(this, hiltViewModelFactory).get("viewModel2", MyViewModel::class.java)

I have tried several different ViewModelFactories, including the hiltViewModelFactory.
But as @danysantiago mention, non of them is a KeyedFactory, so the key parameter is never passed to the factory.

@Chang-Eric
Copy link
Member

Chang-Eric commented Feb 2, 2021

Just a quick summary, we are working on fixing this, but unfortunately just switching back to AbstractSavedStateViewModelFactory will not work due to injection timing issues and the SavedStateHandle. As @danysantiago mentioned above, we're looking at some long term solutions like that in https://android-review.googlesource.com/c/platform/frameworks/support/+/1535760. Unfortunately, until then, HiltViewModels will not be able to support multiple keys.

@JiangL08
Copy link

JiangL08 commented Jul 16, 2021

Any update about this issue?

@ammargitham
Copy link

ammargitham commented Oct 30, 2021

Seems the changes linked in #2328 (comment) has been fixed in https://issuetracker.google.com/issues/188541057. But it seems that it hasn't been released yet.

@kennir
Copy link

kennir commented Dec 13, 2021

Multi-instance is a very important feature for some type of project, Hope it can be fixed soon

Thanks

@Zhuinden
Copy link

Zhuinden commented Dec 13, 2021

it relies on this new "ViewModel CreationExtras" thingy 😅

@JuanchiFraga
Copy link

JuanchiFraga commented Dec 27, 2021

Any new about this issue?

@SecretA
Copy link

SecretA commented Mar 8, 2022

Any updates? We really need this.

@Chang-Eric
Copy link
Member

Chang-Eric commented Mar 10, 2022

The CreationExtras change that we need for this is unfortunately still in alpha https://developer.android.com/jetpack/androidx/releases/lifecycle#2.5.0-alpha01. Once that is stable, I think we can update Hilt's ViewModelFactory to pass through CreationExtras and then I think it would just work since the key is now derived from that instead of via that KeyedFactory interface mentioned in above comments.

@originx
Copy link

originx commented May 6, 2022

androidx.lifecycle moved to beta: Version 2.5.0-beta01 🥳, meaning the API is now stable enough to proceed with this?

@sbatezat
Copy link

sbatezat commented May 13, 2022

androidx.lifecycle moved to rc: Version 2.5.0-rc01 now, stable enough to proceed with this?

@Chang-Eric
Copy link
Member

Chang-Eric commented May 17, 2022

We need it to actually hit the stable release. After that happens (which since it is in RC should be relatively soon hopefully), we should be able to make the change in Hilt and do a release shortly after.

@tommyzat
Copy link

tommyzat commented Jun 30, 2022

Lifecycle hit 2.5.0 stable!

@Zhuinden
Copy link

Zhuinden commented Jul 3, 2022

Yes, it should now be possible.

@antwangorillas
Copy link

antwangorillas commented Jul 6, 2022

any update on this ?
Thanks

@Tgo1014
Copy link

Tgo1014 commented Jul 11, 2022

Also waiting for this much needed feature

@qiuzaiming
Copy link

qiuzaiming commented Jul 15, 2022

I have updated to the latest version 2.5.0, but this problem still exists.

@Chang-Eric
Copy link
Member

Chang-Eric commented Jul 15, 2022

Just upgrading to 2.5.0 isn't enough, it requires a change on the Hilt side. I have a PR to do that, but it has been delayed due to various dependency issues that need sorting out. We're aiming to get it submitted and then do a release next week, assuming no other issues come up.

@Chang-Eric
Copy link
Member

Chang-Eric commented Jul 18, 2022

Forgot to tag this issue in 74ea765, but that commit should fix this issue.

@sebaslogen
Copy link

sebaslogen commented Jul 21, 2022

And the fix is released 🎉 https://github.com/google/dagger/releases/tag/dagger-2.43
According to the release description, it fixes this issue. I'll test it tomorrow on my library.

Great job 👏

@tommyzat
Copy link

tommyzat commented Jul 23, 2022

What is the standard then for initiating hiltViewModels() with keys?

@sebaslogen
Copy link

sebaslogen commented Jul 23, 2022

What is the standard then for initiating hiltViewModels() with keys?

Until Hilt provides an API to use the key explicitly, you easily create one yourself using the viewModel(key = "myKey", factory = hiltFactory) (new API with creation extras) and create an instance of the Hilt factory like this.

FYI: I tested this in my own library and it works like a charm https://github.com/sebaslogen/resaca/blob/main/resacahilt/src/main/java/com/sebaslogen/resaca/hilt/ScopedMemoizers.kt#L57

@tommyzat
Copy link

tommyzat commented Aug 24, 2022

What is the standard then for initiating hiltViewModels() with keys?

Until Hilt provides an API to use the key explicitly, you easily create one yourself using the viewModel(key = "myKey", factory = hiltFactory) (new API with creation extras) and create an instance of the Hilt factory like this.

FYI: I tested this in my own library and it works like a charm https://github.com/sebaslogen/resaca/blob/main/resacahilt/src/main/java/com/sebaslogen/resaca/hilt/ScopedMemoizers.kt#L57

Thank you for this - I tried your library and it seems that only variables are scoped, while running coroutines on viewmodelscope are not. How can I fix this?

@sebaslogen
Copy link

sebaslogen commented Aug 25, 2022

What is the standard then for initiating hiltViewModels() with keys?

Thank you for this - I tried your library and it seems that only variables are scoped, while running coroutines on viewmodelscope are not. How can I fix this?

@tommyzat I'm not sure I fully understand your question correctly, but if it's about my library (https://github.com/sebaslogen/resaca) it's better to move the discussion out of this Hilt/Dagger issue. You can either open an issue on the Resaca GitHub space or contact me on Twitter @sebaslogen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests