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

Mavericks as generic purpose state management #632

Closed
sav007 opened this issue Jun 8, 2022 · 19 comments
Closed

Mavericks as generic purpose state management #632

sav007 opened this issue Jun 8, 2022 · 19 comments

Comments

@sav007
Copy link
Collaborator

sav007 commented Jun 8, 2022

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
Copy link
Contributor

elihart commented Jun 8, 2022

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
Copy link
Collaborator Author

sav007 commented Jun 8, 2022

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
Copy link
Contributor

elihart commented Jun 8, 2022

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 added a commit to sav007/mavericks that referenced this issue Jun 13, 2022
Move everything that could be Android platform agnostic into the core module.

Part of airbnb#632
@sav007
Copy link
Collaborator Author

sav007 commented Jun 13, 2022

@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
Copy link
Contributor

elihart commented Jun 16, 2022

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
Copy link
Collaborator

gpeal commented Jun 16, 2022

What about MavericksStateOwner?

@sav007
Copy link
Collaborator Author

sav007 commented Jun 16, 2022

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
Copy link
Collaborator

gpeal commented Jun 16, 2022

I like MavericksStateOwner, personally. @elihart Wdyt?

@elihart
Copy link
Contributor

elihart commented Jun 17, 2022

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
Copy link
Contributor

elihart commented Jun 17, 2022

I think I like MavericksRepository

@gpeal
Copy link
Collaborator

gpeal commented Jun 17, 2022

@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
Copy link
Collaborator Author

sav007 commented Jun 17, 2022

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
Copy link
Collaborator

gpeal commented Jun 17, 2022

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

@elihart
Copy link
Contributor

elihart commented Jun 17, 2022

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

@sav007
Copy link
Collaborator Author

sav007 commented Jun 19, 2022

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
Copy link
Contributor

elihart commented Jun 22, 2022

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
Copy link
Collaborator Author

sav007 commented Jun 24, 2022

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
Copy link
Contributor

elihart commented Jun 24, 2022

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

elihart pushed a commit that referenced this issue Jul 25, 2022
* Introduce Mavericks core module

Move everything that could be Android platform agnostic into the core module.

Part of #632

* Rename `MavericksSateModel` to `MavericksRepository`

Introduce `ExperimentalMavericksApi` annotation to repository declarations, to denote that repository is experimental API.

Extract `MavericksBlockExecutions` out from `MavericksViewModelConfig`, as `MavericksViewModelConfig` resides in `mvrx` module.

Replace `MavericksRepositoryConfigFactory` with `MavericksRepositoryConfigProvider` for simplicity.

Fix docs in `mvrx-core` module

* Fix mutability checker

* Move some tests around

* Rename VM to Repository

* Feedback - remove `MavericksRepository.onExecute`
Return back `MavericksViewModel.viewModelScope` for backward compatibility.

* Lint

* Delegate over inheritance

* Docs

* Remove `MavericksRepositoryConfigProvider`

* Update docs sample

* Unit tests

* Update docs

* Lint
@sav007
Copy link
Collaborator Author

sav007 commented Aug 12, 2022

Closing as it was merged into main 🎉

@sav007 sav007 closed this as completed Aug 12, 2022
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

No branches or pull requests

3 participants