Discuss whenStarted function deprecation

[denis-bezrukov]

@gpeal @elihart @sav007
In Lifecycle 2.6.0 the functions whenXXX/launchWhenXXX were deprecated, see explanation here

Overall they outline four options for the work to behave when the lifecycle fall below "X" state:

1. The work is paused (current, but deprecated behavior of whenStarted)
2. Let the suspending code continue to run to completion (even if state falls below target state)
3. Cancel the suspending code and restart it when you come back above that state.
4. Cancel the suspending code and don't restart it

Initially, maverick's viewModel.onEach behaved as 2 - the callback was guaranteed to be triggered when lifecycle state is above state, but execution wasn't paused/cancelled if the state falls below started state.
e.g. the following was possible:

viewModel.onEach(State::property) { property -> 
    binding.textView.text = "A" // guaranteed state above Started
    delay(1000)
    binding.textView.text = "B" // no guarantee

In #665 the behavior was changed to 1, as the callback is wrapped into whenStarted

As it is deprecated now (in #689 deprecations were suppressed), eventually it might be removed/hidden so we need to figure out what would be the best expected behavior for mavericks.

Here are my thoughts

1. Behavior 3 & 4 do not make much sense as on higher level there is a DeliveryMode (UniqueOnly, RedeliverOnStart) which allow to configure similar behavior - RedeliveryOnStart=Behavior 3, UniqueOnly=Behavior 4
2. Behavior 1 (current, with whenStarted) only makes sense with UniqueOnly, as with RedeliverOnStart previous work will not be resumed (so it will be equivalent to Behavior 4). It will be cancelled because of new 'redeliver' emission
3. Behavior 2 (behavior prior 3.0.2) actually still makes sense. I was thinking some more about the example in Wrap subscription action with lifecycle whenStarted #665

override fun onCreate(savedInstanceState: Bundle?) {
  viewModel.onEach(ViewState::isVisible) { isVisible ->
    ...
    binding.someview.reveal()  // (1) suspendable, performs animation
    binding.someview.text = "" // (2) touches other UI after continuation
    ...
  }
}

Despite it fixes crash (if view's lifecycle fall into DESTROYED state, while fragment's lifecycle is still in CREATED), it introduce some potential issues.

• Issue 1 - the code won't be ever "resumed" from suspended state just because of RedeliverOnStart, so binding.someview.text = "" will never be called

• Issue 2 - even if it would resume (e.g. if UniqueOnly delivery mode was used), binding.someview.text = "" might reference to another view (if view was recreated).

I think the proper solution/advice would be to move viewModel.onEach that does "suspending stuff with a view" to Fragment.onViewCreated. In this case the job will be created in view's lifecycle scope, so the animation will be tied to view rather than fragment which is more correct

[sav007]

By would be to move viewModel.onEach that does "suspending stuff with a view" to Fragment.onViewCreated do you mean we should use view lifecycle scope rather than fragment (in resolveSubscription)?

[denis-bezrukov]

@sav007 yes, it is already work this way today (link)

fun onCreate(..) {
    viewModel.onEach(State::someProp) { someProp ->
          // launched in lifecycleScope
    }
}

fun onViewCreate(...) { // or onCreateView
    viewModel.onEach(State::someProp) { someProp ->
        // launched in viewLifecycleOwner.lifecycleScope
        binding.someView.reveal()
        binding.anotherView.fadeIn() // will never be called after view's destroy
    }
}

[sav007]

So the plan here is to revert wrapper that we put other day whenStarted and provide a recommendation to use onEach in onViewCreated callback as it will internally resolved to the view scope? In this case we will address the issue of using whenStarted deprecation but user can face the original issue with touching view in wrong lifecycle state.

I believed we already discuss this but maybe we can back to the drawing board again and think is there any other way to make it clear for user to not use onEach with suspendable action in onCreate? Options like new API, lint checks ?

[denis-bezrukov]

In this case we will address the issue of using whenStarted deprecation but user can face the original issue with touching view in wrong lifecycle state.

Yes

I believed we already discuss this but maybe we can back to the drawing board again and think is there any other way to make it clear for user to not use onEach with suspendable action in onCreate? Options like new API, lint checks ?

I think new API or lint check might be too intrusive as after the update you have to update hundreds of usages (or you will end up with multiple APIs like selectSubscribe vs onEach living side by side).

Since selectSubscribe/onEach were designed to be called only after onStart, that actually means it should be always ok (and more correct) to launch it in viewLifecycleScope (from onViewCreated) and never use onCreated. UniqueOnly shouldn't be an exception, since lastDeliveredStates are tracked separately.
So overall I see three options:

1. Add a suggestion to move that onEach to onViewCreated
2. Add a lint check to restrict onEach from onCreate
3. Resolve subscriptions registered in onCreate to view's lifecycle scope as well. It might be tricky as view's lifecycle doesn't exist in onCreate and most likely we have to deal with viewLifecycleOwnerLiveData/Flow to track it

[gpeal]

Thanks for your detailed thoughts, as always, @denis-bezrukov!

IIRC, I don't think viewLifecyleOwner existed when these APIs were first written. I think viewLifecycleOwner was introduced some time in 2018 in ~android.support:28.

With whatever we decide, the main use cases that I think about are:

1. Suspending view animations like the ones you presented which I think you've outlined well
2. Things that fire analytics events. We don't want analytics events to get dropped. However, my guess is that most of these will either not suspend or they are events that track an animation that suspends and if the animation didn't finish, maybe the event shouldn't fire anyway.

I also don't love that onEach's behavior implicitly changes depending on the availability of viewLifecycleOwner which was added in #533. While it does provide an option to do this correctly, it's pretty undiscoverable is one the (hopefully few) cases where Mavericks doesn't carefully guide you to do the right thing.

@denis-bezrukov your third option of always resolving to viewLifecycleOwner sounds interesting. While potentially complex, if we can take on the complexity in the library to make it easier and safer for everybody else, that seems worth it.

Are there cases you can think of where always using viewLifecycleOwner would break currently working behavior?

[elihart]

Thanks for the detailed write up.

At Airbnb we have always used the onViewCreated callback for any onEach registrations, and it has worked well for us.

I believe if there are any cases where somebody wants that callback to continue working even when the view is destroyed, then the behavior must be by definition unrelated to the view and therefore should be able to be done in the viewmodel instead anyway. The one exception I can think of is if the callback needs to access other viewmodels, especially since it isn't always easy or possible to access one viewmodel from another.

That should be a very rare case, but it is possibly something we still need to support.

I like the idea of using the view lifecycle owner livedata to guarantee that we only subscribe to the view lifecycle. It would be cleaner than trying to ensure via docs and lint that everyone does the right thing. However, it's not immediately clear to me how we would achieve that. I think we would also need to offer an escape hatch to customize the lifecycle scope if needed

[elihart]

From a brief look at how we might make that change, we might change MavericksView to have a subscriptionLifecycleOwnerFlow that is passed to the _internalX subscribe functions. we can create that flow from the view lifecycle owner live data, and when it is non null, or the value changes, we can resubscribe, or cancel it when the value becomes null.

It does look like handling all of the details correctly could be complex though to ensure the behavior is correct in all cases