android / architecture-components-samples

Samples for Android Architecture Components.
https://d.android.com/arch
Apache License 2.0
23.36k stars 8.28k forks source link

Caution: ViewModels shouldn't observe LiveData objects! #301

Open ech0s7r opened 6 years ago

ech0s7r commented 6 years ago

As documented here: https://developer.android.com/topic/libraries/architecture/viewmodel.html

[...] However ViewModel objects must never observe changes to lifecycle-aware observables, such as LiveData objects. [...]

In this ViewModel that should be used as sample, you are doing it. https://github.com/googlesamples/android-architecture-components/blob/74b2afe88fd79c37361120e3cb454561687a853d/GithubBrowserSample/app/src/main/java/com/android/example/github/ui/search/SearchViewModel.java#L145

herriojr commented 6 years ago

Not entirely true, see one of my feature requests: https://issuetracker.google.com/issues/73251351

What's important is you don't observe on a LiveData object from something bound to a different lifecycle. For instance, if you have something Fragment scoped and then something activity scoped. If the activity scoped one binds to a livedata object from the fragment one, the fragment's ViewModel will still be referenced even though it is no longer managed.

It is OK to observe on data from the repository w/ observeForever as this isn't bound to a lifecycle. It observes until told otherwise. The part that you have to handle in that case is to make sure you remove it in onCleared(), which they don't appear to be doing. There's a possibility in this case to leak the ViewModel longer than it should be around since they don't reset() the handler in onCleared().

asantibanez commented 5 years ago

Thanks @herriojr for suggesting using observeForever. Was just questioning if ViewModels could observe LiveData objects and if so, how.

filipkowicz commented 5 years ago

use LiveDataMediator -> almost all usecases allow you to that and with this approach you can bypass livecycle aware observator -> just remember that mutating method needs to be called on main thread (you can switch later to background thread anyway)

observeForever could cause leaks if you dont remove observer inonCleared` as @herriojr said

stdpmk commented 4 years ago

@filipkowicz , That does not work if MediatorLiveData is not observed by Activity or Fragment. Related post https://stackoverflow.com/questions/47515997/observing-livedata-from-viewmodel

filipkowicz commented 4 years ago

@stdpmk yes you are right but why observe it then anyway. As far as I understand viewmodel should prepare data for view. If there is no view, stopping livedata observation is good thing

christyjacob4 commented 3 years ago

@herriojr

I have a use case where I have a couple of filters in my fragment represented by checkboxes. So in the onCheckedChange listener, Im adding the currently selected filter to a LiveData<MutableSet<String>> in the ViewModel like so

viewModel.filters.value?.add(currentlySelectedFilter)

I would like that, when this LiveData object is modified, the ViewModel would automatically filter through a main list (that contains all the data without any filters) . So would it be considered a good practice to add an observer to filters like this?

filters.observeForever { 
     onFilterChanged()
}
herriojr commented 3 years ago

@christyjacob4 I haven't been doing a lot of Android development recently, so hopefully my memory on kotlin/mvvm still holds true.

It sounds like you are basically filtering data based on some input by the user. This seems like a fairly common use-case.

First off, my suggestion is to not modify a filters LiveData object directly from the fragment or activity. Your UI should be passing events that are happening on the UI to the ViewModel. In your case, it would be some method of saying that one of the filters were clicked. When talking to the ViewModel from the fragment/activity you shouldn't be accessing the LiveData directly except to listen. Also, chances are if you are accessing the filters LiveData and modifying it, you are exposing a MutableLiveData. Never expose the LiveData as mutable to the activity/fragment. The purpose of the ViewModel/LiveData is to move all your logic outside of the fragment/activity. Exposing MutableLiveData to the activity/fragment allows people to move logic outside of the ViewModel, which you don't want.

so if we move this to a function on the View like so:

class ViewModel: ViewModel() {
    private List<String> filters = emptyList()

    private var query: LiveData<List<Data>> = MutableLiveData(emptyList())
    private val source: MediatorLiveData<List<Data>> = MediatorLiveData()

    val data: LiveData<List<Data>> = source

    init {
        requery(emptyList())
    }

    fun requery(filters: List<String>) {
        source.removeSource(query)
        query = repository.query(filters)
        source.addSource(query, value-> source.setValue(value))
    }

    fun onFilterEnabled(String filter) {
        // maybe do some analytics
        filters.remove(filter)
        // also requery
        requery(filters)
   }
}

Note: This may not compile or anything. It's just an example on a loose memory I have from years ago.

christyjacob4 commented 3 years ago

@herriojr thank you so much. This is very helpful