Ok so I think the answer to this is yes but is it unsafe to have a `ViewModel` collect from a `Flow&...

# coroutines

[dewildte]

Ok so I think the answer to this is yes but is it unsafe to have a ViewModel collect from a Flow<T> given to it by a View , Activity or Fragment ?
I.E.

// MyView.kt
val events: Flow<Event> = flowOf(Event1, Event2)

viewModel.handleEvents(events)

[Adam Powell]

A shape like that can make it very easy for a caller to do the wrong thing. The goal is to avoid holding long-lived references back to the Activity object and prevent it from being collected, or operating on an Activity or Fragment outside of safe lifecycle states. As views, activities and fragments almost always have shorter lifetimes than viewmodels you run this risk.

[Adam Powell]

generally it's better to collect a flow in a narrower scope from the UI and call into the longer-lived viewmodel as part of a collect block if you need information to flow in that direction.

[ursus]

Youd be most likely leaking the view/activity instance. Not really a flow issue, just lifecycles in general. Youd have to make sure to disconnect that flow when view goes away, granted its lifecycle is narrower that viewmodel's

[Hitender Pannu]

Are you asking in general or is there any specific useCase for which you are thinking in this direction

[dewildte]

This was the useCase:

// MyViewController.kt
val _events = MutableSharedFlow<Event>()
val events: SharedFlow<Event> = _events.asSharedFlow()
...
viewModel.handleEvents(events)

I thought it was totally wrong due to the aforementioned problems.
That said I was curious if my intuition was wrong, hence I asked my question.

[dewildte]

I wanted to further decouple my UI code from the ViewModels and have settled on this solution:

// ToDoUIState.kt
data class ToDoUIState(
  val title: String = "",
  ...
)

// ToDoUIEvent.kt
sealed class ToDoUIEvent {
  object CompletedToggle : ToDoUIEvent()
}

// EventHandler.kt
fun interface EventHandler<EVENT> {
  fun handleEvent(event: EVENT)
}

// ToDoUI.kt
@Composable
fun ToDoUI(stateFlow: Flow<ToDoUIState> = emptyFlow(), eventHandler: EventHandler<ToDoUIEvent>? = null) {
  val state = stateFlow.collectAsState(initialState = ToDoUIState())
 ...
 eventHandler?.handleEvent(ToDoUIEvent.CompletedToggle)
}

[dewildte]

This makes making previews for my UI components very easy.
And both the UI and the ViewModel do not have to know about each other.
They both only know about state and events.

[dewildte]

This is a little more code and verbosity but I wanted the decoupling it provides.

[ursus]

if you collect the stream inside view model scope it then has reference to it, which is then transitively acitivty, and if your viewmodel survived orientation change but view doesnt -> leak

[ursus]

I thought about this again. I dont think it leaks (the view). Yes you hold onto the flow instance longer than needed and nobody will ever emit into it again, but I believe you only have the flow reference, not the view. Flow instance doesnt hold any View reference so it should be ok

Youll only accumulate basically dead flow instances in the viewmodel, which is not great, but it's not activity sized leak, and will get cleared when view model dies

It is true that operators have their upstream references, but the builder operator doesnt necessarily need to have ref to its enclosing class (flowOf())

but it might (flow { ... })

so as Adam said, its easy to footgun with this shape

[dewildte]

A function call is safe.

[Adam Powell]

avoid doing this in a @Composable function: eventHandler?.handleEvent(ToDoUIEvent.CompletedToggle) unless it's inside an Effect block

[Adam Powell]

and avoid sending events as a side effect of recomposition entirely, with or without Effect functions.

[dewildte]

Yeah I am thinking about adding the event handler to the Snapshot like so:

data class ToDoUIState(
  val title: String = "Buy Milk",
  val onClick: () -> Unit = {}
)

[dewildte]

// ToDoUIState.kt
data class ToDoUIState(
  val title: String = "Buy Milk",
  val onClick: () -> Unit,
  ...
)

// ToDoUI.kt
@Composable
fun ToDoUI(stateFlow: StateFlow<ToDoUIState>) {
  val state = stateFlow.collectAsState()
 ...
 state.onClick()
}

[dewildte]

@Adam Powell Anything bad about this ☝️

[Adam Powell]

still bad if recomposition calls onClick as opposed to something like an actual callback handler, e.g. Button(onClick = { state.onClick() })

[dewildte]

The onClick() will be called by buttons only.

[dewildte]

So this is fine then, thanks!