Looking into a sample code from `orbit-mvi/orbit-m...

# orbit-mvi

[miqbaldc]

Looking into a sample code from orbit-mvi/orbit-mvi, compose effect are wrapped with LaunchedEffect

Strangely in our case, we were unable to observe same sideEffect twice tho

Does it compose LaunchedEffect are not meant to observe same sideEffect, or maybe any other alternative?

[miqbaldc]

cc @adeyds

[Mikolaj Leszczynski]

@miqbaldc do you mean to have two separate subscriptions?

[miqbaldc]

Looks like in our case, we still wants to have one subscriptions, but seems our code doing:

1. viewmodel.observe, in Activity
2. also subscription inside setContent
are considred two separate subscriptions?


---
here’s our snippet code attachment:

1. Compose Screen
2. Activity

[Mikolaj Leszczynski]

No, this looks fine. I have a few comments here:

1. The way you subscribe to side effects is not entirely correct. This will not cache the side effect emissions correctly when there are no subscribers. We have an open issue that we’re currently fixing. In the meantime you can subscribe in the way Matt outlined in the issue.
2. dialogType is remember, which means that if this set to the exact same value, a recomposition won’t happen. Could this be the issue you’re seeing? I don’t see anything out of the ordinary otherwise.

[miqbaldc]

2. if this set to the exact same value, a recomposition won’t happen

Shots, seems this is the culprit tho’.

In our case, we changed from LaunchedEffect to use SideEffect and able to show the dialog everytime (the same sideEffect subscribed)

[miqbaldc]

1. This will not cache the side effect emissions correctly when there are no subscribers. In the meantime you can subscribe in the way Matt outlined in the issue.

That’s awesome. The extensions would be great! Will implement this later in our extensions as well

[miqbaldc]

but for no. 2, our issue was the LaunchedEffect didn’t trigger any recomposition or whatsoever, and didn’t reach the sideEffectFlow.collect {} part.

[miqbaldc]

The only caveats about using mentioned extensions in here was it makes our Composable functions coupled with orbit-mvi which why the reason we consider to use a Flow or a plain data class <The State> as a Composable argument there

[Mikolaj Leszczynski]

I’m not sure SideEffect is appropriate - it is executed on every successful recomposition which is not really what you want here? Rather you want collect to be kept through the lifetime of the composition

https://developer.android.com/jetpack/compose/side-effects#sideeffect-publish

[Mikolaj Leszczynski]

LaunchedEffect itself won’t cause recompositions, but your actions in collect could, through altering the remembered State objects for example.

[Mikolaj Leszczynski]

Shots, seems this is the culprit tho’.

In our case, we changed from LaunchedEffect to use SideEffect and able to show the dialog everytime (the same sideEffect subscribed)

Not sure I follow. So the thing I outlined was the problem, but you decided to fix it via changing to SideEffect? Not sure this is the right solution.

If you’re handling the dialog as State in Compose by using remember, it should be part of the ViewModel’s State object as well IMHO. The dialog is either there or it isn’t - very stateful behaviour.

What you have currently introduces state in Compose that is decoupled from your ViewModel. Personally I think this piece of state belongs in the ViewModel’s State and Compose should bind to it. I’m firmly in the camp of the ViewModel being the source of truth - otherwise the state ownership is unclear and you can get subtle bugs like this.

Only use side effects if you don’t care about the dialog state in Compose. But personally I think it’s not really the right way.

[miqbaldc]

Not sure I follow. So the thing I outlined was the problem, but you decided to fix it via changing to SideEffect? Not sure this is the right solution.

We’re currently unsure as well if it due to dialogType by remember or not, but we decide to leave it as-is, and change LaunchedEffect to SideEffect

[miqbaldc]

What you have currently introduces state in Compose that is decoupled from your ViewModel. Personally I think this piece of state belongs in the ViewModel’s State and Compose should bind to it. I’m firmly in the camp of the ViewModel being the source of truth - otherwise the state ownership is unclear and you can get subtle bugs like this.

We need to refactor a bit our composable screen for this use case tho’. 🙏

[miqbaldc]

seems using the extensions mentioned collectSideEffect in our part, also doesn’t succeed tho’

[miqbaldc]

Only use side effects if you don’t care about the dialog state in Compose. But personally I think it’s not really the right way.

If we understand you correctly, did you suggest to move dialogType by remember from Composable function, into dialogType by mutableStateOf in ViewModel? cmiiw

[miqbaldc]

cc @adeyds