Ok so I think the answer to this is yes but is it unsafe to

Question

Ok so I think the answer to this is yes but is it unsafe to have a `ViewModel` collect from a `Flow lt T gt ` given to it by a `View` `Activity` or `Fragment` I E ``` MyView kt val events Flow lt Event gt = flowOf Event1 Event2 viewModel handleEvents events ```

Adam Powell · Answer

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 · Answer

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 · Answer

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 · Answer

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

dewildte · Answer

This was the useCase ``` MyViewController kt val events = MutableSharedFlow lt Event gt val events SharedFlow lt Event gt = 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 · Answer

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 lt EVENT gt fun handleEvent event EVENT ToDoUI kt Composable fun ToDoUI stateFlow Flow lt ToDoUIState gt = emptyFlow eventHandler EventHandler lt ToDoUIEvent gt = null val state = stateFlow collectAsState initialState = ToDoUIState eventHandler handleEvent ToDoUIEvent CompletedToggle ```

dewildte · Answer

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 · Answer

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

ursus · Answer

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 gt leak

ursus · Answer

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 · Answer

A function call is safe

Adam Powell · Answer

avoid doing this in a ` Composable` function `eventHandler handleEvent ToDoUIEvent CompletedToggle ` unless it s inside an `Effect` block

Adam Powell · Answer

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

dewildte · Answer

Yeah I am thinking about adding the event handler to the Snapshot like so ```data class ToDoUIState val title String = Buy Milk val onClick gt Unit = ```

dewildte · Answer

``` ToDoUIState kt data class ToDoUIState val title String = Buy Milk val onClick gt Unit ToDoUI kt Composable fun ToDoUI stateFlow StateFlow lt ToDoUIState gt val state = stateFlow collectAsState state onClick ```

dewildte · Answer

< Adam Powell> Anything bad about this point up

Adam Powell · Answer

still bad if recomposition calls `onClick` as opposed to something like an actual callback handler e g `Button onClick = state onClick `

dewildte · Answer

The onClick will be called by buttons only

dewildte · Answer

So this is fine then thanks