What do you folks think about models that contain ...

# android-architecture

[juliocbcotta]

What do you folks think about models that contain data and callbacks?
For instance, a ViewState model that contains a callback for OnClickListener ? I few that it is nice as you can create that in the VM and avoid calling the VM directly, but at the same time I think it might be break the single responsibility idea... WDYT?

[Javier]

state and model words don’t sound like something that should store anything that is not data.

[Stylianos Gakis]

I've read I think on the docs of circuit from slack how they do end up doing that actually, and how that helps with only allowing certain actions to be done when the screen is in a specific state, since you can't access the other lambdas all the time even when they may not be applicable. But it does come with some drawbacks like Javier says regarding mixing concepts together etc.
But it's definitely not unheard of to do it this way

Read more here https://slackhq.github.io/circuit/states-and-events/

[curioustechizen]

The PR linked to at the bottom of Circuit's states and events docs have a lot more detail about the motivation behind this decision. It also lists some pros and cons:

https://github.com/slackhq/circuit/pull/146

[juliocbcotta]

I started to think about this in the android context... Adopting this arch would mean that the model can't be serialised... So no Parcelable/rememberSavable or SavedStateHandle usage would be possible.

[Zac Sweers]

That’s not a bad thing. In circuit that’s actually a feature, it wants you to decompose your state and store it granularly with rememberSaveable, rememberRetained (a tool we built for Circuit), etc

[Zac Sweers]

worth mentioning this is just how Circuit conventionally does it. We’ve explored (and Circuit supports, or at least doesn’t prevent) alternative patterns that we prototyped recently here: https://github.com/slackhq/circuit/pull/1077

Option 1 is what circuit currently does, there’s some others that might be relevant to this thread. The PR description has a good overview of the pros and cons of different approaches

[juliocbcotta]

It is a feature only if the app is using circuit, for everyone else that is still trying to use VMs and other google libraries it is a liability as it is easy to mistakenly try to serialise those models.

[Zac Sweers]

I know, that’s why I prefaced it with “In circuit” 🙂

[Arkadii Ivanov]

From my point of view storing callbacks in the state might be dangerous. A callback usually captures things like VM, repository, Context, etc. And so it becomes easier to leak things, e.g. by storing the state somewhere. For instance, Compose had a bug with leaking previous states (https://issuetracker.google.com/issues/203102179). Even though it's marked as fixed, it still demonstrates some possible issues. Or you may want to just store the state for later, for some reason.

Also, I might be missing some context, but storing callbacks in the state may complicate testing. E.g. it's not so easy to create two equal instances of the state with same data. Like, val a = MyState(x = 1, onClick = { <capturing lambda> }) and val b = MyState(x = 1, onClick = { <capturing lambda> }) may not be equal to each other.

Inability to serialize the state is another concerns. Looks incompatible with debugging tools like time travelling.

[Zac Sweers]

Nah it’s not dangerous, or at least not any more dangerous than any other code that uses lambdas. A good framework/API makes it hard to leak UI state out of the presentation-UI bridge, and substates management has been a pretty good natural forcing function to make developers more intentional about their retention (i.e. `remember`/`rememberRetained`/`rememberSaveable`). A good example of this is that developers naturally don’t try to rememberRetained/rememberSaveable an event sink, rather than accidentally retaining it throwing a larger aggregated state over the fence into storage.

The testing point is a fair one and one we’ve found annoying at times, though not as much as I worried. We’re weighing option 2 in the prototypes PR I linked to solve that.

[Arkadii Ivanov]

I'm pretty sure it is dangerous, as I had fixed a few bugs in production related exactly to callbacks stored in the state, and the state being assigned to a property in a scope wider than the screen's scope (for analytics purposes).

For instance if your screen has:

data class SomeScreenState(val x: Int, val onClick: () -> Unit)

Then you assigned this state somewhere in the screen's parent level (e.g. in Activity or something like that):

var lastSomeScreenState: SomeScreenState? = null

// at some point later

lastSomeScreenState = ...

Then you essentially leak the entire object hierarchy captured by onClick. Even worse, you may leak your Activity if you remember the state in Application scope.

That's said, it's possible to avoid leaks of course, but it's pretty error prone, from my point of view.

[Arkadii Ivanov]

You may need to store the state of a finished screen until the whole flow is finished, so that you can do additional things in the end.

[Zac Sweers]

I think the bigger problem there is that the arch is inviting the developer to store arbitrary state in some parent scope it shouldn’t necessarily have access to 🤷. You could just as easily replace onClick with something that holds any reference that shouldn’t be kept and still have the same problem. Better to make the wrong thing harder to do

[Zac Sweers]

This is a good part of why running presentation logic in compose was enticing with circuit

[Zac Sweers]

There’s an argument maybe that capturing lambdas don’t always make what they’re capturing obvious, but you can again sidestep this by having your arch force the user to be intentional about how sub states are stored in the first place, or limiting their access to ability to store it in places they shouldn’t