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

How to handle one-shot events / side effects? Example: backwards navigation #400

Open
sanogueralorenzo opened this issue May 17, 2020 · 17 comments

Comments

@sanogueralorenzo
Copy link
Contributor

sanogueralorenzo commented May 17, 2020

Hello, creating this issue to gather some feedback or better solutions on handling one-shot events / side effects.

TLDR: Under some circumstances where you need to allow backwards navigation I run into a scenario where you have to combine uniqueOnly() + an Async<Unit> to allow user backwards & forward navigation (sending the same event). The Async seems like a bad workaround.

uniqueOnly() is required so when the user presses back and goes to the previous screen we don't trigger the navigation event again, makes sense 👍

Async<Unit> + .execute() is used to force a change in the state (since it will trigger Loading & Success every time). Replacing Async with a normal variable will become a problem if the user decides to navigate back and try to navigate forward again since clicking the button will set the same value as before, not triggering anything on the UI side.

To make it easier imagine the following scenario:

  1. User sees a screen that only requires name input
    image

  2. Attempting to continue without a name provides an error (simple empty validation)
    image

  3. After name input & clicking next, navigate to the next screen

  4. User is able to press back to show this fragment again and update the name or just check it was right (clicking next again)
    image

Looking at the code side:

State:

data class NameState(
    val name: String? = null,
    val error: Int? = null,
    val complete: Async<Unit> = Uninitialized
) : MvRxState

ViewModel:

class NameViewModel(
    state: NameState,
    private val userManager: UserManager = UserManager()
) : BaseMvRxViewModel<NameState>(state) {

    fun onNameType(name: String) = setState {
        val error = if (name.isBlank()) this.error else null
        copy(name = name, error = error)
    }

    fun onNextClick() {
        withState {
            if (it.name.isNullOrBlank().not()) {
                userManager.username = it.name
                closeScreen()
            } else {
                setState { copy(error = R.string.onboarding_name_error) }
            }
        }
    }

    // 🤔 Using Rx with execute just to clear a previous possible Success to re-trigger navigation
    private fun closeScreen() = Completable.complete().execute { copy(complete = it) }

    // ... MvRxViewModelFactory
}

Fragment:

class NameFragment : ContainerFragment() {
    private val viewModel: NameViewModel by fragmentViewModel(NameViewModel::class)

    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        super.onViewCreated(view, savedInstanceState)
        viewModel.asyncSubscribe(NameState::complete, uniqueOnly(), onSuccess = {
            // Navigate forward to next fragment (with backstack)
        })
    }

    // Irrelevant controller code with callback + Invalidate that does controller.setData
}

How would you approach this scenario to allow the backward & then forward navigation?

Thank you

@elihart
Copy link
Contributor

elihart commented May 18, 2020

I might model this as:

data class NameState(
    val name: String? = null,
    val showError: Boolean
) : MvRxState

have the fragment check whether the input text is not empty when it gets the click and then navigate and tell the viewmodel to save the name or show an error (or hide the error on text input).

@sanogueralorenzo
Copy link
Contributor Author

sanogueralorenzo commented May 18, 2020

Thanks, that would work for this scenario but it moves some logic from the ViewModel to the Fragment (can't cover it in the VM unit test).

Given the above, is there an alternative approach that leaves the fragment as is right now?

I'm looking for an equivalent of SingleLiveEvent where it only gets triggered once per event (that part is covered thanks to the uniqueOnly()) but it also gets triggered when sending the same event multiple times (to allow backward and forward navigation in this case).

If you think it is something not available yet and it makes sense to have I can look into it and put a PR up.

@gpeal
Copy link
Collaborator

gpeal commented May 18, 2020

@sanogueralorenzo If you make your event class not a data class then every instantiation of it will be considered unique

@sanogueralorenzo
Copy link
Contributor Author

sanogueralorenzo commented May 18, 2020

Good point, I'm too used to data classes

That made me think something like the following

// Generic so in more complex scenarios a sealed class can be sent through
class SingleEvent<out T>(val data: T)

State:

data class NameState(
    val name: String? = null,
    val error: Int? = null,
    val navigation: SingleEvent<Unit>? = null
) : MvRxState

Fragment:

viewModel.selectSubscribe(NameState::navigation, uniqueOnly(), {
    if (it == null) return@selectSubscribe
    // do something
})

How do you see the above? The only thing is that you must remember that selectSubscribe will emit the initial null value, so something like a selectNonNullSubscribe or selectSingleEvent that ignores the initial value/nulls/Unitnitialized value would be nice

Update, tested the above and gives the following

java.lang.IllegalArgumentException: Impure reducer set on NameViewModel! navigation changed 
from com.example.SingleEvent@9a00191 to com.example.SingleEvent@c108f6. Ensure that your 
state properties properly implement hashCode.

@gpeal
Copy link
Collaborator

gpeal commented May 19, 2020

@sanogueralorenzo Another option is just to have Channel in your ViewModel that you close in onCleared(). It can be outside of MvRxState. In. your example, navigation wouldn't be accurate in state because if you use it to navigate forward then hit back then your state would be misleading which means it's not really state at all

@sanogueralorenzo
Copy link
Contributor Author

True, thanks for the suggestion. Another similar approach we tried out was having a SingleLiveData but it combined different concepts on the ViewModel and Fragment level which looked ugly.

One of the main thoughts behind having it in the state is to keep that simplicity but also easily test navigation/other one-shot events with MvRxLauncher (which I imagine you are not supposed to do that 😄 ).

The main drawbacks are that it completely goes against several concepts of MvRx state like:

  • State is no longer a representation of your screen because it also includes navigation
  • Possible solutions break the purity of the state (having something like a UUID to force treating it as a new state breaks on debug due to the purity checks)
  • Another possible solution where on fragment navigation consumption it resets the value would break the simplicity of MvRx concepts

So as I see it right now the alternatives are:

  • Take some logic from the ViewModel to the Fragment, validation can still be unit tested but the main drawback is that the Fragment goes from just observe this and do X to do some checks, call the viewmodel and also navigate (only for cases where there is no threading involved)
  • Use Async with uniqueOnly() like in the example above
  • Include alternatives into the viewmodel like Channels or Rx equivalent, adding some equivalent to .execute that autodisposes for you in your base class
  • Add something similar to Async that only has Uninitialized and Success + a singleEventSubscribe() for the Fragment

@gpeal
Copy link
Collaborator

gpeal commented May 19, 2020

@sanogueralorenzo A Channel would do almost exactly that and the semantics of using it vs Mavericks 2.0 would be nearly identical

@gpeal
Copy link
Collaborator

gpeal commented May 19, 2020

Maybe we could expose our flow operator to flow when started or wrap it with a Channel to make it feel and behave even more similarly.

@xudshen
Copy link

xudshen commented Jun 15, 2020

@sanogueralorenzo I'm also dealing with this scenario and introducing a Trigger to handle one-shot events.

data class Trigger internal constructor(private val value: Int) {
    internal var metadata: Any? = null

    private inline fun map(f: (Int) -> Int): Trigger = Trigger(f(extract()))

    private fun extract(): Int = value

    fun activate() = activate { true }

    fun activate(predicate: () -> Boolean) =
        if (predicate()) this.map { it + 1 } else this

    fun deactivate() = this.map { 0 }

    fun <R> fold(ifInactive: () -> R, ifActive: () -> R): R =
        if (this.extract() <= 0) ifInactive() else ifActive()

    companion object {
        fun create(): Trigger = Trigger(0)

        /**
         * Helper to set metadata on a Trigger instance.
         */
        fun Trigger.setMetadata(metadata: Any) {
            this.metadata = metadata
        }

        /**
         * Helper to get metadata on a Trigger instance.
         */
        @Suppress("UNCHECKED_CAST")
        fun <T> Trigger.getMetadata(): T? = this.metadata as T?
    }
}

State:

data class NameState(
    val name: String? = null,
    val error: Int? = null,
    val navigateTrigger: Trigger = Trigger.create()
) : MvRxState

VM:

fun onNextClick() {
    withState {
        if (it.name.isNullOrBlank().not()) {
            userManager.username = it.name
            setState { copy(navigateTrigger = navigateTrigger.activate()) }
        } else {
            setState { copy(error = R.string.onboarding_name_error) }
        }
    }
}

Fragment:

viewModel.selectSubscribe(NameState::navigateTrigger, uniqueOnly()) {
    it.fold({
        // handle inactive state
    }, {
        // Navigate forward to next fragment (with backstack)
    })
})

@erikhuizinga
Copy link
Contributor

erikhuizinga commented Jun 4, 2021

@gpeal a question about the Channel for side effects approach: how can you guarantee the order of state and event operations is correct? E.g. in tests you might want to assert that your VM emits

  • state 1 (init)
  • state 2 (loading)
  • (error happens) event 1 (error message)
  • state 3 (error state or previous state)

(The examples in parentheses are just... examples)

The point is that the order must be testable and therefore guaranteed. However, because Mavericks processes states mostly on background threads, if my VM has a Channel received as a Flow from the view's coroutine scope, I'm not sure if I can be certain about the order. That is because I have to launch a collector of the event flow. That launched coroutine likely runs concurrently with Mavericks' state emissions, doesn't it? If so, then that would break order guarantees.

@gpeal
Copy link
Collaborator

gpeal commented Jun 4, 2021

@erikhuizinga Each ViewModel has a single reducer thread and its stateFlow will always emit items in the same order. You can also use the MavericksTestRule to make it synchronous for tests.

@erikhuizinga
Copy link
Contributor

@gpeal thank you for your reply. I see that the state store uses its own thread pool to concurrently invoke state updates. Is it possible for a state update to be something like:

// In my VM
private val eventChannel = Channel(BUFFERED)
val events = eventChannel.receiveAsFlow()

// In some function
setState {
    eventChannel.trySend { myEvent }
    myState
}

Is the order of states and events guaranteed to be myEvent, myState? I think not, because they are exposed through 2 separate flows. Even though the emitting side synchronously emits elements into these flows, their collectors might be slow and receive the state before the event, right? Is that why Mavericks models events through the onEach and onAsync APIs with uniqueOnly(), so that there really is one stream only? Of course, if the order of states and event wouldn't matter, then the separate channel approach is likely ok.

(I'm asking because I'm investigating whether or not to use Mavericks. I have no experience with it yet.)

@gpeal
Copy link
Collaborator

gpeal commented Jun 5, 2021

@erikhuizinga I think it's hard to understand the exact question without a more complete example of what you're trying to do.

@erikhuizinga
Copy link
Contributor

@gpeal
There are different use cases and I'd like to know if and how they're covered:

  1. It should be possible to buffer message events (one time messages), if multiple messages can be emitted from lower layers asynchronously and they must all show once. However, there might be no observers (yet) while more messages are being produced. How could that buffer be modelled in a MavericksState class, or in the VM, so that no messages are lost? The Channel(BUFFERED) approach works, but is async with the state flow. For example, 3 data layers being fetched concurrently, yielding errors one by one: the corresponding views should maybe change from loading to empty, but synchronously an error message should show for each. Error messages should not show while the UI still shows a loading state. No error messages should be lost. There might not be room for actual error message views, so snackbars are used for this.
  2. Navigation events should, unlike messages, likely not be buffered: only the latest navigation event is the true destination. I think a property that is observed with a uniqueOnly() strategy would work well here? The alternative is a Channel(CONFLATED), but that is again async with the state flow.
  3. What about the behaviour on forward-and-back navigation? I think that's the original question in this issue. Can the user navigate forward again? That is basically the same user action leading to the same state and navigation event, which is not trivial to model in Mavericks (see the various examples in the comments).

I understand that the original question is about putting one-shot event behaviour in the view state and that might not be a good place in the Mavericks paradigm. However, it is reality: a view is not only a pure function of state, but it behaves more dynamically: it goes through lifecycle changes and it has the ability to consume message and navigation events. That's why some developers want to put that consumable behaviour inside the view state too. It makes it easy to test and reproduce, and it makes views simpler as they almost trivially consume the states and events.

That also means that view state and view behaviour should stay in sync. If not in sync, then the view state might not yet be up to date while an event is consumed, possibly degrading UX.

@gpeal
Copy link
Collaborator

gpeal commented Jun 6, 2021

  1. This is an ideal use for derived properties like:
data class MyState(
    val prop1: Async<Foo> = Uninitialized,
    val prop2: Async<Foo> = Uninitialized,
    val prop3: Async<Foo> = Uninitialized,
) : MavericksState {
    val isLoading = prop1 is Loading || prop2 is Loading || prop3 is Loading
    val showError = !isLoading && (prop1 is Fail || prop2 is Fail || prop3 is Fail)
}
  1. Yes, navigation via an onEach + uniqueOnly() is the recommended approach for navigation
  2. There are a few options for navigation such as clearing the prop once you "consume" it via another ViewModel function and reducer. You could also make your own Navigation channel property on your ViewModel. There are no limitations in Mavericks for adding your own properties to your ViewModel. If you are concerned about your channel getting ahead of your navigation event, you can call awaitState() before emitting an item to your navigation channel and it will suspend until all pending reducers are run.

@erikhuizinga
Copy link
Contributor

Thank you very much.
(Your comment is somewhat difficult to read on mobile, it appears that answers 2 and 3 are in a code block, and not wrapping around. Please consider editing it.)

@sanogueralorenzo
Copy link
Contributor Author

Looking back at this my example was pretty poor since it should have been an Async property.

Even then, I think the main problem / confusion / request comes from the fact that we probably come from MVP or MVVM with something like SingleLiveEvent where we are used to move this logic to the view model instead of keeping it simple.

From my experience moving it to the ViewModel to have it unit tested made redundant tests (when a navigation derived property is easier to unit test for ex.) and the other drawback is that it created a bit of a bounce effect between ViewModel and Fragment which we saw with MVP or SingleLiveEvents

Maybe it would be a good idea to collect here examples of simple requirements that would cause a navigation / side effect and then we can create a How-to documentation so people are aware of different options before trying to do something like they would do with MVVM (which was my personal experience on my previous company)

Requirements could be (navigation side effects examples):

  • Click button, make API call and navigate on success
  • Click button and navigate to A B or C depending on the current state of the screen
  • Click button and show a toast but only the first time you are navigating forward

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

5 participants