johnpatrickmorgan / FlowStacks

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

`Route.root` should probably use `.push` instead of `.sheet` #10

Closed zntfdr closed 2 years ago

zntfdr commented 2 years ago

I haven't explored/experimented thoroughly the new 0.1.x release, but I believe I've found an issue.

Click to see reproducible example ```swift enum Screen { case firstScreen case secondScreen } struct ContentView: View { var body: some View { NavigationView { FlowCoordinator(onCompletion: { print("end") }) } } } struct FlowCoordinator: View { @State private var routes: Routes = [.root(.firstScreen)] var onCompletion: () -> Void var body: some View { Router($routes) { screen, _ in switch screen { case .firstScreen: FirstScreen(onCompletion: { routes.push(.secondScreen) }) case .secondScreen: SecondScreen(onCompletion: onCompletion) } } } } struct FirstScreen: View { var onCompletion: () -> Void var body: some View { Button("go to second", action: onCompletion) } } struct SecondScreen: View { var onCompletion: () -> Void var body: some View { Button("complete", action: onCompletion) } } ```

The example above will crash as soon as we try to push to the second screen.

Looking at the FlowStacks codebase, I believe the following definition should use/return push instead of sheet:

https://github.com/johnpatrickmorgan/FlowStacks/blob/6d0431834e8ebedbdfc8cf4e86fedb22c44b7717/Sources/FlowStacks/Combined/Route.swift#L21-L25

Otherwise the canPush will return false in the example above, and trigger an assertion.

https://github.com/johnpatrickmorgan/FlowStacks/blob/6d0431834e8ebedbdfc8cf4e86fedb22c44b7717/Sources/FlowStacks/Combined/Array%2BRouteProtocol.swift#L7-L18

Workarounds for the example above:

I'm curious to know if there are reasons that I didn't think of behind using .sheet for the .root definition. If there are none and this is indeed a bug, I'm happy to create a PR with the change if needed.

Thank you in advance!

johnpatrickmorgan commented 2 years ago

Hi Federico, thanks for checking out the latest release!

I had intially imagined that child coordinators would initialise their routes with a .push instead of a .root (your first workaround suggestion), but I can see how that's not at all intuitive, as .root looks more like the correct approach.

For most purposes it doesn't matter what case the root is, but I set it to .sheet so that it has the option to embedInNavigationView. I thought the canPush assertion would be a helpful addition, but I realise now that it doesn't really work for pushed child coordinators.

I think the best solution is to allow some ambiguity in the canPush logic, to allow for cases where it's possible a parent coordinator is responsible for the NavigationView (#11). It makes the canPush assertion a little less useful, but seems like a decent compromise. Does that sound good to you?

Thanks for raising this issue!

zntfdr commented 2 years ago

Hi John, many thanks for the prompt response!

After thinking about it a little bit more, I must say that I wouldn't mind leaving the responsibility of adding NavigationViews and making sure that a view can push, entirely to the developer.

This is similar to what SwiftUI does: if we try to push in a view that is not embedded in a NavigationView, it just silently fails.

My suggestion would be to remove all the embedInNavigationView and canPush logic/checks. This would also result in a smaller library that focuses on the core functionality.

Thank you again!

johnpatrickmorgan commented 2 years ago

Thanks Federico, I agree it would be good to remove the responsibility of embedding within a NavigationView from the library. Unfortunately, it's not possible to do that with a mixed presentation and navigation flow.

The latest release combines both PStack and NStack into a single Router, which is capable of managing both presentation and navigation within a mixed flow. In order for that to work, the library has to take responsibility for the NavigationView - the NavigationView must be created within the Node, so that it wraps any subsequently pushed screens, e.g. in the case where a screen is presented and then another screen is pushed. Hopefully the benefits of mixed flows make this extra responsibility worthwhile.

zntfdr commented 2 years ago

As a counter-example, the code below now crashes (because of canPush) when we attempt to push to the third screen:

Click to see reproducible example ```swift struct ContentView: View { var body: some View { NavigationView { FlowCoordinator(onCompletion: { print("end") }) } } } enum Screen { case firstScreen case secondScreen case thirdScreen } struct FlowCoordinator: View { @State private var routes: Routes = [.push(.firstScreen)] var onCompletion: () -> Void var body: some View { Router($routes) { screen, _ in switch screen { case .firstScreen: FirstScreen(onCompletion: { routes.presentSheet(.secondScreen) }) case .secondScreen: NavigationView { SecondScreen(onCompletion: { routes.push(.thirdScreen) }) } case .thirdScreen: ThirdScreen(onCompletion: onCompletion) } } } } struct FirstScreen: View { var onCompletion: () -> Void var body: some View { Button("go to second", action: onCompletion) } } struct SecondScreen: View { var onCompletion: () -> Void var body: some View { Button("go to third", action: onCompletion) } } struct ThirdScreen: View { var onCompletion: () -> Void var body: some View { Button("complete", action: onCompletion) } } ```

In case a flow would require a screen to be presented both within a sheet and within a navigation stack, at that point, I would create two "screen" cases, e.g.:

enum Screen {
  case ...
  case secondScreen
  case secondScreenWithNav
}

Where secondScreenWithNav will be used to return the same view embedded in a navigation, e.g.:

Router($routes) { screen, _  in
  switch screen {
    ...
    case .secondScreen:
        SecondScreen(onCompletion: { routes.push(.thirdScreen) })
    case .secondScreenWithNav:
      NavigationView {
        SecondScreen(onCompletion: { routes.push(.thirdScreen) })
      }
}

Unless I miss something, I believe this would cover what you mentioned. Please let me know what you think :)

johnpatrickmorgan commented 2 years ago

Thanks for clarifying @zntfdr! Sadly this approach doesn't work, because the NavigationLinks that Node inserts would be outside the NavigationView provided by the screenView:

https://github.com/johnpatrickmorgan/FlowStacks/blob/c000def23548f58cea7c78f47ca47c840f749d17/Sources/FlowStacks/Combined/Node.swift#L84-L88

It would end up with a hierarchy like:

NavigationView {
  SecondScreen(onCompletion: { routes.push(.thirdScreen) })
}
.background( 
  NavigationLink(destination: next, isActive: pushBinding, label: EmptyView.init).hidden() 
)

rather than:

NavigationView {
  SecondScreen(onCompletion: { routes.push(.thirdScreen) })
    .background( 
      NavigationLink(destination: next, isActive: pushBinding, label: EmptyView.init).hidden() 
    )
}

Using the embedInNavigationView flag allows the Node to wrap the whole thing in a NavigationView:

https://github.com/johnpatrickmorgan/FlowStacks/blob/c000def23548f58cea7c78f47ca47c840f749d17/Sources/FlowStacks/Combined/Node.swift#L102-L110

So if the assertion failure was disabled, your example would still silently fail to push the third screen. I don't think there's a nice way to avoid the library managing the NavigationViews for mixed flows, but I'm open to suggestions. :)

zntfdr commented 2 years ago

Thank you for clarifying this for me! You're completely right: I now understand what you mean.

In this case, what I would do is the following (this works with 0.1.1):

See code ```swift struct ContentView: View { var body: some View { NavigationView { FlowCoordinator(onCompletion: { print("end") }) } } } enum Screen { case firstScreen case sheet } struct FlowCoordinator: View { @State private var routes: Routes = [.push(.firstScreen)] var onCompletion: () -> Void var body: some View { Router($routes) { screen, _ in switch screen { case .firstScreen: FirstScreen(onCompletion: { routes.presentSheet(.sheet) }) case .sheet: NavigationView { SheetCoordinator(onCompletion: onCompletion) } } } } } enum SheetScreen { case secondScreen case thirdScreen } struct SheetCoordinator: View { @State private var routes: Routes = [.push(.secondScreen)] var onCompletion: () -> Void var body: some View { Router($routes) { screen, _ in switch screen { case .secondScreen: SecondScreen(onCompletion: { routes.push(.thirdScreen) }) case .thirdScreen: ThirdScreen(onCompletion: onCompletion) } } } } struct FirstScreen: View { var onCompletion: () -> Void var body: some View { Button("go to second", action: onCompletion) } } struct SecondScreen: View { var onCompletion: () -> Void var body: some View { Button("go to third", action: onCompletion) } } struct ThirdScreen: View { var onCompletion: () -> Void var body: some View { Button("complete", action: onCompletion) } } ```

Since we're moving to a sheet with its own navigation, it probably makes sense to have another coordinator anyway.

Now that I've experimented a little more with the framework, I don't mind having to use .push instead of .root for my coordinator "roots".

Perhaps a disclaimer in the .root documentation is all it's needed to close this issue :)

Apologies for the trouble and thank you again!

johnpatrickmorgan commented 2 years ago

Awesome, that approach works well! I'm glad to know the library can still be used in that way.

I think the changes in #11 are still correct, so I think I'll merge that and then .root can be used in those cases too. Thanks for digging deep into this!

zntfdr commented 2 years ago

Thank you! 🙌🏻