google / dagger

A fast dependency injector for Android and Java.
https://dagger.dev
Apache License 2.0
17.45k stars 2.02k forks source link

Dagger/Hilt ViewModel Injection (with compose and navigation-compose) #2166

Closed jaqxues closed 3 years ago

jaqxues commented 4 years ago

Hello,

I am currently trying to build an app with only Compose (meaning no Fragments and navigation-compose, along with architecture components such as Hilt and ViewModel).

I tried using the viewModel function with the defaultViewModelProviderFactory of the Activity.

    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.ViewModelProvider.get(ViewModelProvider.java:185)
        at androidx.lifecycle.ViewModelProvider.get(ViewModelProvider.java:150)
        at androidx.compose.ui.viewinterop.ViewModelKt.get(ViewModel.kt:75)
        at androidx.compose.ui.viewinterop.ViewModelKt.viewModel(ViewModel.kt:60)

I had to move this code inside a NavHost Composable. I reported this on the KotlinLang Slack and was told this issue relates to #2152 . It uses the incorrect Scope for a Navigation Composable.

In the case of the related issue, the scope is too small and in my case, it is the exact opposite Problem.

In that issue I linked, the fragment is within the Navigation graph, so the issue is that the saved state is too small of a scope (the navigation graph encompasses multiple fragments). In your Compose case, the entire navigation graph in within the single Activity/Fragment, so there the scope is too large and you end up saving state multiple times with the same key.

Although the issue should be fixed by a more correct approach to scoping, it is still worth to file a bug for the inverse problem with saved state.

Tlaster commented 4 years ago

Run into the same issue, a quick workaround is to pass a UUID to viewModel() as key, but this will create a new view model every time.

danysantiago commented 4 years ago

What version of the AndroidX ViewModel extension are you using? The issue that caused the java.lang.IllegalArgumentException: SavedStateProvider with the given key is already registered was fixed in alpha02 of the extension: https://developer.android.com/jetpack/androidx/releases/hilt#1.0.0-alpha02

Tlaster commented 4 years ago

I'm using 1.0.0-alpha02. Note that this happens when using navigation-compose with jetpack compose only, which means that there's only a single activity without any fragment. A quick example like this:

@AndroidEntryPoint
class ComposeActivity : ComponentActivity() {
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContent {
            val navController = rememberNavController()
            NavHost(navController = navController, startDestination = "home") {
                composable("home") {
                    val viewModel = viewModel<HomeViewModel>(factory = defaultViewModelProviderFactory)
                    Button(onClick = {
                        navController.navigate("home2")
                    }) {
                        Text(text = "Click me!")
                    }
                }
                composable("home2") {
                    val viewModel = viewModel<HomeViewModel>(factory = defaultViewModelProviderFactory)
                    Text(text = "home2")
                }
            }
        }
    }
}

class HomeViewModel @ViewModelInject constructor(
    val sharedPreferences: SharedPreferences
) : ViewModel()

When you click the button and navigate to "home2", the app will crash

Bodo1981 commented 4 years ago

I am facing the same issue but for me the app crashes immediatelly when navigating to the second screen. it would be great if we can have a temporarly workaround and maybe a fixed version soon. thx for the great work

danysantiago commented 4 years ago

Thanks for the sample code! It looks like you are hitting the same issue I've described here: https://github.com/google/dagger/issues/2152#issuecomment-722706928

In essence using the activity or fragment as the SavedStateRegistryOwner when the ViewModelStoreOwners is not the same will cause your ViewModel to be different between the your two navigation destinations but because the SavedStateRegistryOwner has a higher scope it complains when trying to provide the same SavedStateHandle that was already consumed. We need to make HiltViewModelFactory use the SavedStateRegistryOwner provided by the Navigation library and specifically the NavBackStackEntry.

Sadly there is no workaround for now since HiltViewModelFactory's constructor is package-protected so you can't build it yourself with the right SavedStateRegistryOwner. We'll try to get this fixed soon!

Tlaster commented 4 years ago

Here is a complete workaround using reflection

@Composable
inline fun <reified VM : ViewModel> navViewModel(
    key: String? = null,
    factory: ViewModelProvider.Factory? = AmbientViewModelProviderFactory.current,
): VM {
    val navController = AmbientNavController.current
    val backStackEntry = navController.currentBackStackEntryAsState().value
    return if (backStackEntry != null) {
        // Hack for navigation viewModel
        val application = AmbientApplication.current
        val viewModelFactories = AmbientViewModelFactoriesMap.current
        val delegate = SavedStateViewModelFactory(application, backStackEntry, null)
        val hiltViewModelFactory = HiltViewModelFactory::class.java.declaredConstructors.first()
            .newInstance(backStackEntry, null, delegate, viewModelFactories) as HiltViewModelFactory
        viewModel(key, hiltViewModelFactory)
    } else {
        viewModel(key, factory)
    }
}

@Composable
fun ProvideNavigationViewModelFactoryMap(factory: HiltViewModelFactory, content: @Composable () -> Unit) {
    // Hack for navigation viewModel
    val factories =
        HiltViewModelFactory::class.java.getDeclaredField("mViewModelFactories").also { it.isAccessible = true }
            .get(factory).let {
                it as Map<String, ViewModelAssistedFactory<out ViewModel>>
            }
    Providers(
        AmbientViewModelFactoriesMap provides factories
    ) {
        content.invoke()
    }
}

usage:

val AmbientApplication = staticAmbientOf<Application>()

@AndroidEntryPoint
class ComposeActivity : ComponentActivity() {
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContent {
            val navController = rememberNavController()
            Providers(
                AmbientApplication provides application
            ) {
                ProvideNavigationViewModelFactoryMap(factory = defaultViewModelProviderFactory as HiltViewModelFactory) {
                    NavHost(navController = navController, startDestination = "home") {
                        composable("home") {
                            val viewModel = navViewModel<HomeViewModel>()
                            Button(onClick = {
                                navController.navigate("home2")
                            }) {
                                Text(text = "Click me!")
                            }
                        }
                        composable("home2") {
                            val viewModel = navViewModel<HomeViewModel>()
                            Text(text = "home2")
                        }
                    }
                }
            }
        }
    }
}

class HomeViewModel @ViewModelInject constructor(
    val sharedPreferences: SharedPreferences
) : ViewModel()
Bradleycorn commented 3 years ago

@Tlaster - In your workaround, what are AmbientViewModelProviderFactory, AmbientNavController, and AmbientViewModelFactoriesMap? Where do you set those up?

Guimareshh commented 3 years ago

@danysantiago just saw you referenced the issue in a commit inside AndroidX for Hilt package. Do you have any ETA on this being released ? The last update for androidx.hilt was in July 2020.

Also, for those using only Dagger2 and not Hilt. If we want to go full Compose (no Fragments and navigation-compose) do we have to switch to Hilt to make DI works ?

danysantiago commented 3 years ago

@Guimareshh, we have a scheduled release of the androidx.hilt artifacts on January 27th (if all goes well), sorry for the trouble. As the reference comment shows, there will be a hilt-navigation artifact you can use to get retrieve a ViewModel out of a NavBackStackEntry

You don't have to switch to Hilt to make DI work with Compose, but Hilt still makes some things easier, such as App, Activity and Service injections along with ViewModel injection. Note that the functions in hilt-navigation scheduled for release will also let you use navigation-compose ViewModels which have more granular scopes.

Guimareshh commented 3 years ago

Thanks for the detailed reply @danysantiago 👍

About your answer on Dagger/Hilt: I asked you this because I'm not comfortable with the design decisions of Hilt not being able to insert components in the middle of the hierarchy (explained here). I imagine that if I want to go with Dagger2 without Hilt, with a full Compose app (with navigation-compose), I will have to build my own Factories.

rafipanoyan commented 3 years ago

@danysantiago Indeed as soon as a custom component is needed to inject our objects from, Hilt does not avoid the dagger-style boilerplate since we need entry points to inject the dependencies if I'm not mistaken

argestes commented 3 years ago

@danysantiago Did you release said component?

danysantiago commented 3 years ago

Hey - For Compose you can use hilt-navigation:

val myViewModel: MyViewModel = viewModel(HiltViewModelFactory(AmbientContext.current, backStackEntry))

We didn't release a hilt-navigation-compose artifact because we are still trying to hash out the APIs: https://android-review.googlesource.com/c/platform/frameworks/support/+/1551264

I'll close this for now since HiltViewModelFactory(AmbientContext.current, backStackEntry) should unblock you but I do think we can make this better and nicer. :)

mitchtabian commented 3 years ago

The hilt-navigation version 1.0.0-alpha03 is great. ViewModels are retained via the NavBackStackEntry now. Love it.

There's still a limitation with building a "compose only" navigation system though. I will try to explain with an example.

Suppose you have a bottom nav with 3 items. You could easily implement that with something like this:

val navController = rememberNavController()
Scaffold(
    bottomBar = {
        BottomNav(navController)
    }
) {
    NavHost(navController = navController, startDestination = Home.route) {
        composable(route = Home.route) {
            HomeScreen()
        }
        composable(route = Profile.route) {
            ProfileScreen()
        }
        composable(route = Settings.route) {
            SettingsScreen()
        }
    }
}

nav demo

Navigating using the back button works great now (Data is persisted) since the ViewModel gets its ViewModelStoreOwner from the NavBackStackEntry. Horray.

The problem

But if you click on any of the entries a second time, the screen stacks. A new viewmodel is created since a new NavBackStackEntry is created.

2

Solution?

We need something to prevent the stacking of back stack entries. If an item is re-selected, it should be removed from its position in the stack and placed at the top. 3

launchSingleTop is great but only works if the entry is already at the top of the stack.

Thanks and sorry for the long post I probably should have created a feature request. One might already exists?

danysantiago commented 3 years ago

@mitchtabian, these screenshots are amazing at explaining the issue, thanks!

However, this is not a Hilt + ViewModel issue and more of the way Navigation currently works. If the NavBackStackEntry is being removed and a new one is created so will the ViewModels. It seems you want multiple back stacks which is a highly requested feature in the Navigation library already and something that is being worked on. I suggest you file a feature request in the Navigation issue tracker here to make sure this is also available for navigation-compose.

05nelsonm commented 3 years ago

@mitchtabian See below for your boolean:

val isOnBackstack = try {
    navController.getBackStackEntry(destinationID)
    true
} catch(e: IllegalArgumentException) {
    false
}
mitchtabian commented 3 years ago

@danysantiago Thank you for your reply!

@05nelsonm Yes of course I can get a boolean that tells me if it's already in the stack. What I meant was if we as developers had a simple boolean similar to launchSingleTop that we could set to tell the navigation system how to behave.

mitchtabian commented 3 years ago

Created a feature request in case anyone wanted to star/follow. https://issuetracker.google.com/issues/178796184

kasem-sm commented 3 years ago

This is now fixed na? You told us in a video and we starred the issue haha