airbnb / mavericks

Mavericks: Android on Autopilot
https://airbnb.io/mavericks/
Apache License 2.0
5.83k stars 500 forks source link

Mavericks as generic purpose state management #632

Closed sav007 closed 2 years ago

sav007 commented 2 years ago

I know this feature request might be a bit of stretch but want to start a conversation.

Mavericks showed a great result as state management in Android VM world but looks like its purpose can be extended beyond it. What if Mavericks becomes a generic purpose state management where it can be used in pure Kotlin world? Like extract the engine that has 0 dependencies to Android artifacts and provide extension to connect it with Android VM?

There is a potential that Mavericks can be used anywhere where stateful component is needed (like repository that requires to have a state). Also having separate pure Kotlin engine will make it possible to use it outside of VM like it was mentioned in this issue and make it possible to bring it to iOS by making it KMP

I know these questions have been already asked before but has anyone done any assessment on the effort required for this kind of work? Are you open for such transformation? Do you think it makes sense and the time is right?

elihart commented 2 years ago

I agree with the sentiment and would love to see a general purpose multiplatform solution, but have not looked into feasibility or effort required. It's also unlikely I would be able to work on this, so it would need to come from external contributors. I would be open reviewing changes though, but it would need to be well planned out.

As far as whether now is the right time, I think it depends on the state of KMM tooling. I haven't used it myself so it is hard for me to say. I want to avoid making the repository hard to maintain due to KMM APIs changing/breaking overtime and configuration nightmares.

sav007 commented 2 years ago

Just to clarify that this feature request is not primarily about KMP, but rather extract the core of Mavericks ( state management) as pure Kotlin module and make VM integration as an extension.

KMP is just one of side effects of such transformation from other applications, where state management is required.

I think this process can be broken into 2 phases: first will be extracting core part and make it 0 dependent on Android platform, build extension for VM integration. And only after this second phase is making core as KMP module.

Even implementing only first phase I think provide more flexibility of using Mavericks outside of Android VM.

elihart commented 2 years ago

Gotcha, I see that a main use case is to use mavericks in a repository pattern outside of a UI scope.

Fwiw, we already use our viewmodels as Singletons using this pattern

/**
 * Singletons in the app that are not tied to an Activity/Fragment lifecycle should implement this.
 *
 * This allows our singletons to manage their State using the same tools that our UI ViewModels do.
 */
abstract class MavericksRepository<S : MvRxState>(
    initialState: S,
    executorOverride: SingleFireRequestExecutor? = null,
    customCoroutineContext: CoroutineContext = inject(MvRxDagger.AppGraph::viewModelCoroutineContext).value
) : MvRxViewModel<S>(initialState, executorOverride, customCoroutineContext) {

    /**
     * Clears this repository. This is NOT supported if this repository is a singleton with application scoped lifecycle.
     *
     * If the repository is scoped to a shorter lifecycle via Dagger then this can be used to clean up the repository
     * when its lifecycle ends.
     */
    override fun onCleared() {
        if (BuildHelper.isDevelopmentBuild()) {
            // Each singleton annotation class package is different per module, but the simple name should be consistent.
            val singletonAnnotationName = AirSingleton::class.java.simpleName
            val isSingleton = this::class.annotations.any {
                it.annotationClass.simpleName == singletonAnnotationName
            }
            if (isSingleton) {
                error("The repository ${this::class.simpleName} is a singleton and cannot be cleared.")
            }
        }
        super.onCleared()
    }
}

This helps improve the naming and pattern for the use case.

Of course, it would be great to have it better abstracted to decouple further from the viewmodel. If you are passionate about that then you get the green light from me. Especially "phase one" seems reasonable and hopefully not too hard; if we want to tackle KMP later on that sets the groundwork, but doesn't commit us to it yet.

Main concern would be maintaining api compatibility with current viewmodels. Especially if we can do that then no worries from me

sav007 commented 2 years ago

@elihart could you pls quickly take a look at my WIP PR, to check if it makes sense (sanity check) so I can flash out all small details to make it production ready?

There is new version MavericksStateModel that is main POI.

Also as I didn't change any of sample apps, API should be 100% the same.

elihart commented 2 years ago

Looks great, happy you were able to pull so much out without breaking API.

I like the direction. Mainly I'm wondering if MavericksStateModel is the best name; I suppose it makes sense, but it doesn't quite resonate with me

gpeal commented 2 years ago

What about MavericksStateOwner?

sav007 commented 2 years ago

Yeah agree, name MavericksStateModel was just a quick stub. We need to name something that going to be a parent for module that holds (owns) and manages state. Couple ideas MavericksStateOwner , MavericksStateHolder

Some biased thinking as our team use the concept of repository, MaverickStateRepository / MavericksRepository would work for us but not sure.

gpeal commented 2 years ago

I like MavericksStateOwner, personally. @elihart Wdyt?

elihart commented 2 years ago

yeah, i like MavericksStateOwner more, although imo it makes it sound like a simple wrapper rather than a potentially complex class that handles a bunch of business logic and data layer access

elihart commented 2 years ago

I think I like MavericksRepository

gpeal commented 2 years ago

@elihart I liked that but it inverts the problem we have today. Repository/ViewModel can be thought of as two different layers so making ViewModel extend Repository is as strange as the inverse imo.

sav007 commented 2 years ago

I can agree in some extent with Gabriel in general, but I don't think that it's very noticeable from usage of API, it's obscured from public API.

Also in some extent view model is a repository, abstraction that works with data access layer, but in our case it's stateful so it manages its state.

MavericksStateOwner I feel the same as Eli mentioned, it sounds like a simple wrapper not like a core class of framework.

gpeal commented 2 years ago

That logic makes sense to me. I'm okay with MavericksRepository unless we can think of something better 😄

elihart commented 2 years ago

Maybe start with MavericksRepository and mark it experiment? Leave it open to a name change later on if we want?

sav007 commented 2 years ago

Is MavericksViewModelConfig.BlockExecutions used in tests only?

I had to extract it into MavericksBlockExecutions to separate it from MavericksViewModelConfig that doesn't belong to mvrx-core module. So this is so far only one breaking changes if someone used MavericksViewModelConfig.BlockExecutions in tests before.

Wdyt how bad is this change?

elihart commented 2 years ago

MavericksViewModelConfig.BlockExecutions is used in tests as well as mocking for dev app type manual testing.

That doesn't sound like too bad of a breaking change. it will affect us in a couple places, but it's minimal. if we can provide a simple find/replace recommendation to update the imports that it seems fine

sav007 commented 2 years ago

Should we bump the version to 3.0.0? Also could you please give another round of review for PR? Meantime I will write some docs for migration and new module introduction

elihart commented 2 years ago

a version bump to 3.0.0 sounds fine to me. i'll try to review soon. thanks!

sav007 commented 2 years ago

Closing as it was merged into main 🎉