airbnb / mavericks

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

How to get all states when invoke function in the init? #149

Closed littleGnAl closed 5 years ago

littleGnAl commented 5 years ago

Hello, I encounter a problem when I write the unit test, I judge the state to determine if the test case passed:

assert(stateList.size == 3 && 
    stateList[1] == loadingState && 
    stateList[2] == firstPageState)

But when I invoke the function in MvRxViewModel init I can't get all the states from the MvRxViewModel. Although I can use the selectSubscribe function to get the state, this function will only get the last state after I create the MvRxViewModel. The reason why I invoke the function in init is that there are some functions which we only need to invoke once for one page, such as the initial function, so I don't invoke the initial function in the onCreate of the Activity/Fragment, when the screen is rotated, we don't need to invoke the initialization function again but just render the UI by using the last MvRxState which MvRx will handle for us, and there is similar usage in the Wiki.

My current solution is to expose the MvRxStateStore for each MvRxViewModel (more detail here MainViewModel.kt):

abstract class MvRxViewModel<S : MvRxState>(
  initialState: S,
  stateStore: MvRxStateStore<S>
) : BaseMvRxViewModel<S>(initialState, BuildConfig.DEBUG, stateStore)

class MainViewModel @AssistedInject constructor(
  @Assisted initialState: MainState,
  @Assisted stateStore: MvRxStateStore<MainState>,
  private val accountingDao: AccountingDao,
  private val applicationContext: Context
) : MvRxViewModel<MainState>(initialState, stateStore) {

    init {
      loadList(lastDate = initialState.lastDate)
    }

    ...
}

So I can wrap the RealMvRxStateStore to get all the states for testing:

class TestMvRxStateStore<S : MvRxState> private constructor(
  private val realMvRxStateStore: RealMvRxStateStore<S>
) : MvRxStateStore<S> {

  companion object {
    fun <S : MvRxState> create(
      initialState: S
    ): TestMvRxStateStore<S> = TestMvRxStateStore(RealMvRxStateStore(initialState))
  }

  override val observable: Observable<S>
    get() = realMvRxStateStore.observable

  override val state: S
    get() = realMvRxStateStore.state

  override fun dispose() {
    realMvRxStateStore.dispose()
  }

  override fun get(block: (S) -> Unit) {
    realMvRxStateStore.get(block)
  }

  override fun isDisposed(): Boolean = realMvRxStateStore.isDisposed

  override fun set(stateReducer: S.() -> S) {
    realMvRxStateStore.set(stateReducer)
  }

  private val _allStates = mutableListOf<S>()

  init {
    realMvRxStateStore.observable.subscribe {
      _allStates.add(it)
    }
  }

  fun testAllStates(a: (List<S>) -> Boolean) {
    assert(a(_allStates))
  }
}

And my test case will be like this (more detail here MainViewModelTest):

class MainViewModelTest {

    @Test
    fun testLoadList() {
        testStateStore = TestMvRxStateStore.create(initialState)
        mainViewModel = MainViewModel(
            initialState,
            testStateStore,
            accountingDao,
            applicationContext)

        testStateStore.testAllStates { stateList ->
            stateList.size == 3 && 
                stateList[1] == loadingState && 
                stateList[2] == firstPageState
        }
    }
}

But In this way, I need to pass the RealMvRxStateStore to each MvRxViewModel.

Any suggestions for this? How you guys test this case? Thanks.

gpeal commented 5 years ago

@littleGnAl What is the purpose of testing the intermediate state prior to the completion of the ViewModel init? That will never get dispatched to your UI or subscribers anyway. I would recommend just making sure that the state is correct after init.

littleGnAl commented 5 years ago

Hello, @gpeal thanks for your response. Because I want to make sure every state is correct include the the state indicate the loading, but not just the final state.

gpeal commented 5 years ago

@littleGnAl If it's really critical, you could create an initializeIfNeeded function that you call from any fragments that use the viewmodel in onCreate and that function could check if it is already initialized. Then in your test you could add your subscriber before calling it.

littleGnAl commented 5 years ago

@gpeal Thanks for your suggestion, do you mean the code like this:

class CustomViewModel(...) : MvRxViewModel<...>(initialState) {
  private var isInitialized = false

  fun initializeIfNeeded() {
    if (isInitialized) return
    ...
    isInitialized = true
  }
}

class CustomFragment : BaseFragment() {

  private val viewModel by fragmentViewModel(...)

  override fun onCreate(savedInstanceState: Bundle?) {
    super.onCreate(savedInstanceState)
    viewModel.initializeIfNeeded()
  }
}
gpeal commented 5 years ago

@littleGnAl No, you want to use some property on your state to determine that because MvRx can restore your state in a new process with @PersistState

You might want to do something like:

data class MyState(
    val someProp: Async<String>,
    ...
) : MvRxState

class MyViewModel(state: MyState) : MvRxViewModel<MyState>(state) {

    fun initializeIfNeeded() = withState { state ->
        if (state.someProp !is Uninitialized) return@withState
        // initialize
    }
}

where someProp is initialized to something in your initialize function.

gpeal commented 5 years ago

@littleGnAl In general, never store state as properties in your ViewModel. They should be totally stateless. If this resolves your issue, can you close it?

littleGnAl commented 5 years ago

@gpeal Sorry for so late to respond, thanks for your good suggestion, but there's a case that I don't really like using Async<*>, such the code below:

data class SummaryMvRxViewState(
  val isLoading: Boolean = false,
  val summaryChartData: SummaryChartData = SummaryChartData(),
  val summaryItemList: List<SummaryListItem> = emptyList(),
  val error: Throwable? = null
) : MvRxState

  fun loadSummary() {
    Observable.zip(
        getSummaryChartData(),
        getSummaryItemList(),
        BiFunction { summaryChartData: SummaryChartData, list: List<SummaryListItem> ->
          summaryChartData to list
        })
        .execute {
          when (it) {
            is Fail -> {
              copy(isLoading = false, error = it.error)
            }
            is Success -> {
              val pair = it()!!
              copy(
                  isLoading = false,
                  summaryChartData = pair.first,
                  summaryItemList = pair.second)
            }
            is Loading -> {
              copy(isLoading = true)
            }
            else -> {
              copy()
            }
          }
        }
  }

In this case if I use Async I need to define the state like:

data class SummaryMvRxViewState(
  val summaryPair: Async<Pair<SummaryChartData, List<SummaryListItem>>> = Uninitialized
) : MvRxState

Which make the state so unclear, although I can create a new class to host these data, but it seems a little redundant.

Btw, maybe it's inappropriate to discuss this in this issue.

gpeal commented 5 years ago

@littleGnAl What I would do (without knowing anything else about your app) is{

data class SummaryMvRxViewState(
  val summaryChartData: Async<SummaryChartData> = Uninitialized,
  val summaryItemList: Async<List<SummaryListItem>> = Uninitialized
) : MvRxState {
    val isLoading: Boolean = summaryChartData is Loading || summaryItemList is Loading
    var error: Throwable? = (summaryChartData as? Fail)?.error ?: (summaryItemList as? Fail)?.error
}

Then you can fetch them independently without your complicated zip which also has the effect of failing both stream if either fails.

littleGnAl commented 5 years ago

@gpeal Thank you so much, I will think more about this.