Summary
This issue reports a crash when using mavericksViewModel() with androidx.navigation3.
The Problem
The mavericksViewModel() composable function implicitly assumes that its scope (which defaults to LocalLifecycleOwner.current) also implements the ViewModelStoreOwner and SavedStateRegistryOwner interfaces.
|
inline fun <reified VM : MavericksViewModel<S>, reified S : MavericksState> mavericksViewModel( |
|
scope: LifecycleOwner = LocalLifecycleOwner.current, |
|
noinline keyFactory: (() -> String)? = null, |
|
noinline argsFactory: (() -> Any?)? = null, |
|
): VM { |
|
val activity = extractActivityFromContext(LocalContext.current) |
|
checkNotNull(activity) { |
|
"Composable is not hosted in a ComponentActivity!" |
|
} |
|
|
|
val viewModelStoreOwner = scope as? ViewModelStoreOwner ?: error("LifecycleOwner must be a ViewModelStoreOwner!") |
|
val savedStateRegistryOwner = scope as? SavedStateRegistryOwner ?: error("LifecycleOwner must be a SavedStateRegistryOwner!") |
However, androidx.navigation3, following the principle of separation of concerns, provides these roles via distinct decorators, each exposing a different CompositionLocal.
Because these three Owners are distinct objects in the androidx.navigation3 environment, the current implementation of mavericksViewModel fails on its internal casts, leading to a crash.
Root Cause
The implementation of mavericksViewModel() depends on the specific pattern of traditional Android components (like Activity or Fragment) where a single class implements all three Owner interfaces. This compromises compatibility with modern libraries like androidx.navigation3 that have a more decoupled design.
Proposed Solution
I propose changing the default behavior of mavericksViewModel(). Instead of relying solely on LocalLifecycleOwner, it should individually resolve LocalLifecycleOwner, LocalViewModelStoreOwner, and LocalSavedStateRegistryOwner.
This change would allow the parameter-less mavericksViewModel() call to work robustly with libraries like androidx.navigation3.
There are two potential approaches for this:
Proposed Solution A: Change the signature (Ideal, Breaking Changes)
From an API design perspective, the cleanest solution is to change the signature of mavericksViewModel. This makes the function's dependencies explicit and clear.
// In mvrx-compose/src/main/kotlin/com/airbnb/mvrx/compose/MavericksComposeExtensions.kt
@Composable
@Suppress("DEPRECATION")
inline fun <reified VM : MavericksViewModel<S>, reified S : MavericksState> mavericksViewModel(
// Split arguments by responsibility
lifecycleOwner: LifecycleOwner = LocalLifecycleOwner.current,
viewModelStoreOwner: ViewModelStoreOwner = checkNotNull(LocalViewModelStoreOwner.current) {
"ViewModelStoreOwner not found!"
},
savedStateRegistryOwner: SavedStateRegistryOwner = checkNotNull(LocalSavedStateRegistryOwner.current) {
"SavedStateRegistryOwner not found!"
},
noinline keyFactory: (() -> String)? = null,
noinline argsFactory: (() -> Any?)? = null,
): VM {
val activity = extractActivityFromContext(LocalContext.current)
checkNotNull(activity) { "Composable is not hosted in a ComponentActivity!" }
val savedStateRegistry = savedStateRegistryOwner.savedStateRegistry
val viewModelClass = VM::class
val view = LocalView.current
// Use `lifecycleOwner` instead of `scope`
val viewModelContext = remember(lifecycleOwner, activity, viewModelStoreOwner, savedStateRegistry) {
val parentFragment = when (lifecycleOwner) {
is Fragment -> lifecycleOwner
is ComponentActivity -> null
else -> findFragmentFromView(view)
}
if (parentFragment != null) {
val args = argsFactory?.invoke() ?: parentFragment.arguments?.get(Mavericks.KEY_ARG)
FragmentViewModelContext(activity, args, parentFragment, viewModelStoreOwner, savedStateRegistry)
} else {
val args = argsFactory?.invoke() ?: activity.intent.extras?.get(Mavericks.KEY_ARG)
ActivityViewModelContext(activity, args, viewModelStoreOwner, savedStateRegistry)
}
}
return remember(viewModelClass, viewModelContext) {
MavericksViewModelProvider.get(
viewModelClass = viewModelClass.java,
stateClass = S::class.java,
viewModelContext = viewModelContext,
key = keyFactory?.invoke() ?: viewModelClass.java.name
)
}
}
Proposed Solution B: Change the internal behavior (Backward-Compatible)
If breaking changes should be avoided, an alternative is to maintain the existing scope: LifecycleOwner argument but change the internal behavior to check if the scope is using its default value.
// In mvrx-compose/src/main/kotlin/com/airbnb/mvrx/compose/MavericksComposeExtensions.kt
@Composable
@Suppress("DEPRECATION")
inline fun <reified VM : MavericksViewModel<S>, reified S : MavericksState> mavericksViewModel(
scope: LifecycleOwner = LocalLifecycleOwner.current,
noinline keyFactory: (() -> String)? = null,
noinline argsFactory: (() -> Any?)? = null,
): VM {
val activity = extractActivityFromContext(LocalContext.current)
checkNotNull(activity) { "Composable is not hosted in a ComponentActivity!" }
val localLifecycleOwner = LocalLifecycleOwner.current
val viewModelStoreOwner: ViewModelStoreOwner
val savedStateRegistryOwner: SavedStateRegistryOwner
// Determine if the scope is the default, or if it was explicitly provided.
if (scope === localLifecycleOwner) {
// If default, resolve each owner from its respective CompositionLocal.
viewModelStoreOwner = checkNotNull(LocalViewModelStoreOwner.current) {
"ViewModelStoreOwner not found!"
}
savedStateRegistryOwner = checkNotNull(LocalSavedStateRegistryOwner.current) {
"SavedStateRegistryOwner not found!"
}
} else {
// If scope was explicitly provided, maintain the existing behavior.
viewModelStoreOwner = scope as? ViewModelStoreOwner ?: error("Provided scope must be a ViewModelStoreOwner!")
savedStateRegistryOwner = scope as? SavedStateRegistryOwner ?: error("Provided scope must be a SavedStateRegistryOwner!")
}
// ... (The rest of the implementation is the same as in Solution A, starting from `val savedStateRegistry = ...`)
}
Contribution
If either of these proposed solutions is considered appropriate, I would be happy to create a Pull Request to implement it. Please let me know what you think.
Current Workaround
For now, the issue can be circumvented by creating the following helper function and using it instead of the standard mavericksViewModel().
import androidx.compose.runtime.Composable
import androidx.compose.runtime.remember
import androidx.lifecycle.Lifecycle
import androidx.lifecycle.LifecycleOwner
import androidx.lifecycle.ViewModelStoreOwner
import androidx.lifecycle.compose.LocalLifecycleOwner
import androidx.lifecycle.viewmodel.compose.LocalViewModelStoreOwner
import androidx.savedstate.SavedStateRegistryOwner
import androidx.savedstate.compose.LocalSavedStateRegistryOwner
import com.airbnb.mvrx.MavericksState
import com.airbnb.mvrx.MavericksViewModel
import com.airbnb.mvrx.compose.mavericksViewModel
@Composable
inline fun <reified VM : MavericksViewModel<S>, reified S : MavericksState> mavericksNav3ViewModel(
noinline keyFactory: (() -> String)? = null,
noinline argsFactory: (() -> Any?)? = null,
): VM {
val lifecycleOwner = LocalLifecycleOwner.current
val viewModelStoreOwner = checkNotNull(LocalViewModelStoreOwner.current) {
"ViewModelStoreOwner not found. Did you forget to add rememberViewModelStoreNavEntryDecorator()?"
}
val savedStateRegistryOwner = checkNotNull(LocalSavedStateRegistryOwner.current) {
"SavedStateRegistryOwner not found. Did you forget to add rememberSavedStateNavEntryDecorator()?"
}
val mavericksScope = remember(lifecycleOwner, viewModelStoreOwner, savedStateRegistryOwner) {
object : ViewModelStoreOwner by viewModelStoreOwner,
SavedStateRegistryOwner by savedStateRegistryOwner {
override val lifecycle: Lifecycle
get() = lifecycleOwner.lifecycle
}
}
return mavericksViewModel(
scope = mavericksScope,
keyFactory = keyFactory,
argsFactory = argsFactory,
)
}
Summary
This issue reports a crash when using
mavericksViewModel()withandroidx.navigation3.The Problem
The
mavericksViewModel()composable function implicitly assumes that its scope (which defaults toLocalLifecycleOwner.current) also implements theViewModelStoreOwnerandSavedStateRegistryOwnerinterfaces.mavericks/mvrx-compose/src/main/kotlin/com/airbnb/mvrx/compose/MavericksComposeExtensions.kt
Lines 57 to 68 in 09d84b0
However,
androidx.navigation3, following the principle of separation of concerns, provides these roles via distinct decorators, each exposing a differentCompositionLocal.LifecycleOwneris provided by:The internal
transitionAwareLifecycleNavEntryDecoratorused byNavDisplay, which provides aLifecycleOwnerthat is aware of transition animations.(See: navigation3/navigation3-ui/src/commonMain/kotlin/androidx/navigation3/ui/TransitionAwareLifecycleNavEntryDecorator.kt)
SavedStateRegistryOwneris provided by:rememberSavedStateNavEntryDecorator, which provides aLocalSavedStateRegistryOwner.(See: navigation3/navigation3-runtime/src/commonMain/kotlin/androidx/navigation3/runtime/SavedStateNavEntryDecorator.kt)
ViewModelStoreOwneris provided by:rememberViewModelStoreNavEntryDecorator, which provides aLocalViewModelStoreOwner.(See: lifecycle-viewmodel-navigation3/src/main/kotlin/androidx/lifecycle/viewmodel/navigation3/ViewModelStoreNavEntryDecorator.kt)
Because these three
Owners are distinct objects in theandroidx.navigation3environment, the current implementation ofmavericksViewModelfails on its internal casts, leading to a crash.Root Cause
The implementation of
mavericksViewModel()depends on the specific pattern of traditional Android components (likeActivityorFragment) where a single class implements all three Owner interfaces. This compromises compatibility with modern libraries likeandroidx.navigation3that have a more decoupled design.Proposed Solution
I propose changing the default behavior of
mavericksViewModel(). Instead of relying solely onLocalLifecycleOwner, it should individually resolveLocalLifecycleOwner,LocalViewModelStoreOwner, andLocalSavedStateRegistryOwner.This change would allow the parameter-less
mavericksViewModel()call to work robustly with libraries likeandroidx.navigation3.There are two potential approaches for this:
Proposed Solution A: Change the signature (Ideal, Breaking Changes)
From an API design perspective, the cleanest solution is to change the signature of mavericksViewModel. This makes the function's dependencies explicit and clear.
Proposed Solution B: Change the internal behavior (Backward-Compatible)
If breaking changes should be avoided, an alternative is to maintain the existing scope: LifecycleOwner argument but change the internal behavior to check if the scope is using its default value.
Contribution
If either of these proposed solutions is considered appropriate, I would be happy to create a Pull Request to implement it. Please let me know what you think.
Current Workaround
For now, the issue can be circumvented by creating the following helper function and using it instead of the standard
mavericksViewModel().