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.25k stars 1.42k forks source link

Optional reducer crash when onAppear used from TabView #282

Closed maciejczechowski closed 3 years ago

maciejczechowski commented 4 years ago

Describe the bug We hit assertion on optional reducer if onAppear (from optional state) is called within tabView. OnAppear is called immediately after state is set to nil, causing a crash due to assertion.

To Reproduce The project file is here: ComposableTests.zip

In general, the structure is:

We have a root view, which can be either login or content. Content has an option to log out. Right now this is done via struct, we'll be experimenting with enums soon (I don't think it has anything to do with that):

struct LoginState: Equatable {}
struct LoggedInState: Equatable{}

enum LoginAction: Equatable {  case login }
enum LoggedInAction: Equatable {
    case logout
    case onAppear
}

struct RootState: Equatable {
    var loginState: LoginState?
    var loggedInState: LoggedInState?

    init(){
        loginState = LoginState()
    }
}

enum RootAction: Equatable{
    case login(LoginAction)
    case loggedIn(LoggedInAction)
}
struct Environment {}

The reducer is a combination of optional pullbacks and switching main state:

let loginReducer = Reducer<LoginState, LoginAction, Environment>
{ state, action, environment in
    return .none
}

let loggedInReducer = Reducer<LoggedInState, LoggedInAction, Environment>
{ state, action, environment in
    return .none
}

let rootBaseReducer = Reducer<RootState, RootAction, Environment>{
    state, action, environment in
    switch action {

    case .login(.login):
        state.loggedInState = LoggedInState()
        state.loginState = nil
    case .loggedIn(.logout):
        state.loggedInState = nil
        state.loginState = LoginState()
    default:
        return .none
    }
    return .none
}

let rootReducer = Reducer<RootState,RootAction, Environment>.combine(
    loginReducer.optional().pullback(state: \.loginState,
                                     action: /RootAction.login,
                                     environment: { $0 }),

    loggedInReducer.optional().pullback(state: \.loggedInState,
                                        action: /RootAction.loggedIn,
                                        environment: { $0 }),
    rootBaseReducer
)

The content view is a simple tab:


struct ContentView: View {
    let store: Store<RootState, RootAction>
    var body: some View {
        IfLetStore(self.store.scope(state: {$0.loginState},
                                    action: RootAction.login),
                   then: { store in
                    WithViewStore(store){viewStore in
                        Button("Login"){
                            viewStore.send(.login)
                        }
                    }

                   })

        IfLetStore(self.store.scope(state: {$0.loggedInState},
                                    action: RootAction.loggedIn),
                   then: { store in
                    WithViewStore(store){viewStore in
                        TabView{
                            Text("A").onAppear{viewStore.send(.onAppear)}.tabItem{ Text("1")}
                            Button("logout"){viewStore.send(.logout)}.tabItem{ Text("2")}
                        }
                    }
                   })
    }
}

After tapping logout, we get a crash on optional reducer assertion. It comes from onAppear which is quite unexpected here.

Expected behavior The app should not crash ;) This might be actually a SwiftUI bug, but then we should be able to workaround it somehow. So far I haven't found a good way to do this.

Environment

mbrandonw commented 4 years ago

Sadly this is a SwiftUI bug :( I can even reproduce it with plain SwiftUI:

struct ContentView: View {
  @State var isLoggedIn = false

  var body: some View {
    if self.isLoggedIn {
      TabView {
        Text("A")
          .tabItem{ Text("1") }
          .onAppear{ print("❌ This is called when logging out, even though it should not be.") }

        Button("logout") {self.isLoggedIn = false }
          .tabItem{ Text("2") }
      }
    } else {
      NavigationView {
        Button("Login"){
          self.isLoggedIn = true
        }
      }
    }
  }
}

I've filed a feedback (FB8653218), but you may want to also.

To work around you could implement your own version of .optional() without the precondition just so that you can keep moving forward. This is definitely not ideal because the precondition can really help you catch subtle bugs, such as forgetting to cancel an inflight request when you log out. But at least it's possible to do if you want.

Maybe we need to make an escape hatch for optional() to work around these SwiftUI bugs :/

maciejczechowski commented 4 years ago

That's what I was afraid of :(

I ended up with custom optional version that just prints warning on console in debug. Having such option built-in would be really handy in such cases, SwiftUI will have many more surprises for sure.

ohitsdaniel commented 4 years ago

We could inject the logging as a closure (String) -> Void. This would allow us to replace the fatalError wherever needed.

Another option would be to allow passing a NilFailureMode (naming suggestions welcome).

enum NilFailureMode {
    case strict // fatalError
    case lenient // logger ? print ?   
}

Problem here is that we would restrict library consumers to a single, lenient escape hatch which would be predetermined. Injecting the closure would allow library consumers to define their own way of handling such errors. We could also combine both approaches, offer the NilFailureMode version as a default way to treat such situations but also expose the closure version for better customizability.

hfossli-agens commented 3 years ago

I have extended your example a little bit to reveal a bit more about what's going on

struct ContentView: View {
    @State var isLoggedIn = false
    @State var currentTab = 1

    @ViewBuilder
    func renderTab(label: String, tag: Int) -> some View {
        VStack {
            Print(label, "onRender: isLoggedIn =", self.isLoggedIn)
            Text(label)
            Button("logout") {
                print(label, "on button tap: Log out")
                self.isLoggedIn = false
            }
        }
        .onAppear {
            print(label, "onAppear: isLoggedIn =", self.isLoggedIn)
        }
        .onDisappear {
            print(label, "onDisappear")
        }
        .tabItem {
            Image(systemName: "circle")
            Text(label)
        }
        .tag(tag)
    }

    var body: some View {
        if self.isLoggedIn {
            TabView(selection: $currentTab) {
                renderTab(label: "Tab 1", tag: 0)
                renderTab(label: "Tab 2", tag: 1)
            }
        } else {
            NavigationView {
                Button("Login"){
                    print("Root view on button tap: Log in")
                    self.isLoggedIn = true
                }
            }
        }
    }
}

extension View {
    func Print(_ vars: Any...) -> some View {
        print(vars)
        return EmptyView()
    }
}

Open app and press log in you'll see this in console:

1. Root view on button tap: Log in
2. ["Tab 1", "onRender: isLoggedIn =", true]
3. ["Tab 2", "onRender: isLoggedIn =", true]
4. Tab 2 onAppear: isLoggedIn = true
5. Tab 1 onAppear: isLoggedIn = true

and then switch tab to Tab 1 and you'll see this in console:

6. Tab 2 onDisappear

and then tap "log out" you'll see this in console:

7. Tab 1 on button tap: Log out
8. Tab 2 onAppear: isLoggedIn = false
9. Tab 1 onDisappear

There's 2 issues here.

  1. 5. Tab 1 onAppear: isLoggedIn = true is logged even though only tab 2 is visible
  2. 8. Tab 2 onAppear: isLoggedIn = false is logged even though TabView isn't visible anymore

Here's a proposal for workaround that may cover both issues

struct ContentView: View {
    @State var isLoggedIn = false
    @State var currentTab = 1

    @ViewBuilder
    func renderTab(label: String, tag: Int) -> some View {
        Group {
            if tag == currentTab {
                VStack {
                    Print(label, "onRender: isLoggedIn =", self.isLoggedIn)
                    Text(label)
                    Button("logout") {
                        print(label, "on button tap: Log out")
                        self.isLoggedIn = false
                    }
                }
                .onAppearWorkaround {
                    print(label, "onAppear: isLoggedIn =", self.isLoggedIn)
                }
                .onDisappear {
                    print(label, "onDisappear")
                }
            } else {
                /*
                 Workaround issue 1: Don't render the tab content if the tab isn't selected
                 */
                Text("Empty")
            }
        }
        .tabItem {
            Image(systemName: "circle")
            Text(label)
        }
        .tag(tag)
    }

    var body: some View {
        if self.isLoggedIn {
            TabView(selection: $currentTab) {
                renderTab(label: "Tab 1", tag: 0)
                renderTab(label: "Tab 2", tag: 1)
            }
        } else {
            NavigationView {
                Button("Login"){
                    print("Root view on button tap: Log in")
                    self.isLoggedIn = true
                }
            }
        }
    }
}

extension View {
    func Print(_ vars: Any...) -> some View {
        print(vars)
        return EmptyView()
    }
}

struct OnAppearWorkaroundView<Content: View>: View {
    var view: Content
    var onAppear: (() -> ())?
    var onDisappear: (() -> ())?

    class State {
        var didRender = false
    }

    var state = State()

    var body: some View {
        state.didRender = true
        /*
         Workaround issue 2: Don't call onAppear unless the view tree has been recreated
         */
        return view
            .onAppear {
                if state.didRender {
                    self.onAppear?()
                }
            }
            .onDisappear {
                state.didRender = false
                self.onDisappear?()
            }
    }
}

extension View {
    @ViewBuilder
    func onAppearWorkaround(_ onAppear: (() -> ())?) -> some View {
        OnAppearWorkaroundView(view: self, onAppear: onAppear)
    }
}

In our app we are already doing workaround for issue 1 because of tracking and performance.

AlexisQapa commented 3 years ago

I didn't updated the lib in a while and I wasn't hitting this after pulling the main branch I am hitting this with 100% repro. I'll try to get the version I was previously on.

hfossli-agens commented 3 years ago

We are writing our on TabView due to yet another life cycle issue with apples tabview.

AlexisQapa commented 3 years ago

I thought my issue was the same as it was always crashing on onAppear on a tabview but it's something else. It's crashing on bad memory access. After reducing the size of my struct it now works again. Please ignore my comments my issue has nothing to do with this one.

hfossli-agens commented 3 years ago

I have created a ponyfill replacement for TabView which solves all the lifecycle issues (yes, there are more issues).

https://gist.github.com/hfossli/f93cb5d1fdbe311b5a1bcbdd2a04cf0f

hfossli-agens commented 3 years ago

I wrote about this topic here https://hfossli.medium.com/lifecycle-bugs-in-swiftuis-tabview-c130b79d1cb

stephencelis commented 3 years ago

Doing some old issue cleanup and since this is a bug in SwiftUI I'm going to convert to a discussion, where the information/workarounds may be helpful to others.