Feature: Use type safe view states

[sebaslogen]

Why is this important?

To encourage the use of ViewStates that avoid illegal states (e.g. HomeViewState(userEmailTitle = "[email protected]", isLoading = true, showError = true) 🤯😕)

Notes

The MutableStateFlow in the repo needs to be a MutableSharedFlow, otherwise it won't emit new events when the same data is received from the backend, in this case we want to get updates and let the UI do the diffs.

[Frank1234]

I believe sealed ViewStates are inconvenient in usage. I tried it a few times, but always move away from them eventually, when not doing SDUI. Some of the reasons:

1. They presume you never load while showing content. (pull to refresh, for example, or partial loading).
2. They presume you never error (like a snackbar or any non fullscreen error) when showing old or partial content or loading.
3. It's inconvenient when you get content from different sources (multiple observables from the domain layer, callbacks from something, function calls from the view). You need to be able to create or copy the ViewState.Content (so support both: copy and create) in all locations where you want to adjust something in the ViewState's content.

Unless you have a fix for this, I believe with the old ViewState you are more flexible and have to write less code to change values in the content.

[sebaslogen]

Thanks for the feedback, I hadn't though on the perspective of SDUI vs non-SDUI.

My ideas through the reasons with your new perspective of non-SDUI apps:

1. This we solved for Hema and PostNL (SDUI), but I think the same solution applies for non-SDUI apps: the app defines clear ViewStates for Full-screen-loading vs content+loading, it would look like this:

sealed interface HomeViewState {
    data class Content(val isLoadingWithPTR: Boolean, val userEmailTitle: ViewStateString) : HomeViewState
    data object FullScreenLoader : HomeViewState
    data object Error : HomeViewState
}

The advantage here is that dev has to make an explicit decision about which type of loader (s)he wants to show, making the case of showing a PTR loader when there is no content impossible.

2. The same concept applies for full screen errors and transient errors on both Hema and PostNL:

sealed interface HomeViewState {
    data class Content(val transientError: MyErrorType?, val userEmailTitle: ViewStateString) : HomeViewState
    data object Loader : HomeViewState
    data object FullScreenError : HomeViewState
}

In this scenario, normally full-screen-errors are just a simple boolean, and transient-errors (like snackbars) can contain more complex logic like a message plus an action of a more complex type. So this distinction is an advantage.

3. For this one I wanna make sure I got point correctly, what I understand is:
   1. for example: the ViewState has some content with 3 items that are not favorite
   2. the user favorites element 1 on the UI layer and a call to the ViewModel is triggered to change the data model
   3. the inconvenience you mention is that in the VM the dev should write something like:

fun onFavoriteClicked(index: Int) {
  when(viewState) {
    is HomeViewState.Content -> {
      viewState.items[index].isFavorite = true
    }
    HomeViewState.Loading, HomeViewState.Error -> {} // no-op or throw IllegalStateException()
  }
}

instead of just

fun onFavoriteClicked(index: Int) {
  viewState.items?.get(index)?.isFavorite = true
}

Is this understanding correct? Please correct me if I misunderstood this point. Otherwise, this is my proposed solution:
Since we can only modify content once we already have content, we can start those functions with a check, either a soft one or a hard one:

fun onFavoriteClicked(index: Int) {
  if (viewState !is HomeViewState.Content ) return // or throw IllegalStateException()
  viewState.items[index].isFavorite = true
}

This has the advantage of making the code simple and readable but still able to catch bugs faster.

Let me know what you think of these solutions and if you think we can reach a more convenient and also safe solution.

[sebaslogen]

Besides the inconvenient usage points discussed above, my main concern to open this PR is to discourage the type of mutable state that mixes concerns (a single ViewState class with many fields) from becoming a pattern in the codebase.

My worry is that if a different developer takes the single class ViewState and organically starts adding state to the class, it will eventually lead to a complex mix of incongruent or invalid states.

Let me illustrate my fears, suppose a dev adds to the original ViewState a couple more fields of data:

data class HomeViewState(
  val isLoading: Boolean = false,
  val showError: Boolean = false,
  val userEmailTitle: ViewStateString? = null,
  val favoriteItems: List<ItemType>? = null,
  val showLoginButton: Boolean = false
)

When the ViewModel gets an authentication error, it should set viewState.showLoginButton = true, the other fields should also be cleared but what about the userEmailTitle should it stay for next login or should it be cleared? That is a good question for a PO, but managing the state permutations becomes very complex.

The systemic problem here is that we are mixing, error + loading + content + authentication states into one single class and for every new field in this class we should create a matrix of combinations between all the fields to find all the illegal states and write defensive code to proactively prevent each on of those cases.

The bugs that come from not proactively preventing each illegal state combination can be very tricky to reproduce and debug.

Bug example from the existing code in main branch for fetchUser:

    fun fetchUser() {
        viewModelScope.launch {

            _uiState.update { it.copy(showError = false, isLoading = true) }

            handleAction(
                action = fetchUserUseCase(),
                onError = { _uiState.update { it.copy(showError = true, isLoading = false) } },
                onSuccess = { _uiState.update { it.copy(isLoading = false) } },
            )
        }
    }

If fetchUser is called twice, it's possible that both calls set the state to loading, then the first one fails setting showError = true, isLoading = false and the second one succeeds but the content is not shown because the error is still visible, this is the code on main for success (the developer forgot to remove the error on success😱):

  onSuccess = { _uiState.update { it.copy(isLoading = false) } },

[ninovanhooff]

Pfew, This is a hard one to have an opinion about. I tried this approach once and regretted it.

But you make some good points. Let's discuss them during a android coffee or dev meet. I'd like to know what the other devs think.

[sebaslogen]

More input from an article explaining the gotchas of using both approaches and why sealed hierarchies are safer (including drawbacks and solutions)

https://proandroiddev.com/how-to-safely-update-state-in-your-kotlin-apps-bf51ccebe2ef