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

Fixes a bug that cannot use custom scope viewModel in mvrx-compose. #712

Merged
merged 3 commits into from Mar 19, 2024

Conversation

JSpiner
Copy link
Contributor

@JSpiner JSpiner commented Mar 18, 2024

What is bug?

mvrx-compose has a mavericksViewModel() function that can inject a viewModel from compose.

inline fun <reified VM : MavericksViewModel<S>, reified S : MavericksState> mavericksViewModel(
    **scope: LifecycleOwner = LocalLifecycleOwner.current,**
    noinline keyFactory: (() -> String)? = null,
    noinline argsFactory: (() -> Any?)? = null,
): VM {

In compose, it is necessary to manage viewModel with a different scope than fragment or activity for various reasons.
(ex: navigation component for compose)
So mavericksViewModel() is implemented to receive a custom scope as an argument.
However, when using mavericksViewModel in compose within a fragment, customScope is ignored and created as fragment scope.

if (parentFragment != null) {
    val args = argsFactory?.invoke() ?: parentFragment.arguments?.get(Mavericks.KEY_ARG)
    FragmentViewModelContext(activity, args, parentFragment) // viewModelStoreOwner, savedStateRegistry is ignored.
} else {
    val args = argsFactory?.invoke() ?: activity.intent.extras?.get(Mavericks.KEY_ARG)
    ActivityViewModelContext(activity, args, viewModelStoreOwner, savedStateRegistry)
}

This is because according to the internal implementation of mavericksViewModel, it ignores viewModelStoreOwner and saveStateRegistry if there is a parentFragment.
Looking at the Git history, it seems that when the maverickViewModel function was created, there were no viewModelStoreOwner and savedStateRegistry arguments in FragmentViewModelContext.

How to fix?

스크린샷 2024-03-18 14 32 04

Simply adding to use viewModelStoreOwner and savedStateRegistry solved it.
(The fragment's viewModelStoreOwner and savedStateRegistry were used as the default arguments.)

What changed

  1. I created some test code that fails.
    • I created a test for each activity and fragment to check whether the two customScopes create different viewModels.
    • Activity scope succeeds, fragment scope fails.
  2. Pass custom viewModelStoreOwner and savedStateRegistry as arguments to FragmentViewModelContext.
  3. I verified that the test was successful.

- Even if create and pass a custom lifecycle scope in Fragment, `mavericksViewModel` function uses the fragment's lifecycleScope.
…Owner and savedStateRegistry.

- Even if create and pass a custom lifecycle scope in Fragment, `mavericksViewModel` function uses the fragment's lifecycleScope.
@elihart
Copy link
Contributor

elihart commented Mar 18, 2024

Thanks for the writeup, fix, and tests. Before merging, I want to ensure that this change won't break the way that anybody is using the code now. Can you think of anything in the change that might not be backwards compatible?

@JSpiner
Copy link
Contributor Author

JSpiner commented Mar 19, 2024

@elihart
That's a good point. I am confident that there are no backward compatibility issues for the following two reasons.

  1. All existing and newly created tests passed.
  2. For code that does not use a custom scope, it operates logically the same.

And for code that use a custom scope, It wouldn't have worked properly. So I fixed the bug.

@elihart
Copy link
Contributor

elihart commented Mar 19, 2024

Ok, great, thanks for that. I'll merge once checks pass

@elihart elihart merged commit e88e8be into airbnb:main Mar 19, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants