airbnb / mavericks

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

Decouple State and Args #83

Closed Mishkun closed 5 years ago

Mishkun commented 6 years ago

Now to pass some arguments to ViewModel we have to include them into the initialState, like in the example below.

data class MyState(val foo: Int) : MvRxState {
    constructor(args: MyArgs) : this(args.foo)
}

It works well in the simple examples, but sometimes we need some info that does not need to be rendered. An api parameter, some userId, for example. We have to use "dead" fields in state that make no sense for the View and only needed to construct a ViewModel or perform background operations.

@Parcelize
data class MyArgs(val userId: String)

data class MyState(val args: MyArgs, val coinsCount: Int = 0) : MvRxState{
    constructor(args: MyArgs) : this(args)
}

class MyViewModel(initialState: MyState) : BaseMvRxViewModel(initialState) {
    init {
        // retrieve `userId`
       initialState.args.let { 
             // use it 
       }
    }
}

The factory approach is not working, because MvRxViewModelFactory<MyState, MyViewModel>#create gets the internally constructed state.

Wouldn't be nice to have something like other MvRxViewModelArgFactory version, taking MyArgs in create?

class MyViewModel(private val args: MyArgs, initialState: MyState) : BaseMvRxViewModel(initialState) {
    init {
       // just use the args
       args.let { // ... }
    }
    companion object : MvRxViewModelArgFactory<MyState, MyViewModel> {
        @JvmStatic
        override fun create(activity: FragmentActivity, args: MyArgs): MyViewModel {
            return MyViewModel(args, createState())
        }
    }
}

Any thoughts? It would be a neat thing to hide this implementation details from the View

BenSchwab commented 6 years ago

Thanks for the suggestion. This seems reasonable to me. We initially believed that giving access to the activity (and in turn DI frameworks such as Dagger) would largely cover this, but I actually had an engineer at Airbnb have a similar need yesterday. Any concerns @gpeal?

gpeal commented 6 years ago

@Mishkun @BenSchwab The tricky part that led us to the current world is that MvRx handles process restoration so your ViewModel may be instantiated with non-initial state (restored state).

We wanted the mental model to be args -> initial state -> subsequent state

In the state restoration case, you would be given initial args + subsequent state which creates a potentially confusing world. I see the advantage of passing in args here and I'm not opposed to it, we just need to make sure that it makes sense for the view model restoration case as well.

Mishkun commented 6 years ago

@gpeal good note about state restoration, haven't think about that. To handle that case, we can add to our factory interface a restore method, that would give us the args and restored state so we can initialize viewmodel properly. To reduce the boilerplate in simple cases we can also provide a default implementation that will call create and then set the restored state.

BenSchwab commented 6 years ago

@gpeal I'm not sure I see how state restoration changes the interface (or mental model). Args should be the same in the state restoration case vs. the normal case.

@Mishkun I do see that I misread the suggestion a bit. I don't think we should make the ViewModelFactory responsible for creating initialState. I was more imagining just one extra arg from before:

 class MyViewModel(private val args: MyArgs, initialState: MyState) : BaseMvRxViewModel(initialState) {
    init {
       // just use the args
       args.let { // ... }
    }
    companion object : MvRxViewModelArgFactory<MyState, MyViewModel> {
        @JvmStatic
        override fun create(activity: FragmentActivity, initialState: MyState, args: MyArgs): MyViewModel {
            return MyViewModel(args, intialState)
        }
    }
}
hellohuanlin commented 5 years ago

Sometimes args is not enough to construct the state. My preference is to create the initial state inside view model. This decouples state and args, and will give us enough flexibility.

class MyViewModel(
  private val x: X, 
  private val y: Y,  
  private val foo: Foo
): BaseMvRxViewModel<MyState>() {
  override val initialState = MyState(
    x = x, 
    y = y, 
    foo = foo
  )

  companion object : MvRxVMFactory {
    override fun create(args: MyArgs) {
        val someDaggerComponent = ...
        return MyViewModel(args.x, args.y, someDaggerComponent.foo)
    }
  }
}

Or this

class MyViewModel(
  private val args: MyArgs, 
  private val foo: Foo
): BaseMvRxViewModel<MyState>() {
  override val initialState = MyState(
    x = args.x, 
    y = args.y, 
    foo = foo
  )

  companion object : MvRxVMFactory {
    override fun create(args: MyArgs) {
        val someDaggerComponent = ...
        return MyViewModel(args, someDaggerComponent.foo)
    }
  }
}

Another example is with generic state:

data class MyState<T>(
  val x: Foo,
  val t: T
)
class MySuperViewModel<T>(
  val x: Foo, 
  val initialT: T
) {
  override val initialState = MyState<T> {
    x = x,
    t = initialT
  }
}

class MySubViewModel(
  val x: Foo
): MySuperViewModel<String>(x = x, initialT = "Hello") {}

@BenSchwab @gpeal @elihart

Mishkun commented 5 years ago

@hellohuanlin You can instantiate state in the factory method of a ViewModel. This would be much more cleaner approach involving real DI instead of fake one. Real DI constructs everything needed to object A completely outside this object. Fake DI gives A some deps that A glues together Also you loose benefits of state management done by MvRx With suggested approach in initial proposal you can easily move initial state creation to the ViewModelFactory and still be compliant with all the framework features

hellohuanlin commented 5 years ago

@Mishkun yes moving it into ViewModel feels like a better approach.

bearprada commented 5 years ago

I got the same question when I want to reuse the same ViewModel with a different configuration in the same activity.

gpeal commented 5 years ago

@bearprada you can use keyFactory for that: by fragmentViewModel(keyFactory = { "foo" })

Every key will create a separate ViewModel.

bearprada commented 5 years ago

What if we have two lists on the same page(Fragment). One list shows category A's items, another list shows category B's items. We want to re-use the same ViewModel's implementation.

Right now, here is no way to inject two different Args objects in a Fragment, no?

Sorry, my question might not suitable for this thread, and it might not your original design.

gpeal commented 5 years ago

@bearprada You can make your fragment args contain as much information as you want because it's just a parcelable class. We recommend using a kotlin data class with @Parcelize. You can store the information you need for both view models there.

gpeal commented 5 years ago

@Mishkun @bearprada @hellohuanlin @BenSchwab Is https://github.com/airbnb/MvRx/pull/148 sufficient to resolve this?

BenSchwab commented 5 years ago

@gpeal I don't think so. I think the rationale here is that if your state needs to reference a property that is stored in something other than args (state in a dagger component for example), you can't put in it your state without making it artificially nullable. Even with fragmentFactory, the state will be created before the dagger component is easily accessible.

My thought of a pretty easy fix is to introduce an optional fun initialState(activity/fragment): S inside ViewModel Factories. Internally, we would take the result of this function, and then apply state restoration, before passing to createViewModel.

gpeal commented 5 years ago

@BenSchwab That could work actually!

BenSchwab commented 5 years ago

Played around a bunch, and couldn't find a clean fully backwards compatible solution. This is what I am leaning towards now:

interface MvRxVMFactory<S: MvRxState> {

    fun create(viewModelContext: ViewModelContext): BaseMvRxViewModel<S>? = null

    fun initialState(viewModelContext: ViewModelContext): S? = null

}

sealed class ViewModelContext {
    abstract val args: Any?
}

class ActivityViewModelContext(val activity: Activity, override val args: Any?) : ViewModelContext()

class FragmentViewModelContext(val activity: Activity, val fragment: Fragment, override val args: Any?) : ViewModelContext()

This approach has a few advantages: 1) One factory for both activity scoped and fragment scoped VMs. Currently, if you use a FragmentViewModelFactory but scope it to an activity, you will crash. The use of a sealed class makes acknowledging this possibility explicit. 2) Args are weakly typed in this model (which I think is fine, because inherently they are loosely typed). This keeps the model consistent with the constructor based state creation, which could have many constructors with different arg types. 3) Decouples args from state -- you can access args just for use in ViewModel 4) Allows DI of state objects by giving access to activity/fragment. 5) Nullable return types for state creation and view model creation allow clean fallbacks to default factories.

elihart commented 5 years ago

@BenSchwab that looks pretty good to me - the benefits you listed all seem great.

One thing - are you sure weakly typed args are a good idea? I think in most cases there will only be one type, and if somebody has a case for multiple args types they can use Any explicitly. I'm concerned that not providing strong typing would lead to more boilerplate, and making it harder to catch the error case where the wrong args type is passed.

BenSchwab commented 5 years ago

Good point @elihart. I like the idea of preferring typed args, but allowing Any as an escape. Only downside I see is it forces one more generic into the factory interface.