Route.root should probably use .push instead of .sheet

[zntfdr]

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

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<Screen> = [.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:

FlowStacks/Sources/FlowStacks/Combined/Route.swift

Lines 21 to 25 in 6d04318

	/// The root of the stack. The presentation style is irrelevant as it will not be presented.
	/// - Parameter screen: the screen to be shown.
	public static func root(_ screen: Screen, embedInNavigationView: Bool = false) -> Route {
	return .sheet(screen, embedInNavigationView: embedInNavigationView)
	}

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

FlowStacks/Sources/FlowStacks/Combined/Array+RouteProtocol.swift

Lines 7 to 18 in 6d04318

	var canPush: Bool {
	for route in self.reversed() {
	switch route.style {
	case .push:
	continue
	case .cover(let embedInNavigationView), .sheet(let embedInNavigationView):
	return embedInNavigationView
	}
	}
	// We have to assume that the routes are being pushed onto a navigation view outside this array.
	return true
	}

Workarounds for the example above:

• replace the routes definition with: @State private var routes: Routes<Screen> = [.push(.firstScreen)] (where I replaced .root(.firstScreen) with .push(.firstScreen)).
• like above, but replace .root(.firstScreen) with .root(.firstScreen, embedInNavigationView: true), however that would not work for child coordinators (and would embed another NavigationStack to the current one).

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]

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]

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]

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]

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

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<Screen> = [.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]

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:

FlowStacks/Sources/FlowStacks/Combined/Node.swift

Lines 84 to 88 in c000def

	screenView
	.background(
	NavigationLink(destination: next, isActive: pushBinding, label: EmptyView.init)
	.hidden()
	)

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:

FlowStacks/Sources/FlowStacks/Combined/Node.swift

Lines 102 to 110 in c000def

	if route?.embedInNavigationView ?? false {
	NavigationView {
	unwrappedBody
	}
	.navigationViewStyle(supportedNavigationViewStyle)
	} else {
	unwrappedBody
	}
	}

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]

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

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<Screen> = [.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<SheetScreen> = [.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]

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]

Thank you! 🙌🏻