pointfreeco / swift-composable-architecture

A library for building applications in a consistent and understandable way, with composition, testing, and ergonomics in mind.
https://www.pointfree.co/collections/composable-architecture
MIT License
11.91k stars 1.37k forks source link

Over-Observation with enum based State #3163

Closed andtie closed 1 month ago

andtie commented 1 month ago

Description

When using the @ObservableState-macro in a reducer with enum based state, every action sent to the store triggers a re-render of SwiftUI-views.

Checklist

Expected behavior

When sending an action to a store that does not update the state, a SwiftUI-view should not be re-rendered.

Actual behavior

The SwiftUI-view is re-rendered for every action.

Steps to reproduce

Consider the following feature and view:


@Reducer
struct SomeOtherFeature {}

@Reducer
struct EnumBugFeature {

    @ObservableState
    enum State {
        case loggedOut
        case loggedIn(SomeOtherFeature.State)
    }

    enum Action {
        case noop
    }

    var body: some Reducer<State, Action> {
        EmptyReducer()
    }
}

struct EnumBugView: View {
    let store: StoreOf<EnumBugFeature> = .init(initialState: .loggedOut) {
        EnumBugFeature()
    }

    var body: some View {
        let _ = Self._printChanges()
        switch store.state {
        case .loggedIn:
            Text("LoggedIn")
        case .loggedOut:
            Button("Test") {
                store.send(.noop)
            }
        }
    }
}

Every press on the Button triggers a view update.

This is what I found debugging this issue

Part of the code generated by the @ObservableState macro is the following:

public var _$id: ComposableArchitecture.ObservableStateID {
    switch self {
    case .loggedOut:
        return ObservableStateID()._$tag(0)
    case let .loggedIn(state):
        return ._$id(for: state)._$tag(1)
    }
}

The problem seems to be that the id for the loggedOut-case is regenerated every time it is accessed. If I add an associated value to the case that is itself marked with @ObservableState the problem goes away and there are no unnecessary re-renders.

Bonus

In the following code, this bug actually triggers an infinite rerender-loop in SwiftUI:

struct InfiniteBugEnumView: View {
    let store = StoreOf<EnumBugFeature>(initialState: .loggedOut) {
        EnumBugFeature()
    }

    var body: some View {
        ZStack {
            Text("count: \(store.state)")
        }
        .fullScreenCover(isPresented: .constant(true)) {
            NavigationStack {
                Form {
                    Text("count: \(store.state)")
                        .sheet(isPresented: Binding(
                            get: { true },
                            set: { _ in }
                        )) {
                            NavigationStack {
                                ZStack {
                                    Form {
                                        Text("count: \(store.state)")
                                    }
                                    .sheet(isPresented: .constant(true)) {
                                        Text("Leaf")
                                            .onAppear {
                                                print("onAppear", Date())
                                                store.send(.noop)
                                            }
                                    }
                                }
                            }
                        }
                }
            }
        }
    }
}

The Composable Architecture version information

1.11.1

Destination operating system

iOS 17.5

Xcode version information

Version 15.4 (15F31d)

Swift Compiler version information

Apple Swift version 5.10 (swiftlang-5.10.0.13 clang-1500.3.9.4)
Target: arm64-apple-darwin23.5.0
mbrandonw commented 1 month ago

Hi @andtie, the first issue, that of over-observing, is to be expected. If the case of an enum holds non-ObservableState (which is the situation with loggedOut), then we have no choice but to consider every value completely distinct from any other value. Observable state is what gives us the _$id that allows us to distinguish when completely new states are created.

You can see this concretely if you default your EnumBugView to start in the loggedIn state. Then you will see that sending .noop does not cause any view re-renders:

struct EnumBugView: View {
  let store: StoreOf<EnumBugFeature> = .init(initialState: .loggedIn(SomeOtherFeature.State())) {
    EnumBugFeature()
  }

  var body: some View {
    let _ = Self._printChanges()
    switch store.state {
    case .loggedIn:
      Text("LoggedIn")
      Button("Test") {
        store.send(.noop)
      }
    case .loggedOut:
      Button("Test") {
        store.send(.noop)
      }
    }
  }
}

And it's because we are able to see that the _$id of SomeOtherFeature.State did not change:

However, that doesn't mean there isn't something to improve here. We technically could special case Void in order to elide view updates, or maybe even try to dynamically open Equatable existentials at runtime to elide renders. It's something worth thinking about, but so far our opinion has been that it isn't really a big deal to get a few extra renders for non-TCA features embedded in enums.

And then as for the InfiniteBugEnumView, this seems to be more of a SwiftUI bug than anything else. First of all, both ZStacks and Forms have a notoriously rocky relationship with onAppear. When mixing the two you will often find that onAppear calls way too often. We have filed many, many bugs with Apple about this.

And further, you are using .constant(true) for all of the bindings, which is likely to confuse SwiftUI quite a bit. SwiftUI tends to like a stable notion of state, and here you will be creating a fresh new binding every time the view re-computes. I'm almost certain you could reproduce the bug you are seeing in vanilla SwiftUI using just an @Observable model.

If you were to replace the .constant(true) bindings with a true binding, like one derived from @State, you will see that the problem goes away:

struct InfiniteBugEnumView: View {
  let store = StoreOf<EnumBugFeature>(initialState: .loggedOut) {
    EnumBugFeature()
  }

  @State var isCoverPresented = true
  @State var isSheetPresented = true
  @State var isOtherSheetPresented = true

  var body: some View {
    ZStack {
      Text("count: \(store.state)")
    }
    .fullScreenCover(isPresented: $isCoverPresented) {
      NavigationStack {
        Form {
          Text("count: \(store.state)")
            .sheet(isPresented: $isSheetPresented) {
              NavigationStack {
                ZStack {
                  Form {
                    Text("count: \(store.state)")
                  }
                  .sheet(isPresented: $isOtherSheetPresented) {
                    Text("Leaf")
                      .onAppear {
                        print("onAppear", Date())
                        store.send(.noop)
                      }
                  }
                }
              }
            }
        }
      }
    }
  }
}

The onAppear is still called way more than I would expect, but again I think that is due to a confluence of bugs in ZStack and Form.

Since this isn't an issue with the library I am going to convert it to a discussion. Please feel free to continue the conversation over there!