Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SwiftUI's View.navigate() + CaseStudy #20

Closed
wants to merge 1 commit into from

Conversation

jtouzy
Copy link

@jtouzy jtouzy commented Jun 2, 2022

SwiftUI mechanism for Navigations can be weird for good old UIKit developers.

And for those who wants to make all their Views isolated, and make them not aware of any navigation context, it can be hard to achieve this with the current SwiftUI implementation.

So, that's my reflexions here :

A basic navigation is used like this :

struct CustomView: View {
  var body: some View {
    VStack {
      // ...View contents
      NavigationLink("MyNavigationButton") {
        MyDestinationView()
      }
      // ...View contents
    }
  }
}

That NavigationLink must be in the view that makes navigation. But what if we want to make our Custom view completely isolated from Navigation context ? For example, the navigation is triggered by some Button, but we don't want to do this directly via NavigationLink, because it can have multiple destinations, depending on app context.

So, swiftui-navigation is a good start for this. We can use pointfree's amazing tools as a start. Using the new inits with unwrapping / case parameters and a Route enum, we can easily drive the navigation.

But there's the last problem : Our view still knows about the NavigationLink.

Here's the solution : we can make a custom ViewModifier to wrap that binding-controlled NavigationLink in a new Custom view, which automatically embeds your CustomView, with the desired NavigationLink. As the NavigationLink view is an empty view, we can easily hides it with the modifier (see the implementation in PR).

So then, we can have something like this :

struct MyApp: App {
  var body: some View {
    CustomView()
      .navigate(when: $route, is: /Route.destination) { _ in
        MyDestination()
      }
  }
}

For a more complete example, look at the 10 CaseStudy included in the PR.
This is a first draft for this feature. Sure it can be improved.

@stephencelis
Copy link
Member

stephencelis commented Jun 2, 2022

Hi @jtouzy! Thanks for the PR.

This pattern seems to regularly appear in the iOS dev community, and it springs out of a very real need. Unfortunately, the problem is a bit thornier than it may first seem. In particular, EmptyView isn't really as empty as many folks think. If someone has the "button shapes" accessibility setting turned on, they'll see tiny boxes wherever these otherwise "invisible" navigation links appear in the view hierarchy. For example, in the demo on this branch:

image

Another issue with SwiftUI navigation links is they can only activate if they're rendered onscreen. So if you have a scroll view and your programmatic navigation happens when the hidden link is offscreen, it won't work (we believe this is a general SwiftUI navigation bug, to be fair, but think it means the underlying view hierarchy of such a solution needs to be carefully considered).

And there are probably more issues lurking in the shadows 😨

There may be workarounds to these issues and more, but it may also point to the fact that Apple's APIs weren't designed to be used in this way.

I think for the purpose of this library, we're a bit worried to introduce APIs that stray too much from Apple's, as we can't anticipate all of the gotchas hiding around the corner, and we also don't want to put users of our library in a bad spot during OS upgrades (at least not one that Apple has already put us in 😅). I think the real path forward is to continue to file feedback with Apple requesting additional, sanctioned APIs for programmatic navigation in SwiftUI, and to report issues with the existing navigation APIs.

In the meantime, you should still be able to use SwiftUI Navigation's NavigationLinks in workarounds you're comfortable with in your own apps.

Let us know what you think 😄

@jtouzy
Copy link
Author

jtouzy commented Jun 2, 2022

Ok, I understand the point of view, and your opinion to not want to stray too much from Apple.

I didn't even know that ButtonShape option draws around EmptyView... I think it's not really related to EmptyView, but the OS considers that NavigationLink has still a button as child, with nothing inside.
To fix ButtonShape, maybe we can do a little trick with opacity or something else. I'll check.

Thanks for taking the time to make the detailed answer ;-)

@jtouzy jtouzy closed this Jun 2, 2022
@stephencelis
Copy link
Member

@jtouzy If you do end up figuring out the button shape issue, please do share your findings with us!

Another thought is that you could bundle this functionality and example into its own library that folks can use. If you do ship a library that layers this functionality on top of SwiftUI Navigation, we'd definitely be interested to see it and even link to it from the README. Just let us know 😄

@jtouzy
Copy link
Author

jtouzy commented Jun 6, 2022

@stephencelis Hope you've seen that! .navigationDestination()!

@stephencelis
Copy link
Member

@jtouzy We have 😄 Still digesting and need to experiment a lot, though!

@jtouzy
Copy link
Author

jtouzy commented Jun 7, 2022

Yesterday night I've tested the basics, and it seems WAY better to isolate components :

https://twitter.com/jtouzy/status/1534040202305384448?s=21&t=BYf_tAtd5dVXsXIu1MOnIg

@mbrandonw
Copy link
Member

Hey @jtouzy, thanks for sharing!

In our minds, the big unknown is what do you do here:

image

We like navigation to be driven by state so that we can hand a binding or scoped store down to the child view, but it's not clear how to do that with the new API. Looking through Apple's food truck sample app doesn't make anything clearer unfortunately.

@jtouzy
Copy link
Author

jtouzy commented Jun 7, 2022

Ok I see the point. It's just basic tests even before the videos 🙃

For example, in our apps, we prefer to have isolated components which are unaware of any navigation, and then, a parent module who drives the navigation.

In this case, this is perfect: we just drive with the parent, and the child views simply trigger delegate actions to the parent, who drives it.

@lukeredpath
Copy link

@jtouzy even if you take that approach, you're still left a bit stuck with the new NavigationLink APIs - lets say you have some root NavigationPath in your AppState and you handle all of the destinations in your root view - how do your child views communicate with TCA to manipulate the path? NavigationLink only lets you send values directly to the stack - there's no way to get it to send the value to the TCA store first.

@jtouzy
Copy link
Author

jtouzy commented Jun 7, 2022

@lukeredpath You don't need NavigationLink's to make it work. At least, with tests i've made (didn't look videos yet).
If you look at my Twitter post example, you'll never notice any NavigationLink. Just an action send to the reducer to trigger a view change, which the parent view can handle.

In this example i use a single module, but you can imagine embedded children, it's the same principle.

import ComposableArchitecture
import SwiftUI

enum Route {
  case second
  case third
}

struct State: Equatable {
  var navigation: NavigationPath
}

enum Action {
  case actionTapped
  case navigationPathChanged(NavigationPath)
  case secondPageTapped
}

let reducer: Reducer<State, Action, Void> = .init { state, action, _ in
  switch action {
  case .actionTapped:
    state.navigation.append(Route.second)
  case .navigationPathChanged(let navigation):
    state.navigation = navigation
  case .secondPageTapped:
    state.navigation.append(Route.third)
  }
  return .none
}

struct View: SwiftUI.View {
  let store: Store<State, Action>

  var body: some SwiftUI.View {
    WithViewStore(store) { viewStore in
      NavigationStack(
        path: viewStore.binding(get: \.navigation, send: Action.navigationPathChanged)
      ) {
        Button(
          "Click me to navigate", action: { viewStore.send(.actionTapped) }
        )
        .navigationDestination(for: Route.self, destination: { route in
          switch route {
          case .second:
            Button("Second page", action: { viewStore.send(.secondPageTapped) })
          case .third:
            Text("Third page")
          }
        })
        .navigationTitle("Navigation")
      }
    }
  }
}

@lukeredpath
Copy link

lukeredpath commented Jun 7, 2022

@jtouzy this isn't a realistic example of a TCA app though. In a TCA app, we don't pass generally pass around simple values. We pass around either a scoped store or a binding. This normally means we have some optional child state that we need to initialize before navigating.

Unfortunately NavigationPath has no APIs for introspecting the values in the path, so its impossible to respond to changes in the path in any meaningful way. Also, the fact of the matter is if you want to present a list of links in a list with the correct UI accessory and behaviour, you really need to be using a NavigationLink. Additionally, the path will change when the user interacts with the back button. In both of these cases there's no way to know what has changed in the path in order to update your app state.

I'm not even sure this problem is TCA-specific. I can also see similar issues when trying to encapsulate your app state inside view models and needing to route from one view to another and pass a child view model along.

@jtouzy
Copy link
Author

jtouzy commented Jun 7, 2022

The path you use as binding doesn't have to be a NavigationPath. A NavigationPath is required when you use multiple data types in the full path. It can be a basic collection of Routes, in this case, because we describe the route as an enum.

I've completed with a "more realistic" example with an optional navigation. It comes with some boilerplate for arrays and some problems to think (for example same view multiple times in stack), but it can be done.

import CasePaths
import ComposableArchitecture
import SwiftUI

struct Main {
  enum Route: Hashable {
    case second(SecondPage.State)
  }
  struct State: Equatable {
    var navigation: [Route]

    var secondPage: SecondPage.State? {
      get {
        let route = navigation.first(where: { element in
          guard case .second = element else { return false }
          return true
        })
        return (/Route.second).extract(from: route)
      }
      set {
        guard
          let newValue,
          let routeIndex = navigation.firstIndex(where: { element in
            guard case .second = element else { return true }
            return false
          })
        else {
          return
        }
        navigation[routeIndex] = .second(newValue)
      }
    }
  }
  enum Action {
    case actionTapped
    case navigationPathChanged([Route])
    case secondPage(SecondPage.Action)
  }
  static let localReducer: Reducer<State, Action, Void> = .init { state, action, _ in
    switch action {
    case .actionTapped:
      let route = Main.Route.second(.init(content: "Content"))
      state.navigation.append(route)
    case .navigationPathChanged(let navigation):
      state.navigation = navigation
    case .secondPage:
      ()
    }
    return .none
  }
  static let reducer: Reducer<State, Action, Void> = .combine(
    SecondPage.reducer.optional().pullback(
      state: \.secondPage,
      action: /Action.secondPage,
      environment: { _ in }
    ),
    localReducer
  )
  struct View: SwiftUI.View {
    let store: Store<State, Action>

    var body: some SwiftUI.View {
      WithViewStore(store) { viewStore in
        NavigationStack(
          path: viewStore.binding(get: \.navigation, send: Action.navigationPathChanged)
        ) {
          Button(
            "Click me to navigate", action: { viewStore.send(.actionTapped) }
          )
          .navigationDestination(for: Route.self, destination: { route in
            switch route {
            case .second:
              IfLetStore(
                store.scope(
                  state: \.secondPage,
                  action: Action.secondPage
                )
              ) { store in
                SecondPage.View(store: store)
              }
            }
          })
          .navigationTitle("Navigation")
        }
      }
    }
  }
}

struct SecondPage {
  struct State: Equatable, Hashable {
    var content: String
  }
  enum Action {
    case none
  }
  static let reducer: Reducer<State, Action, Void> = .empty
  struct View: SwiftUI.View {
    let store: Store<State, Action>

    var body: some SwiftUI.View {
      WithViewStore(store) { viewStore in
        Text("Second page \(viewStore.content)")
      }
    }
  }
}

@lukeredpath
Copy link

lukeredpath commented Jun 7, 2022

You're right, you don't have to use NavigationPath if you can work with a single homogenous list of values to represent your navigation path, but doing so forces you to define the navigation destination for the entire navigation stack in the root view, whereas it makes much more sense to me that individual child views know how to navigate to their children and so on, rather than having one giant switch for my entire navigation stack in the root.

Right now the only option for heteregenous navigation values (which is the only type that supports composed navigation destinations) is NavigationPath.

In your example, I'm curious to know how you'd handle a route from SecondPage to ThirdPage.

@jtouzy
Copy link
Author

jtouzy commented Jun 7, 2022

To be clear, the reflexion is made as the same time i'm writing 😄 It's just pure discovering of the new APIs for now.

I think it's just two different points of view tho : in my opinion, it's better to have a dedicated "manager" for navigation. Like I said in previous messages, each child should be independent and reusable, and should not be aware of any navigation, just emits some events at some points.

Like, if you take a good old UIKit, in most cases your modules (in, for example, an MVP architecture) should not be aware about other modules. In most times, a Router is here to be dedicated to UIKit navigation.

I also understand that in this point of view, it's for maximum reusability of components for multiple apps/situations, but in a more simple app, it's a bit overwhelming.

I'm gonna check for the third view, and maybe a better optimized way for arrays. Like I said, it's discovery time!

@lukeredpath
Copy link

I can see how some kind of dedicated router/navigation thing makes sense in UIKit but to me that pattern doesn't make a whole lot of sense in SwiftUI, which is largely state-driven and designed as a tree, as is the TCA app state.

@davdroman
Copy link

davdroman commented Jun 9, 2022

Agreed. From the moment I saw these new APIs I knew they weren't going to be straightforward to apply to TCA.

I think the main reason for this is that NavigationPath is a concrete struct, instead of a protocol. This lack of abstraction forces the state to always follow an array-like structure, which makes things like enum-driven navigation quite unwieldy.

Truly a "square peg in a round hole" situation.

@RemiBardon
Copy link

Hello @davdroman, I was lurking for information about this new API and how it could solve some issues with NavigationViews. I just wanted to ask if you have sent a feedback to the SwiftUI team to maybe change this or provide an alternative?

@jtouzy
Copy link
Author

jtouzy commented Jun 9, 2022

@lukeredpath FYI, an example with 3 views.
The Main view still acts as the principal router. So, his role is to be aware of all navigation contents.
If you need to pass informations from view 2 to 3, you can still have them in the app.
The problem I can see is still the same : For the same view multiple times in the navigation, this solution does not fit (first).

import CasePaths
import ComposableArchitecture
import SwiftUI

// NOTE: Utility for quicker state scopes
extension Array where Element == Main.Route {
  func find<Value>(_ casePath: CasePath<Main.Route, Value>) -> Value? {
    compactMap { element in
      guard let value = casePath.extract(from: element) else { return nil }
      return value
    }.first
  }
  mutating func update<Value>(_ casePath: CasePath<Main.Route, Value>, with value: Value)
  where Value: Equatable {
    guard
      let routeIndex = firstIndex(where: { element in
        guard casePath.extract(from: element) != .none else { return true }
        return false
      })
    else {
      return
    }
    self[routeIndex] = casePath.embed(value)
  }
}

struct Main {
  enum Route: Hashable {
    case second(SecondPage.State)
    case third(ThirdPage.State)
  }
  struct State: Equatable {
    var navigation: [Route]

    var secondPage: SecondPage.State? {
      get { navigation.find(/Route.second) }
      set { newValue.map { navigation.update(/Route.second, with: $0) } }
    }
    var thirdPage: ThirdPage.State? {
      get { navigation.find(/Route.third) }
      set { newValue.map { navigation.update(/Route.third, with: $0) } }
    }
  }
  enum Action {
    case actionTapped
    case navigationPathChanged([Route])
    case secondPage(SecondPage.Action)
    case thirdPage(ThirdPage.Action)
  }
  static let localReducer: Reducer<State, Action, Void> = .init { state, action, _ in
    switch action {
    case .actionTapped:
      let route = Main.Route.second(.init(content: "Content"))
      state.navigation.append(route)
    case .navigationPathChanged(let navigation):
      state.navigation = navigation
    case .secondPage(let secondPageAction):
      switch secondPageAction {
      case .didNavigateOnThirdPage:
        let route = Main.Route.third(.init(content: "Content3"))
        state.navigation.append(route)
      }
    case .thirdPage:
      ()
    }
    return .none
  }
  static let reducer: Reducer<State, Action, Void> = .combine(
    SecondPage.reducer.optional().pullback(
      state: \.secondPage,
      action: /Action.secondPage,
      environment: { _ in }
    ),
    ThirdPage.reducer.optional().pullback(
      state: \.thirdPage,
      action: /Action.thirdPage,
      environment: { _ in }
    ),
    localReducer
  )
  struct View: SwiftUI.View {
    let store: Store<State, Action>

    var body: some SwiftUI.View {
      WithViewStore(store) { viewStore in
        NavigationStack(
          path: viewStore.binding(get: \.navigation, send: Action.navigationPathChanged)
        ) {
          Button(
            "Click me to navigate", action: { viewStore.send(.actionTapped) }
          )
          .navigationDestination(for: Route.self, destination: { route in
            switch route {
            case .second:
              IfLetStore(
                store.scope(
                  state: \.secondPage,
                  action: Action.secondPage
                )
              ) { store in
                SecondPage.View(store: store)
              }
            case .third:
              IfLetStore(
                store.scope(
                  state: \.thirdPage,
                  action: Action.thirdPage
                )
              ) { store in
                ThirdPage.View(store: store)
              }
            }
          })
          .navigationTitle("Navigation")
        }
      }
    }
  }
}

struct SecondPage {
  struct State: Equatable, Hashable {
    var content: String
  }
  enum Action {
    case didNavigateOnThirdPage
  }
  static let reducer: Reducer<State, Action, Void> = .empty
  struct View: SwiftUI.View {
    let store: Store<State, Action>

    var body: some SwiftUI.View {
      WithViewStore(store) { viewStore in
        Button("Second Page - Go tho third page") {
          viewStore.send(.didNavigateOnThirdPage)
        }
      }
    }
  }
}

struct ThirdPage {
  struct State: Equatable, Hashable {
    var content: String
  }
  enum Action {
    case none
  }
  static let reducer: Reducer<State, Action, Void> = .empty
  struct View: SwiftUI.View {
    let store: Store<State, Action>

    var body: some SwiftUI.View {
      WithViewStore(store) { viewStore in
        Text("Third page")
      }
    }
  }
}

@mbrandonw
Copy link
Member

Hey everyone, there's been some really great discussion in here, but it's a bummer it's "trapped" in a closed PR. How about we move any future discussion over to this thread I just started over on TCA?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants