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
12.35k stars 1.44k forks source link

onAppear is called multiple times within an IfLetStore #39

Closed LK closed 4 years ago

LK commented 4 years ago

Describe the bug If I use an IfLetStore to swap between two views, and the "then" view includes an .onAppear modifier, the callback is called twice each time the view is presented. Replicating the same behavior using a regular if viewStore.optional != nil seems to work as expected.

To Reproduce Here's a quick repro case: TCABug.zip

The commented out chunk in ContentView.swift implements the functionality without IfLetStore, and appears to work. Using the IfLetStore version, you should see two logs every time you switch to the hello world view.

Expected behavior I expect onAppear to only be called once.

Screenshots N/A

Environment

Additional context N/A

stephencelis commented 4 years ago

Thanks for the detailed report and repro! We'll dig into things.

mbrandonw commented 4 years ago

Hey @LK, we looked into this issue and believe that this is a bug in SwiftUI, not TCA. Here's what we uncovered.

The bug seems to be directly related to using Group for the then view. If you wrap your if viewStore.showing != nil inside Group { ... } you will see that it has the same problem.

In fact, you can even refactor the entire example to just use vanilla SwiftUI with @State and you will see that .onAppear is called twice with nested Groups :(

So, as a workaround, if you can restructure your ThenView so that it's not a Group at the root, then it should work. For example, if you used a VStack:

 IfLetStore(self.store.scope(state: \.showing), then: { myStore in
-    Group {
+    VStack {
         Text("Hello, World!")
         Button(action: { viewStore.send(.toggle) }) {
             Text("TOGGLE")
         }
     }
     .onAppear { print("onAppear :(") }
 }, else: OtherView(store: self.store))

Another work around is moving .onAppear to a view inside the group:

 IfLetStore(self.store.scope(state: \.showing), then: { myStore in
     Group {
         Text("Hello, World!")
         Button(action: { viewStore.send(.toggle) }) {
             Text("TOGGLE")
         }
+        .onAppear { print("onAppear :(") }
     }
-    .onAppear { print("onAppear :(") }
 }, else: OtherView(store: self.store))

But, while typing all of this out I just realized that our implementation of IfLetStore can be done without the Group, which avoids the doubly nested groups, and so will at least fix this problem. PR coming soon!

mbrandonw commented 4 years ago

Should be fixed here https://github.com/pointfreeco/swift-composable-architecture/pull/41

Gonna remove the bug label from this issue since it technically isn't a TCA bug 😅

mbrandonw commented 4 years ago

Sorry, didn't mean to auto-close this with the PR. Please feel free to re-open if you have more problems!

LK commented 4 years ago

You're right, I didn't realize IfLetStore was introducing another layer of Group which was causing the issue. I'll file a radar. Thank you both for the quick response!