This is bad right? ```@Composable fun SomeScreen(...

# compose

[Colton Idle]

This is bad right?

@Composable
fun SomeScreen(
    viewModel: SomeScreenViewModel = hiltViewModel(),
    someIdINeed: String = ""
) {
    viewModel.init(someIdINeed)
    MyActualScreenContent()
}

The viewModel.init(id) should go inside of a remember{}? Or is it better to do LaunchedEffect(Unit)?

edit: I almost wonder how viable it'd be to create a lint rule or something that would flag this sort of stuff in a composable. The above code was in our codebase for like 3 months before i noticed it just now.

[Adam Powell]

what would that lint check detect?

[Adam Powell]

calling Unit-returning non-@Composable functions on a parameter object in a composable or something?

[Colton Idle]

The lint was more of aside, but yeah theorhetically some sort of "strict mode" that could help find possible violations. Probably get a good amount of false positives.

In my case, I just didn't noticed that we were calling viewModel.something() and like I alluded to in the question. I don't know if that's necessarily wrong, but I'm fairly sure it is. 😄

[hfhbd]

Regarding viewModel init: What about using a custom ViewModel factory which remembers the created viewModel instance?

[theapache64]

I think the problem here is not about remembering the viewModel. Its about potential recomposition causing multiple calls to init method.
@Colton Idle You can inject the params to the viewModel through a Bundle and then access it using a SavedStateHandle in the ViewModel? if you’re using navigation compose, this is already there.

[Colton Idle]

I guess theres two things I'm hinting at here.

1. Is there a better way to inject values like this into a viewModel yet? What I think is im looking for assisted injection but thats not quite there yet?
2. Lets pretend that init() was just called start() or something and it didn't take any variables or anything. How would I make sure that it's only called once? Both remember{} and launchedEffect{} seem right.

[theapache64]

Yeah,in that case LaunchedEffect would be more suitable here IMO. I think you can use a boolean flag or lateinit var inside viewModel to prevent multiple calls when navigated back.

fun start(){
    if(isInitialized) return
    // ...
}

what do you think ?

[Colton Idle]

Yeah, I think that should do it. 👍

[Adam Powell]

re. remember {} - keep in mind that until the composition is applied, nothing that executed in a composable has "happened" yet. Beware of executing side effects that leak this way.

[Adam Powell]

is there a better way to represent what's happening here other than creating an event for a viewmodel to handle in the form of an on-appeared method call?

[Adam Powell]

it sounds like at a minimum there's a scenario being created here with some sort of id parameter where that id can potentially change, and subsequent recompositions would not capture this if you enforce a once-only call. Same as initializing the contents of a remember {} from parameters without a key.

[Adam Powell]

this sort of thing is why SideEffect doesn't take keys - it expects whatever it does to implement idempotence somewhere else

[Colton Idle]

Yeah, I think what I really need is to basically use viewModels init {} block (I know its contensious, but its a pretty good default/safe spot to put the kind of code you need triggered on "screen is now showing")

[Colton Idle]

So the fact that I added and named a new function init() that takes an arg basically does exactly what we want it to do.

[nitrog42]

usually on my code, the method init(someId) simply does idStateFlow.emit(someId) , which won't reemit the same value so it's kinda ok for me.
I could also do something like

LaunchedEffect(viewModel, someId) {
    viewModel.init(someId)
}

but like you i'm not sure what is the general consensus here (but like some others comments I try to do it through navArgs and getting them from the SavedStateHandle in the viewModel to avoid having it in UI code)