johnpatrickmorgan / FlowStacks

FlowStacks allows you to hoist SwiftUI navigation and presentation state into a Coordinator
MIT License
854 stars 66 forks source link

Nested coordinator can't open more than one screen #76

Closed lisindima closed 3 months ago

lisindima commented 5 months ago

Hi, I found another bug with nested coordinators. A nested coordinator can't open more than one screen, when trying to call a transition - nothing happens, and if you close the open screen after that - navigation will be broken.

Example code that reproduces the behavior:

import SwiftUI
import FlowStacks

enum Path: Hashable {
    case sport(Int, SecondCoordinator)
}

enum NewPath: Hashable {
    case challenge(String, SecondCoordinator)
}

class FirstCoordinator: ObservableObject {
    @Published var path: Routes<Path> = []

    func push() {
        path.push(.sport(1, .init(self)))
    }
}

struct ContentView: View {
    @ObservedObject var coordinator: FirstCoordinator

    var body: some View {
        FlowStack($coordinator.path, withNavigation: true) {
            VStack {
                Button {
                    coordinator.push()
                } label: {
                    Text("Push")
                }
            }
            .navigationTitle("ROOT")
            .flowDestination(for: Path.self) { screen in
                switch screen {
                case let .sport(value, coordinator):
                    SportView(coordinator: coordinator, value: value)
                }
            }

        }
    }
}

class SecondCoordinator: ObservableObject {
    private let parent: FirstCoordinator
    private let id = UUID()

    @Published var path: Routes<NewPath> = []

    init(_ parent: FirstCoordinator) {
        self.parent = parent
    }

    func push() {
        path.push(.challenge("2", self))
    }
}

extension SecondCoordinator: Hashable, Equatable {
    static func == (lhs: SecondCoordinator, rhs: SecondCoordinator) -> Bool {
        lhs.id == rhs.id
    }

    func hash(into hasher: inout Hasher) {
        hasher.combine(id)
    }
}

struct SportView: View {
    @ObservedObject var coordinator: SecondCoordinator

    let value: Int

    var body: some View {
        FlowStack($coordinator.path) {
            VStack {
                Button {
                    coordinator.push()
                } label: {
                    Text("push")
                }
                Text(value.description)
            }
            .navigationTitle("Sport")
            .flowDestination(for: NewPath.self) { screen in
                switch screen {
                case let .challenge(value, coordinator):
                    ChallengeView(coordinator: coordinator, value: value)
                }
            }
        }
    }
}

struct ChallengeView: View {
    @ObservedObject var coordinator: SecondCoordinator

    let value: String
    var body: some View {
        VStack {
            Text(value)
            Button {
                coordinator.push()
            } label: {
                Text("challenge")
            }
        }
        .navigationTitle("Challenge")
    }
}

I also attach a recording of the behavior https://github.com/johnpatrickmorgan/FlowStacks/assets/43087859/557012be-0619-4838-af8f-a4bf5d63121b

lisindima commented 4 months ago

@johnpatrickmorgan hi, do you have any ideas or suggestions?)

johnpatrickmorgan commented 4 months ago

Hi @lisindima, thanks for raising this. Sorry for the delay - I've been looking into it, and I'm a bit puzzled so far. I'll give more info about my findings so far on Monday. Thanks!

johnpatrickmorgan commented 4 months ago

Hi, here's a quick update:

FlowStack holds onto the binding passed in to its initialiser (in its externalTypedPath property) and ensures any changes made to it get propagated to its own internal source of truth - its path property. It does so via a call to onChange(of: ). The issue seems to arise because changes to the externalTypedPath are triggering the FlowStack to recompute its body, but the onChange closure is for some reason not getting called. Adding _printChanges shows that the mutation causes the FlowStack's body to be recomputed (@self, _externalTypedPath changed is printed), but the onChange(of: _externalTypedPath) closure is not called. I've been trying to figure out why but I haven't found anything concrete so far.

Along the way I did notice a simpler issue that was causing FlowLinks not to work within a nested FlowStack, so I put in a fix for that in v0.6.3.

lisindima commented 4 months ago

Thanks for your investigation, I also found one rather dirty hack, but it works well so far.

Use withNavigation: true in the nested coordinator. This will result in duplication of the NavigationBar, but this can be fixed with navigationViewModifier and we get this code:

struct SportView: View {
    @ObservedObject var coordinator: SecondCoordinator

    let value: Int

    var body: some View {
        FlowStack($coordinator.path, withNavigation: true, navigationViewModifier: NavigationBarHidden()) {
            VStack {
                Button {
                    coordinator.push()
                } label: {
                    Text("push")
                }
                Text(value.description)
            }
            .navigationTitle("Sport")
            .flowDestination(for: NewPath.self) { screen in
                switch screen {
                case let .challenge(value, coordinator):
                    ChallengeView(coordinator: coordinator, value: value)
                }
            }
        }
    }
}

struct NavigationBarHidden: ViewModifier {
   func body(content: Content) -> some View {
        content
            .navigationBarHidden(true)
    }
}

Maybe this will help you)

lisindima commented 4 months ago

with this hack there was a problem, on iOS 15 onAppear is called twice, without this hack the problem is similar - the nested coordinater is not able to open more than one screen.

also this hack doesn't work with FlowPath()

@johnpatrickmorgan hi, is there any progress on this issue?

lisindima commented 4 months ago

@johnpatrickmorgan I don't want to seem intrusive, but my team is very much blocked by this bug, we encountered this after almost completely rewriting the application to this library, perhaps there is some progress with fixing this error?

johnpatrickmorgan commented 4 months ago

@lisindima Sorry for the slow progress; I'm struggling to find a solution. In the meantime, some workarounds would be:

I imagine that none of these are quite what you're hoping for, but they might help you for the time being. Thanks!

johnpatrickmorgan commented 3 months ago

Thanks for your patience. I now understand the problem at least: the onChange in the child FlowStack doesn't fire if it's no longer onscreen. The solution will require a bit of refactoring.

phamdinhduc795397 commented 3 months ago

@johnpatrickmorgan I encountered this problem while creating a child FlowStack. I hope you resolve this issue quickly.

johnpatrickmorgan commented 3 months ago

Thanks for your patience. I believe this issue should now be resolved in v0.7.0, but I'd appreciate your feedback @lisindima @phamdinhduc795397. I had to amend the way nesting FlowStacks works, but it shouldn't affect your use case - please see the amended docs.

phamdinhduc795397 commented 3 months ago

@johnpatrickmorgan In v0.7.0, use withNavigation: true in presenting child FlowStack was not working. It only works if called routes.presentCover(.child, withNavigation: true)

johnpatrickmorgan commented 3 months ago

@johnpatrickmorgan In v0.7.0, use withNavigation: true in presenting child FlowStack was not working. It only works if called routes.presentCover(.child, withNavigation: true)

Thanks @phamdinhduc795397 - if I understand correctly, that's the intended behaviour. When nesting FlowStacks, the parent FlowStack determines whether the child FlowStack is presented with navigation, and the withNavigation parameter passed to the child FlowStack is ignored. I'll clarify that in the docs, thanks!

phamdinhduc795397 commented 3 months ago

`struct ParentCoordinator: View { enum Screen: Hashable { case child }

@State private var routes: Routes<Screen> = []

var body: some View {
    FlowStack($routes, withNavigation: true) {
        rootView()
        .flowDestination(for: Screen.self) { screen in
            switch screen {
            case .child:
                ChildCoordinator()
            }
        }
    }
}

@ViewBuilder
func rootView() -> some View {
    Button("present child") {
        routes.presentCover(.child)
    }
}

}`

`struct ChildCoordinator: View { enum Screen: Hashable {} @State private var routes: Routes = []

var body: some View {
    FlowStack($routes, withNavigation: true) {
        Text("ChildCoordinator")
            .navigationTitle("ChildCoordinator")
    }
}

} Use FlowStack($routes, withNavigation: true) in child ChildCoordinator,withNavigation: truewill be ignored, except you passwithNavigation: truewithroutes.presentCover(.child, withNavigation: true)`. In v0.6.4, it works well.

lisindima commented 3 months ago

Спасибо за ваше терпение. Я считаю, что эта проблема должна быть решена в v0.7.0, но я был бы признателен за ваш отзыв@lisindima @phamdinhduc795397Мне пришлось изменить способ работы вложенных FlowStacks, но это не должно повлиять на ваш вариант использования — см. измененную документацию .

Hi, the error with which I opened this problem is fixed in the new version, thank you