Various issues with starting a SubFlow from a Flow, see details below.

[Sajjon]

I've created a small demo app that demonstrates issues with initiating a subflow (child coordinator) from a flow. You can find the git repo for this demo app here, for the sake of convenience I will also provide a "snapshot" of the entire code below (but might get stale if I update the code, so git repo is source of truth.)

This is the flow of the app:

AppCoordinator

            Main <---------------*
          /    |                  \
Splash ->{     {Sign Out}          \
          \   /                     \
            OnboardingCoordinator    \   
                - Welcome             \
                - TermsOfService       \
                - SignUpCoordinator     |
                    - Credentials       | 
                    - PersonalInfo      |
                - SetupPINCoordinator   |
                    - InputPIN         /
                    - ConfirmPIN ->---*

The relevant part is the onboarding flow, which starts with OnboardingView (coordinator) as root, and then pushes the screens and subflows.

Code of whole app

Click to expand

```swift // // DemoFlowStacksApp.swift // DemoFlowStacks // // Created by Alexander Cyon on 2022-04-21. // import SwiftUI import FlowStacks // MARK: - APP @main struct DemoFlowStacksApp: App { var body: some Scene { WindowGroup { VStack { Text("`push`: `SetPIN` backs to `Credentials`") AppCoordinator() .navigationViewStyle(.stack) .environmentObject(AuthState()) } } } } struct User { struct Credentials { let email: String let password: String } struct PersonalInfo { let firstname: String let lastname: String } let credentials: Credentials let personalInfo: PersonalInfo } typealias PIN = String final class AuthState: ObservableObject { @Published var user: User? = nil @Published var pin: PIN? = nil var isAuthenticated: Bool { user != nil } func signOut() { user = nil } public init() {} } // MARK: - App Coord. struct AppCoordinator: View { enum Screen { case splash case main(user: User, pin: PIN?) case onboarding } @EnvironmentObject var auth: AuthState @State var routes: Routes = [.root(.splash)] var body: some View { Router($routes) { screen, _ in switch screen { case .splash: SplashView( toOnboarding: toOnboarding, toMain: toMain ) case let .main(user, pin): MainView(user: user, pin: pin, signOut: signOut) case .onboarding: OnboardingCoordinator(done: onboardingDone) } } } private func signOut() { auth.signOut() toOnboarding() } private func onboardingDone(user: User, pin: PIN?) { toMain(user: user, pin: pin) } private func toOnboarding() { routes = [.root(.onboarding)] } private func toMain(user: User, pin: PIN?) { routes = [.root(.main(user: user, pin: pin))] } } // MARK: - Splash struct SplashView: View { @EnvironmentObject var auth: AuthState var toOnboarding: () -> Void var toMain: (User, PIN?) -> Void var body: some View { ZStack { Color.pink.edgesIgnoringSafeArea(.all) Text("SPLASH").font(.largeTitle) } .onAppear { // Simulate some loading DispatchQueue.main.asyncAfter(deadline: .now() + 0.8) { if let user = auth.user { toMain(user, auth.pin) } else { toOnboarding() } } } } } // MARK: - Main struct MainView: View { let user: User let pin: PIN? let signOut: () -> Void var body: some View { ZStack { Color.blue.opacity(0.65).edgesIgnoringSafeArea(.all) VStack { Text("Hello \(user.personalInfo.firstname)!") Button("Sign out") { signOut() } } } .navigationTitle("Main") } } // MARK: - Onboarding Flow struct OnboardingCoordinator: View { enum Screen { case welcome // Screen case termsOfService // Screen case signUpFlow // Flow of multiple screens and subflows. case setPINflow(User) // Flow of multiple screens } let done: (User, PIN?) -> Void @EnvironmentObject var auth: AuthState @State var routes: Routes = [.root(.welcome)] var body: some View { NavigationView { Router($routes) { screen, _ in switch screen { case .welcome: WelcomeView(start: toTermsOfService) case .termsOfService: TermsOfServiceView(accept: toSignUp) case .signUpFlow: SignUpFlow( routes: [ .push(.initial) // .root(.initial, embedInNavigationView: false) ], signedUpUser: toSetPIN ) .environmentObject(auth) case .setPINflow(let user): SetPINFlow( routes: [ .push(.initial)] // .root(.initial, embedInNavigationView: false)] , user: user, doneSettingPIN: { maybePin in doneSettingUser(user, andPIN: maybePin) } ) .environmentObject(auth) } } } } private func toTermsOfService() { routes.push(.termsOfService) } private func toSignUp() { print("🔮✍🏽 push-ing `signUpFlow`") routes.push(.signUpFlow) } private func toSetPIN(user: User) { print("🔮🔐 push-ing `setPINflow`") routes.push(.setPINflow(user)) } private func doneSettingUser(_ user: User, andPIN pin: PIN?) { done(user, pin) } } // MARK: - Welcome (Onb.) struct WelcomeView: View { var start: () -> Void var body: some View { ZStack { Color.green.opacity(0.65).edgesIgnoringSafeArea(.all) VStack { Button("Start") { start() } } } .buttonStyle(.borderedProminent) .navigationTitle("Welcome") } } // MARK: - Terms (Onb.) struct TermsOfServiceView: View { var accept: () -> Void var body: some View { ZStack { Color.orange.opacity(0.65).edgesIgnoringSafeArea(.all) VStack { Text("We will steal your soul.") Button("Accept terms") { accept() } } } .buttonStyle(.borderedProminent) .navigationTitle("Terms") } } // MARK: - SignUp SubFlow (Onb.) struct SignUpFlow: View { enum Screen { static let initial: Self = .credentials case credentials case personalInfo(credentials: User.Credentials) } @EnvironmentObject var auth: AuthState @State var routes: Routes let signedUpUser: (User) -> Void init( // The sensible default value for `routes` is `.root(.initial)`, making // this flow stand alone testable. However when this flow is a *subflow* // of another flow (e.g. being subflow of Onboarding flow), we want to // set `routes` to `.push(.initial)` instead. routes: Routes = [.root(.initial)], signedUpUser: @escaping (User) -> Void ) { self.routes = routes self.signedUpUser = signedUpUser } var body: some View { Router($routes) { screen, _ in switch screen { case .credentials: CredentialsView(next: toPersonalInfo) case .personalInfo(let credentials): PersonalInfoView(credentials: credentials, done: done) } } } private func toPersonalInfo(credentials: User.Credentials) { routes.push(.personalInfo(credentials: credentials)) } private func done(user: User) { auth.user = user signedUpUser(user) } } // MARK: - Credentials (Onb.SignUp) struct CredentialsView: View { @State var email = "jane.doe@cool.me" @State var password = "secretstuff" private var credentials: User.Credentials? { guard !email.isEmpty, !password.isEmpty else { return nil } return .init(email: email, password: password) } var next: (User.Credentials) -> Void var body: some View { ZStack { Color.yellow.opacity(0.65).edgesIgnoringSafeArea(.all) VStack { TextField("Email", text: $email) SecureField("Password", text: $password) Button("Next") { next(credentials!) }.disabled(credentials == nil) } } .buttonStyle(.borderedProminent) .textFieldStyle(.roundedBorder) .navigationTitle("Credentials") } } // MARK: - PersonalInfo (Onb.SignUp) struct PersonalInfoView: View { @State var firstname = "Jane" @State var lastname = "Doe" let credentials: User.Credentials private var user: User? { guard !firstname.isEmpty, !lastname.isEmpty else { return nil } return .init(credentials: credentials, personalInfo: .init(firstname: firstname, lastname: lastname)) } var done: (User) -> Void var body: some View { ZStack { Color.brown.opacity(0.65).edgesIgnoringSafeArea(.all) VStack { TextField("Firstname", text: $firstname) TextField("Lastname", text: $lastname) Button("Sign Up") { done(user!) }.disabled(user == nil) } } .buttonStyle(.borderedProminent) .textFieldStyle(.roundedBorder) .navigationTitle("Personal Info") } } // MARK: - SetPIN SubFlow (Onb.) struct SetPINFlow: View { enum Screen { static let initial: Self = .pinOnce case pinOnce case confirmPIN(PIN) } @EnvironmentObject var auth: AuthState @State var routes: Routes let user: User let doneSettingPIN: (PIN?) -> Void init( // The sensible default value for `routes` is `.root(.initial)`, making // this flow stand alone testable. However when this flow is a *subflow* // of another flow (e.g. being subflow of Onboarding flow), we want to // set `routes` to `.push(.initial)` instead. routes: Routes = [.root(.initial)], user: User, doneSettingPIN: @escaping (PIN?) -> Void ) { self.routes = routes self.user = user self.doneSettingPIN = doneSettingPIN } var body: some View { Router($routes) { screen, _ in switch screen { case .pinOnce: InputPINView(firstname: user.personalInfo.firstname, next: toConfirmPIN, skip: skip) case .confirmPIN(let pinToConfirm): ConfirmPINView(pinToConfirm: pinToConfirm, done: done, skip: skip) } } } private func toConfirmPIN(pin: PIN) { routes.push(.confirmPIN(pin)) } private func done(pin: PIN) { auth.pin = pin doneSettingPIN(pin) } private func skip() { doneSettingPIN(nil) } } // MARK: - InputPINView (Onb.SetPIN) struct InputPINView: View { let firstname: String var next: (PIN) -> Void var skip: () -> Void @State var pin = "1234" var body: some View { ZStack { Color.red.opacity(0.65).edgesIgnoringSafeArea(.all) VStack { Text("Hey \(firstname), secure your app by setting a PIN.").lineLimit(2) SecureField("PIN", text: $pin) Button("Next") { next(pin) }.disabled(pin.isEmpty) } } .navigationTitle("Set PIN") .toolbar { ToolbarItem(placement: .navigationBarTrailing) { Button("Skip") { skip() } } } .buttonStyle(.borderedProminent) .textFieldStyle(.roundedBorder) } } // MARK: - ConfirmPINView (Onb.SetPIN) struct ConfirmPINView: View { let pinToConfirm: PIN var done: (PIN) -> Void var skip: () -> Void @State var pin = "1234" var body: some View { ZStack { Color.teal.opacity(0.65).edgesIgnoringSafeArea(.all) VStack { SecureField("PIN", text: $pin) if pin != pinToConfirm { Text("Mismatch between PINs").foregroundColor(.red) } Button("Confirm PIN") { done(pin) }.disabled(pin != pinToConfirm) } } .navigationTitle("Confirm PIN") .toolbar { ToolbarItem(placement: .navigationBarTrailing) { Button("Skip") { skip() } } } .buttonStyle(.borderedProminent) .textFieldStyle(.roundedBorder) } } ```

Problems

I have to select one of these scenarios, all suboptimal:

1. Nice code, but bad app experience.
   • Nice UI (but incorrect behaviour of back button)
   • Correct behaviour of back button (but unacceptable UI because of double navigation bar)
2. Messy code, but correct app experience (nice UI and correct back behaviour).

The problem with the back button is that the SignUpFlow itself has multiple screens, and when it finishes and we push the SetupPIN subflow, pressing the back button from any screen in the SetupPIN subflow, we get back to the first screen of the SignUpFlow and not the last one as expected. The result is that we loose the entire state of the SignUpFlow. In this example above that might not be so bad, since it is only two screens of state we lose. But imagine a subflow with many screens this is very problematic.

push(.initial)

This video is a run of the code, we see that after having pressed "Sign up" on "Personal Info" screen we push "SetupPIN" flow, starting with "Set PIN" (InputPINView) screen, the back button in the navigation bar says back to "Credentials", which is wrong, and also pressing it indeed incorrectly takes us back to CredentialsView, but we should really go back to PersonalInfoView.

https://user-images.githubusercontent.com/864410/164883008-e7290c37-dca3-4933-9e4b-065c7d371fb2.mov

Focusing the most relevant piece of the code, this demo is running this code as body inside the OnboardingCoordinator view:

var body: some View {
    NavigationView {
        Router($routes) { screen, _ in
            switch screen {
            case .welcome:
                WelcomeView(start: toTermsOfService)
            case .termsOfService:
                TermsOfServiceView(accept: toSignUp)
            case .signUpFlow:
                SignUpFlow(
                    routes: [
                        .push(.initial)
                    ],
                    signedUpUser: toSetPIN
                )
                .environmentObject(auth)
            case .setPINflow(let user):
                SetPINFlow(
                    routes: [
                        .push(.initial)]
                    ,
                    user: user,
                    doneSettingPIN: { maybePin in
                        doneSettingUser(user, andPIN: maybePin)
                    }
                )
                .environmentObject(auth)
            }
        }
    }
}

We are using push as initial route for SignUpFlow:

...
SignUpFlow(
    routes: [
        .push(.initial)
    ],
    signedUpUser: toSetPIN
)
...

.root(.initial, embedInNavigationView: false)

If we instead do .root(.initial, embedInNavigationView: false). i.e. the body of CoordinatorView is:

var body: some View {
    NavigationView {
        Router($routes) { screen, _ in
            switch screen {
            case .welcome:
                WelcomeView(start: toTermsOfService)
            case .termsOfService:
                TermsOfServiceView(accept: toSignUp)
            case .signUpFlow:
                SignUpFlow(
                    routes: [
                        .root(.initial, embedInNavigationView: false)
                    ],
                    signedUpUser: toSetPIN
                )
                .environmentObject(auth)
            case .setPINflow(let user):
                SetPINFlow(
                    routes: [
                        .root(.initial, embedInNavigationView: false)
                    ],
                    user: user,
                    doneSettingPIN: { maybePin in
                        doneSettingUser(user, andPIN: maybePin)
                    }
                )
                .environmentObject(auth)
            }
        }
    }
}

Focus on:

...
SignUpFlow(
    routes: [
        .root(.initial, embedInNavigationView: false)
    ],
    signedUpUser: toSetPIN
)
...

We get the exact same behaviour:

https://user-images.githubusercontent.com/864410/164883140-f250159e-991e-4bff-bfda-69b2e7474c32.mov

However, if we...

.root(.initial, embedInNavigationView: true)

Specify that we embedInNavigationView, then we get correct behaviour, but double navigation bar, which is of course completely unacceptable. Yes we can hide the first navigation bar, but then we do not get the back to Terms back button in Credentials, which is not OK either.

...
SignUpFlow(
    routes: [
        .root(.initial, embedInNavigationView: true)
    ],
    signedUpUser: toSetPIN
)
...

https://user-images.githubusercontent.com/864410/164883238-7913f2eb-1c47-4473-9967-ceae8c3b31ec.mov

Solution

Is there any solution to this? It feels like this is a bug in FlowStacks? Shouldn't FlowStacks be able to see pop back to the last element of the routes of the last route, i.e. when we press back from SetupPIN, we pop to SignUpFlow(routes: [.root(.credentials), .push(.personalInfo)]) and FlowStack see that that routes [.root(.credentials), .push(.personalInfo)]) contains two elements and would display PersonalInfoView screen!

If you agree and this is a bug hopefully it can be fixed quickly! Otherwise, do you have any workaround for now?

The only work around I can think of is to change the code, to "flatten" it, and let Credential, PersonalInfo, InputPin and ConfirmPIN all be cases of OnboardingCoordinator.Screen, i.e. change from:

struct OnboardingCoordinator: View {
    enum Screen {
        case welcome // Screen
        case termsOfService // Screen
        case signUpFlow // Flow of multiple screens and subflows.
        case setPINflow(User) // Flow of multiple screens
    }
    ...
}

to

struct OnboardingCoordinator: View {
    enum Screen {
        case welcome
        case termsOfService
        case credentials
        case personalInfo
        case inputPIN
        case confirmPIN
    }
    ...
}

But I would really like to avoid that, becaues it reduced the modularity and testability of my app.

[Sajjon]

From README:

Coordinators are just views themselves, so they can be presented, pushed, added to a TabView or a WindowGroup, and can be configured in all the normal ways views can. They can even be pushed onto a parent coordinator's navigation stack, allowing you to break out parts of your navigation flow into separate child coordinators. When doing so, it is best that the child coordinator is always at the top of the parent's routes stack, as it will take over responsibility for pushing and presenting new screens. Otherwise, the parent might attempt to push screen(s) when the child is already pushing screen(s), causing a conflict.

I guess I have implemented something that breaks the guideline

top of the parent's routes stack

But I think it is a very common usecase, at least with TCA, to create subflows and compose an Onboarding flow consisting of multiple flows.

And intuitively as I wrote in the end of the issue description above, it feels like it should be possible.

[johnpatrickmorgan]

Hi @Sajjon, thanks for raising and for such a comprehensive example! :smile:

You are correct that the issue is described (albeit poorly) in that section of the README. To explain in more detail -

FlowStacks takes the routes array and translates it so that the first view contains a navigation link to the second view etc.:

(A)
 A1 -> A2                // Coordinator A's routes

If coordinator A pushes a child coordinator B onto its stack, B can start pushing new routes and it all works as expected:

(A)
 A1 -> A2 -> (B)         // Coordinator A's routes
              B1 -> B2   // Coordinator B's routes

But if coordinator A tries to push a new route on top of that child coordinator which is already pushing its own routes, the following happens:

(A)
 A1 -> A2 -> (B) -> A3
              B1 -> B2 

Coordinator A can't 'reach into' the child coordinator B's stack to insert a navigation link into B2; it can only insert it into the root of the child coordinator's stack. When it does so, it creates a conflict: B1 is now pushing both A3 and B2. This results in the strange behaviour you've described (CredentialsView was pushing both PersonalInfoView and InputPINView).

Once coordinator B is pushed onto A's stack, it takes over responsibility for pushing new screens - A should avoid pushing screens until B is popped off its stack. For multiple coordinators to exist in the same navigation stack, you can instead have coordinator B push the new screen/coordinator, e.g. B could push a new child coordinator C:

(A)
 A1 -> A2 -> (B)       
              B1 -> B2 -> (C)
                           C1 -> C2 ...

So in your example that would mean that the SignUpFlow would push the SetPINFlow itself instead of delegating that to the OnboardingCoordinator. Or as you say, one of the coordinators could flatten its child into its own routes, e.g. SignUpFlow and SetPINFlow could be merged into a single coordinator (you wouldn't have to flatten everything into OnboardingCoordinator in that case).

Hopefully one of those options doesn't stray too far from your preferred structure?

[johnpatrickmorgan]

By the way, I'm also intending to improve the documentation around child coordinators to better describe this limitation!

[Sajjon]

Thanks! For your quick replies :) I appreciate it!

In Coordinator pattern one key point is that Screen A should not need to know about Screen B, their coordinator should! :)

Similarly: child coordinator CC0 should not need to know about CC1.

And why break up any coordinator in multiple child coordinators, all on "same depth", consecutively pushed by their parent coordinator? Why not just skip these extra child coordinators altogether? Well, it really comes down to modularity! One amazing feature using TCA (I use FlowStacks via your other Package TCACoordinators) is the ability to write "Preview Apps", i.e. starting just the SetupPIN flow, without having to click through all screens leading to it! PointFree.co make heavy use if this in Isowords, and so do I. It is an extremely time saving and powerful feature! Which makes writing features so much faster.

So that is why I would like to keep the SetupPIN as a seperate SPM package (which was not clear from my example code above... it is very very simplified), with the inter-screen logic inside the SetuoPINCoordinator, why it needs to be a child coordinator of Onboarding coordinator. And since Onboarding flow composes many of these sub coordinators together, I need to find a solution to my issue above, why SignUp cannot be responsible for pushing SetupPIN (SignUp is located in its own Package which should not depend on or know of SetupPIN).

So I wonder: 1) Do you agree that my use case ought to be supported? 2) Do you believe FlowStacks can support this with appropriate code changes (given answer "YES" to previous question ofc) 3) If I were to make a PR for this, can you point me in some helpful direction? :)

[johnpatrickmorgan]

Thanks @Sajjon I have a better understanding of the problem now. I can see why you want to be able to break down your coordinators in that way.

I'd certainly like the library to support your use case. The current limitation that any child coordinator must be top of its parent's stack is a direct effect of the way SwiftUI manages navigation, so it would require some workarounds.

There are two ways I can think of that might allow your use case to be supported.

Option 1: View injection

The parent could inject a view for a child to push at the end of its flow. Since the type of the view would need to form part of the child's body's type signature, either the view would be erased to an AnyView or the child would have a generic parameter for the type of screen it should push. I don't particularly like this option, as even though the child doesn't know what type of view it's pushing, it still has to know that it's pushing something. It would be preferable if it could just invoke a closure at the end.

Option 2: Coordinator composition

It might be possible to design a more formal way of composing coordinators. The parent's screen type could include those of its children, e.g.:

enum ParentScreen {
  case home
  case child1Screen(Child1Screen)
  case child2Screen(Child2Screen)
}

The parent could then transform a child that operates on [Route<Child1Screen>] into one that can operates instead on [Route<ParentScreen>], so that the parent can inject its routes array into the child for the child to operate on. Or maybe it would be the routes array itself that would be transformed and injected into the child. In any case, for testing the child in isolation in its own module, a simple [Route<Child1Screen>] would be injected. I think this is a better approach, but I think it might add a good deal of complexity to the library, and I might also be overlooking something that would prevent this approach from even working.

Note that I'm toying with a rewrite of Node and the introduction of a Routes object for #21 so if you raise a PR, it would likely require changes to fit with that rewrite.

[johnpatrickmorgan]

Note: #51 includes details of a potential rewrite of the library that should help break down flows between different modules without these limitations.

[ChaosCoder]

I now stumbled across the same problem where I want to push a subcoordinator flow into a flow and after the subcoordinator is done, the parent coordinator should just proceed pushing new screens. I'm using TCACoordinators.

In https://github.com/johnpatrickmorgan/FlowStacks/issues/23#issuecomment-1107660432, you describe the following problematic flow: Coordinator A pushed two screens, then subcoordinator B was pushed on the stack. B pushes two screens (B1 and B2) and then A wants to continue with the flow by pushing A3. The navigation links are imho build like so:

 A1 -> A2 -> (B) -> A3
              ↳ B1 -> B2 

This breaks the linearity a navigation stack needs leading to undefined behavior, because two navigation links are active on one the same screen in the stack.

Idea: What if we flatten the subcoordinator B's routes stack into A's routes stack and build the navigation links based on that?

That would mean building the navigation links like that:

 A1 -> A2 -> (B)       ↱ A3
              ↳ B1 -> B2 

This would mean, that we need to check if a route is a subcoordinator (not sure how we would do that best). Also the coordinator that contains the last element of that flattened routes array is 'active', meaning that B should not push screens after A3 is presented. I think this idea might work but I saw that you said:

Coordinator A can't 'reach into' the child coordinator B's stack to insert a navigation link into B2

Could you elaborate why this wouldn't work?

I'm looking at the Router's body implementation here:

routes
      .enumerated()
      .reversed()
      .reduce(Node<Screen, ScreenView>.end) { nextNode, new in
        let (index, route) = new
        return Node<Screen, ScreenView>.route(
          route,
          next: nextNode,
          allRoutes: $routes,
          index: index,
          buildView: { buildView($0, index) }
        )
      }

The algorithm we need is recursive, every time new is a coordinator (which has a routes array), we need to recursively call a function which enumerates the subcoordinators routes and after reaching the beginning (because it is .reversed()) it automatically proceedes at the parent level again. I think the change would be very small, but I'm not sure how to implement it correctly. @johnpatrickmorgan Would that idea work? And if yes, could you tell me what changes would need to be done? I would be happy to open a PR for that.

[johnpatrickmorgan]

Thanks @ChaosCoder for giving this problem your thought. I think the idea could be made to work, but I don't think it would be a small change, as there are some subtleties to the implementation.

E.g. how would we know that the child view is a child coordinator? We would need to access the child's Router's routes state and buildView closure, so maybe we could call the child view’s body to see if it returns a Router. But Router requires two generic parameters which would be unknown, so I suppose we would have to erase those types. We could instead add some sort of Coordinator protocol that somehow allows the parent to inject additional routes, presumably erased to AnyViews. But what if the parent adds a modifier to the child view, e.g. onAppear? Then it would no longer conform to the Coordinator protocol, so you might want to add conditional conformances for other types too.

As it happens, I've been experimenting with a change that I think would remove this limitation when composing coordinators, based on the approach taken in SwiftUI's NavigationStack. Here's the discussion and the work-in-progress branch, though I haven't implemented the composition yet.

[johnpatrickmorgan]

The new API has now been released and it's now possible for parent and child FlowStacks to share the parent's navigation state, so that either can append to it, albeit with some other limitations. See Approach 1 in the docs.