Swift UI views are not updated due to capturing viewStore (vs current state) in closures.

[grigorye]

Describe the bug

A typical example of TCA usage advocates accessing state via viewStore, directly in bodies, like below (please note, viewStore.send is there just for completeness of the example, and has nothing to do to the problem per se - what is important is viewStore.state):

WithViewStore(store) { viewStore in
    WrapperX {
        ComponentY(viewStore.state, action: { viewStore.send(.action) })
    }
}

However, it results in no updates in Swift UI hierarchy in some "pretty innocent" cases of WrapperX implementation, based on closures.

To Reproduce

Please see https://github.com/grigorye/SwiftUIBodies.

Expected behavior

I would expect that WithViewStore would trigger updates inside WrapperX, in any variant of the implementation.

Screenshots

Please see https://github.com/grigorye/SwiftUIBodies.

Environment

• swift-composable-architecture version 0.33.1
• Xcode 13.2.1
• Swift 5.5
• OS: Any

Additional context

The problem lies in the fact that viewStore is captured by reference (vs value, hence not accounting the value of state, if any), and the fact that body-generating closures in the wrappers are apparently part of the equation used when SwiftUI determines the need for re-render.

In the simplest case, a wrapper like below might be used to avoid the possibility of capturing the viewStore, but I'm not sure if it's applicable to more complex scenarios:

// Convenience initializer that makes it impossible to *accidentally* capture viewStore in a closure.
// See https://github.com/grigorye/SwiftUIBodies for more info.
extension WithViewStore where State: Equatable, Content: View {
    init(
        _ store: Store<State, Action>,
        content: @escaping (_ state: State, _ sendAction: @escaping (Action) -> Void) -> Content
    ) {
        self.init(store, content: { (viewStore: ViewStore<State, Action>) -> Content in
            content(viewStore.state, viewStore.send)
        })
    }
}

Usage:

WithViewStore(store) { state, sendAction in
    WrapperX {
        ComponentY(state, action: sendAction(.action))
    }
}

[tgrapperon]

I didn't have the time to investigate much, but please note that manually installing an @ObservedObject viewStore: ViewStore in your Component view instead of using WithViewStore fixes the issue.

There's a subtlety in SwiftUI's invalidation process that I'm missing because I don't get why the WithViewStore content doesn't get the same treatment as its enclosing view. There are known issues with WithViewStore and GeometryReader or Steppers for example (#235, #224), and they're maybe related to the same cause.

A quick glance at the WithViewStore implementation makes me wonder if the Content closure shouldn't be eagerly evaluated in the init instead of capturing it. The GeometryReader closure is escaping, so I wonder if this is not causing too many closures being captured instead of evaluated, and changes being unaccounted. The same kind of things seems to happen in your demo with your TextViews. Many Views in SwiftUI are eagerly evaluating their @ViewBuilder arguments.
I don't know if it'll fix the issue and how much it will impact performance (we don't have SwiftUI benchmarks to check yet). I'll experiment later.

[tgrapperon]

Thanks @iampatbrown! That makes sense I guess.

I think creating a copy of the ViewStore is a fairly lightweight way to fix this from a WithViewStore perspective.

That's indeed transparent for the user. I have two small reservations:

• Wouldn't this cause over-invalidation of the dependency graph? We're always providing a new value when building the view, even if the viewStore's state hasn't changed. Maybe we can perform additional checks somewhere to vend a new copy only when needed (we can maybe lazily instantiate the copy on access, store it in the viewStore itself to reuse it, and delete it on objectWillChange?). We can store the copy in the ViewStore or maybe in WithViewStore, into some @State box. But maybe this is not needed. I need to think more about it. As I'm getting it, the copy should happen in the init instead of body to be detected, isn't it?
• Reassigning the objectWillChange publisher is a little scary at first sight: is it the only thing we need to perform to make both viewStores identical as ObservableObject, are we not breaking some internal wiring with @Published properties for example? EDIT: I've checked again your implementation, and you initialize the ViewStore directly from the original, so I have no concern over this.

I still think that having a value type + dedicated property wrapper could be desirable in the long run, but using copies is indeed a lightweight fix to the nesting issue.

[tgrapperon]

@iampatbrown, after thinking more about it, I don't have reservations anymore about over-invalidation. The WithViewStore body will run only when the state has changed, so it's not problematic to use a new instance when doing this, and the viewStore will behave like a value type, with a new value when the state changes.
And I don't have issues either with the way you build a copy as it happens in the ViewStore's initializer. The @ObservedObject instance is private, so it changes nothing for the user, and the wiring makes them talk to it directly, without much indirection.

So this seems a very good solution, optimal in every aspect!
Do you want to prepare a PR? or I can give it a try if you don't have the time.

[iampatbrown]

The WithViewStore body will run only when the state has changed, so it's not problematic to use a new instance when doing this, and the viewStore will behave like a value type, with a new value when the state changes.

@tgrapperon This is basically what I was thinking. I wanted to ensure my understanding was correct before getting back to you though :)

I'm happy to do a PR, I'd like to wait for some input from @stephencelis and @mbrandonw though. There could be some stylistic considerations as well as implications extending beyond this particular issue.

If you feel like putting together a PR now though, go for it :) We can use that for further discussion.

[tgrapperon]

@iampatbrown I've drafted #1015 in this spirit.
It seems to fix the issue by the way!

[iampatbrown]

Awesome. Good work!

[mbrandonw]

This isn't a problem with TCA per se, and would be easily reproducible in a vanilla SwiftUI application too. In fact we have some bugs filed with Apple that demonstrate the problem (here and here).

However, there is an easy change that fixes the problem, and it's probably the better way of constructing your views anyway. Although most SwiftUI views involve closures in some way, 99% of the time they are non-escaping closures. That means the closures aren't actually meant to be stored away and lazily evaluated later, but rather they just serve as a convenient syntax and are evaluated immediately.

For example, the Section view takes view builder closures for parent, content and footer views, but none of those closures are escaping:

extension Section where Parent : View, Content : View, Footer : View {
  public init(@ViewBuilder content: () -> Content, @ViewBuilder header: () -> Parent, @ViewBuilder footer: () -> Footer)
}

This is how SwiftUI is meant to be used.

So, if you change all of your views to no longer take escaping closures in the initializers, and instead eager evaluate the closures, you will see that everything works as you expect:

struct FancyTextWrapper: View, Traceable {
  let content: Text
  init(@ViewBuilder content: () -> Text) {
    self.content = content()
  }
  var body: some View {
    content
  }
}

Hope it helps!

Also, I'm going to convert this issue to a discussion since it does not seem to be a bug in the library.

[grigorye]

Thanks! This looks really interesting - will try that. Yeah, I did not meant it's a bug in TCA per se, rather something that we encountered while working with TCA and Swift UI, that is still new for us as developers. Wondering if something like this (eager evaluation of closures) could be/is put in best practices or somewhere else - it does not look evident at all, from my perspective.

[grigorye]

Checked it and I can confirm that eager evaluation fixes the problem. All in all, now it looks reasonable to me: on a change of the state, we want to regenerate the body, right away, and the most straightforward way to achieve that is just evaluating all the closures responsible for the generation. Lazy evaluation provides an alternative (if it's sound at all) but, as we figured it out, it's risky, does not provide any benefits (including performance, as far as I could imagine), and is just a more complex solution for the same task.

[grigorye]

bugs filed with Apple that demonstrate the problem (here and here).

Just in case, I could not reproduce anything meaningful with the second example. Left a comment in the gist.