After reading the source code and some texts here ...

# compose

[zoha131]

After reading the source code and some texts here I think people are saying that DisposableEffect with empty onDispose is a code smell. And we should avoid that.

I want to load an article from server after user open the detail page. What should be the recommended way to load the data?

Previously I was using onActive to load the data. ViewModel emits StateFlow which the view collect as state.

// pre- Alpha-11
    onActive { viewModel.loadData(articleID) }

    // post - Alpha-11
    DisposableEffect(articleID){
        viewModel.loadData(articleID)
        onDispose {  }
    }

[Zach Klippenstein (he/him) [MOD]]

It’s a code smell here because if your composable gets skipped while the load is in progress, the load should be cancelled

[zoha131]

I am loading data for the whole screen here. And I think if user change the screen then the viewModel will be cancelled therefor the coroutine will also be cancelled.

[allan.conda]

I would load it on init of viewmodel

[allan.conda]

keep your views passive

[zoha131]

that’s a good idea. Thanks @allan.conda

[Dominaezzz]

Would not recommend having side effects in init of viewmodel, especially if you plan to write any tests.

[allan.conda]

but it’s not a side effect, that’s the main business logic. I see no problem in testing for this particular case

[Marko Novakovic]

just so you know you have SideEffect {} for running code when composable is composed/recomposed. it may not be the best idea to call viewModel.loadData() from there but you can use SideEffect for some other things. DisposableEffect is used when you have resources to clear aka dispose. DisposableEffect with empty onDispose() {} is just a SideEffect

[zoha131]

TBH I am kinda lost here. Earlier I was using Fragment and onResume then I moved from fragment to composables for screens and was using onActive to load data for that screen using ViewModel. And any ongoing work will be stopped if the ViewModel gets cleared.

Now that onActive is gone I have to use DisposableEffect but having empty onDispose in almost all the screens feel wired and also a code smell.

Have I done anything wrong? or I need to think differently? I am thinking compose as a screen not a view.

[allan.conda]

in our architecture the only things exposed by the viewmodels is the state and an interface for user events (onClick, etc).
in your case your View is telling viewModel what to do, when it’s only supposed to observe

[zoha131]

let’s say user navigates to Article Detail Screen. Then what would be optimal place to load that article from server?

It may take some time. So loading article inside list screen would not be a good UX.

[allan.conda]

load it from that screen’s viewmodel

[zoha131]

I am loading it from the ViewModel. I can’t do this inside the init block as I need articleID to load the article. How can I pass the articleID to the ViewModel and ask for loading?

[allan.conda]

pass it from your viewmodel factory so you can access whatever your viewmodel needs to load data

[zoha131]

can you please direct me to any article/documentation to learn how to pass dependency from composable to ViewModel? I am using Hilt.

[allan.conda]

check this one out
https://proandroiddev.com/whats-new-in-hilt-and-dagger-2-31-c46b7abbc64a

[Marko Novakovic]

@zoha131 SideEffect is DisposableEffect with no onDispose so when you need something like DisposableEffect and you don’t have use for onDispose just use SideEffect

[Marko Novakovic]

@zoha131 SideEffect is executed on every composition and recomposition

[Marko Novakovic]

@zoha131 you should build ViewModel with factory and pass id into it via factory, that is the best approach

[Marko Novakovic]

@zoha131 composable functions can host ViewModel just like Fragment of Activity so when you call viewModel() you will get ViewModel that is tied to a composable. that allows you to call code related to a composable inside ViewModel.init block and be sure it’s cleared if composable is removed from the tree

[zoha131]

@Marko Novakovic I will use ViewModel with AssistedInject as described by @allan.conda. Thank you all for helping me with this. 😇

[Marko Novakovic]

@zoha131 that is the best approach. I wanted to explain SideEffect and DisposableEffect for future use. these two are important and have their uses. good luck

[Zach Klippenstein (he/him) [MOD]]

I think you’re right, DisposableEffect is the right tool for the job here. And you’re also right about the empty onDispose being a smell. The reason it’s a smell is because DisposableEffect is meant to deal with resources or tasks that are tied to the lifecycle of the composition. Resources/tasks which are allocated/initialized/started when the composable is first composed, and which are disposed when the composition is skipped, since they’re no longer needed by the composition.

This is also exactly what you’re describing – you’re starting a long-running task (from the perspective of compose, “long running” being something that could take longer than a single composition) that is tied to the lifecycle of the composition. The task is because the composable is being displayed, so presumably it should be stopped if the composable is no longer displayed.

Compose doesn’t know that the task will be cancelled because the ViewModel is cancelled – nor should it, that’s a concern in a different layer than the view layer. In fact, previous threads in this channel about similar code have proposed that the composable shouldn’t be calling loadData directly at all – they should simply be telling the view model that the screen has started being displayed (and in the onDispose case, that it has stopped being displayed) – and it’s the view model’s responsibility to interpret the onScreenDisplayed event to mean it should start loading some data. The reasoning is that the view layer shouldn’t be concerned about starting/stopping the loading of data or other business logic, it should only contain view logic. It’s the responsibility of the ViewModel to communicate between the view layer and the other layers of your app.

So for your case, I would suggest something like this:

Untitled.kt

[Cyril Find]

why not using LaunchedEffect(articleID) { ... } here ?

[zoha131]

@Cyril Find Actually I was looking for the recommended way here. LaunchedEffect(articleID) { ... } and DisposableEffect(articleID) { … } with empty onDispose both works here. But these are not recommended. At least this is what I understood after reading all the messages here.

[Cyril Find]

I don't think LaunchedEffect(articleID) { ... } is not recommended ?
I'm kinda confused by the exchange too 😅 that's why I was asking this

[Dominaezzz]

You can use remember and SideEffect.