Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prototype different state designs #1077

Closed
wants to merge 4 commits into from
Closed

Prototype different state designs #1077

wants to merge 4 commits into from

Conversation

ZacSweers
Copy link
Collaborator

@ZacSweers ZacSweers commented Dec 21, 2023

Based on some conversations with @adamp and @jingibus

There are a few different examples here.

  1. AsDataClass - a canonical Circuit state data class with an encapsulated event sink.

    • Pros
      • Familiar to UDF
      • Easy to pipe into Molecule, which unlocks...
        • Dead-simple to stand up tests, assert new state emissions
        • Easy to potentially run presenters detached from the UI (retained, off the main thread, etc)
        • Easy to interop to non-Compose/non-Circuit frameworks
      • Dead-simple to stand up UI tests with stub states
      • Testing events coming out of UIs is easy w/ TestEventSink
    • Cons
      • New states emit each time, incurring recompositions
      • Event sink property results in broken equals/hashCode/toString unless you remember the sink first (sometimes?). See Add rememberEventSink #1074
  2. AsPokoClass - similar to AsDataClass, but would use the Poko library to generate equals/hashCode/toString for every property but the eventSink. Events in this case would move to a EventSink interface, where events are sent instead to a send() function.

    • Pros
      • Familiar to UDF
      • Easy to pipe into Molecule, which unlocks...
        • Dead-simple to stand up tests, assert new state emissions
        • Easy to potentially run presenters detached from the UI (retained, off the main thread, etc)
        • Easy to interop to non-Compose/non-Circuit frameworks
      • Dead-simple to stand up UI tests with stub states
      • Event sink doesn't participate in equals/hashCode/toString
      • Testing events coming out of UIs is easy w/ TestEventSink
    • Cons
      • New states emit each time, incurring recompositions
      • Requires another library/compiler plugin
    • Unclear if pro or con or neither
      • State now is an event sink
  3. AsInterface - an interface implementation of state, more akin to traditional Compose states with interfaces backed by State objects and driven by snapshots. This exposes no event sink, but rather semantically meaningful functions. These state objects are remember'd and only one is ever emitted.

    • Pros
      • Familiar to conventional Compose
      • Single state emission/instance, stable by definition. Properties can be backed directly with State objects. More granular recomposition in the UI as a result.
      • Event sink doesn't participate in equals/hashCode/toString
    • Cons
      • Realistically requires lint checks to prevent var, MutableState, missing remember, etc calls.
      • Unclear if this can work with Molecule at all, which in turn...
        • Makes interop with other frameworks harder
        • Cannot potentially run presenters detached from the UI (retained, off the main thread, etc)
      • Standing up stub states in tests are more tedious vs simple value classes
      • Line between the state class and the presenter get blurrier WRT state ownership/responsibilities
      • Testing events coming out of UI is no longer easy or doable with TestEventSink. Developers may reach for mocking + verify if they aren't using a functional presenter
    • Unclear if pro or con or neither
      • State now only emits once, tests must now assert state properties individually
      • Somewhat departs UDF, or at least a significantly different flavor of it
      • Presenter tests are different, only one state means you just interact with that instance. Tools like snapshotFlow could still be useful in tandem with Turbine, but most of the existing test infra in Circuit isn't useful for this pattern.

This boils down to two patterns - immutable state trees (1 and 2) and stable snapshot-driven objects (3). 1 and 2 are more traditional UDF, 3 is more traditional compose.

Others presented as examples
4. AsInterfaceWithEventSink - similar to AsInterface, but with an EventSink supertype for event objects rather than function calls.
5. AsAbstractClassWithEventSink - something in between the 1 and 3, with the best/worst of both worlds.


One really great thing here is that Circuit's current published API supports all of these cases, so its preference/guidance for one doesn't inherently prohibit the use of the others.

@ZacSweers ZacSweers mentioned this pull request Dec 21, 2023
@ZacSweers
Copy link
Collaborator Author

Note these UI tests assume that Compose UI test APIs work, which...

image

@ZacSweers
Copy link
Collaborator Author

Added a demo of an interface approach in CatchUp here to offer a more side-by-side example: ZacSweers/CatchUp#1168

@chrisbanes
Copy link
Contributor

chrisbanes commented Dec 22, 2023

Personally, I'm not a huge fan of any of these 😬. They all feel a bit like papering over the cracks of us merging State + events, as a convenient way to get them into the Ui.

The vague idea I've had in the back of my head is making the event sink an explicit part of the Presenter and Ui APIs:

abstract class Presenter<CircuitState, CircuitEvent>(val eventSink: Flow<CircuitEvent>)  {
    @Composable abstract fun present(): CircuitState
}

and then pass the same flow in in a similar fashion to the Ui. We could probably tidy up how the Flow is passed in, but the general idea of injecting the flow feels cleaner to me.

@saket
Copy link
Contributor

saket commented Dec 22, 2023

Based on some conversations with @adamp and @jingibus

Can you share some more context about the motivations for these ideas? These ideas don't feel very different from the current design.

I don't fully understand the differences between ideas #1 and #2. Does using poko solve the equals problem because event sinks will no longer participate in equals/hashCode/toString?

I'd advice against #3. One of the benefits of using objects for dispatching events is that your presenters can dynamically provide events to the UI. Here's an example:

@Composable 
fun FooListItem(model: M) {
  Box(
    Modifier.onLeftSwipe { 
      model.eventSink(model.onLeftSwipeEvent)
    }
  )
}

Sure you can achieve this by having a semantic onLeftSwipe() function in the interface, but it's much easier if you're able to specify the event inline when constructing UI models.

In general I agree with @chrisbanes that keeping the event sink separate is always a good idea, if that ship hasn't sailed yet.

@ZacSweers
Copy link
Collaborator Author

ZacSweers commented Dec 22, 2023

val eventSink: Flow

Circuit actually did this initially (matching what Broadway does iirc) before we moved to this continuable state pattern, though the sink came in as a param to present() rather than the presenter constructor. The reasons we moved away from this though were:

  1. Context-switching between compose and coroutines were tedious. We dried some of it up with an EventCollector helper like this

    override fun present(events: Flow<Event>): State {
      EventCollector(events) { event ->
        // ...
      }
    }
  2. Event flows were limited and global. Even if you had n-different states, you could only have a global set of events and need to handle all of them regardless of the state. This meant you could have awkward situations where a loading state needs to handle events that could only happen in an error situation. It's not the worst, but it did get a little weird. Switching to per-state sinks actually exposed tests that were trying to do such situations, and just plainly didn't compile (yay) after the switch.

  3. Composite presenters were tedious because you have to make both composite state and shuttle composite events around too. You had to do really parent interfaces and then flows filtering by instance, etc etc. Here's an example from the PR where we switched.

  4. There was a potential for dropping events with the intermediary Channel, but I don't remember exactly what it was now.

If there's a way to support this again for those cases where it's easier/simple enough I'd be game, but I wasn't able to figure out a way that doesn't just end up with two different presenter interfaces. Your constructor approach is interesting though, and maybe there's a path there that involves supporting the event sink as an assisted injection parameter. That complicates some other API surface areas, but open to suggestions.

Does using poko solve the equals problem because event sinks will no longer participate in equals/hashCode/toString?

Yep! They've even got a test for this case: https://github.com/drewhamilton/Poko/blob/main/poko-tests/src/commonTest/kotlin/SimpleWithExtraParamTest.kt

@ZacSweers
Copy link
Collaborator Author

re: motivations

Mostly just explorations for fun. Adam's a big advocate (and correct me if I'm wrong!) of the snapshot-driven state as a means to make recompositions much more granular without having to really think about it or worry about some of the stuff we have now with new state emissions incurring them. The event sink as a property participating in equals/hashcode has also been a long annoyance as it makes tests feel a little impure when you can't actually compare two states.

@alexvanyo
Copy link
Contributor

Here's maybe a fun exercise: when you see just @Composable fun FooListItem(model: M), what is your initial assumption about what M looks like?

Are you assuming that M is an immutable data class, or do you assume that M is a stable, snapshot-aware object?

I am also a fan of the snapshot-state object flexibility (probably influenced by Adam), and I really like the 3 version of the FooListItem case where M has an onLeftSwipe method:

@Composable 
fun FooListItem(model: M) {
  Box(
    Modifier.onLeftSwipe {
      model.onLeftSwipe()
    }
  )
}

When I'm creating M to implement onLeftSwipe, I can delegate it, have it depend on more localized state, or do anything else that I can with a method.

But since I've been using that pattern a lot, I may have a different view when initially reading @Composable fun FooListItem(model: M): I am very strongly expecting M to be a stable, snapshot-aware object. It could be a fully immutable data class, since a immutable data class is a simple case of a stable, snapshot-aware object, but I don't necessarily assume that it is immutable.

@chrisbanes
Copy link
Contributor

The list item example is being used above, but the reality is that you wouldn't pass down the State instance to a list item like that.

Your top level UI composable would receive the State, and you'd the deconstruct it and pass down the appropriate sub-state/lambdas. Unless anyone has a real world example of this?

@ZacSweers
Copy link
Collaborator Author

Yeah I agree with Chris, I would conventionally write that function like this

@Composable 
fun FooListItem(model: M, onLeftSwipe: () -> Unit) {
  Box(
    Modifier.onLeftSwipe(onLeftSwipe)
  )
}

@ZacSweers
Copy link
Collaborator Author

ZacSweers commented Jan 11, 2024

Gonna close this out, may probably write these out to a section of the doc site for posterity and call it a day. Thanks for the inputs!

@ZacSweers ZacSweers closed this Jan 11, 2024
@ZacSweers ZacSweers deleted the z/statePrototypes branch January 11, 2024 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants