TCACoordinators icon indicating copy to clipboard operation
TCACoordinators copied to clipboard

Weird push/pop behavior in iOS 14 when embedding a SubCoordinator in a Coordinator in a WithViewStore 😅

Open FredericRuaudel opened this issue 2 years ago • 6 comments

Hi @johnpatrickmorgan !

First, thanks for this great library that saved me some headaches with setting up initial states with complex hierarchy 💯

However, I'd like to raise a weird bug I've found on iOS 14.5 that is not here anymore on iOS 15, but, you know, we need to manage both, at least for a year or so 😅

So I've managed to squeeze the minimum config to reproduce the bug in the sample project attached to this issue.

Here my navigation configuration:

[AppView -> [Pushed1View -> Pushed2View]]

A main Coordinator router that manages an AppView screen and a SubCoordinator screen. And the SubCoordinator router that manages a Pushed1View and a Pushed2View.

The three screen (AppView, Pushed1View and Pushed2View) are displayed in one NavigationView stack using only push actions.

So, here are my use cases:

  1. If I setup the Coordinator directly in my App Window as my root view, everything is fine ! ✅
  2. If I setup the Coordinator inside an AppContainerView which embed the Coordinator inside a WithViewStore, then, when I push Pushed2View, it immediately pop back on iOS14 ❌ but its ok on iOS 15 ✅
  3. If I setup the Coordinator inside an AppContainerView which embed the Coordinator inside a WithViewStore, but I don"t use a SubCoordinator, then, when I push Pushed2View, everything is fine ✅
  4. If I setup the Coordinator directly in my App Window as my root view, but I embed the TCARouter inside a WithViewStore directly in the body of the CoordinatorView, then, when I push Pushed2View, it immediately pop back on iOS14 ❌ but its ok on iOS 15 ✅
  5. If I show Pushed2View as a modal instead of a push, everything is fine too

Hope my explanations are clear enough.

Here is a screen recording to make this more visual too 😉

2022-06-13 17 27 37 - 01

TCAMultiModalStateWithTCACoordinator-bugIOS14.zip

FredericRuaudel avatar Jun 13 '22 15:06 FredericRuaudel

Thanks for raising this issue @FredericRuaudel and for the nice reproduction. I haven't looked too deeply yet, but my hunch is that the WithViewStore is causing unnecessary re-renders of screens that have pushed other screens. I believe on iOS 14, triggering a re-render of a screen that is pushing another screen causes the screen to be popped. If you need to use WithViewStore, you might need to pass it a removeDuplicates closure, so that it doesn't re-render if only the routes have changed. I'll try to look more deeply and be more specific if I can.

johnpatrickmorgan avatar Jun 13 '22 22:06 johnpatrickmorgan

Hi @johnpatrickmorgan !

Thanks for your insight. I've made some additional tests and indeed, it can be solved by tweaking the "Equatability" of the router's State.

Here the solutions I've managed to find to fix this bug:

  1. If I'm in use case 4 above, I can fix the problem by adding a removeDuplicates closure to my router's WithViewStore like so:
WithViewStore(store, removeDuplicates: { $0.routes.count == $1.routes.count }) { viewStore in
      TCARouter(store) { screen in
          // [...]
      }
}

But as soon as I put another WithViewStore in the hierarchy above (eg: put back the AppContainerView), the bug is back 😢

  1. If I override the == function of my router state instead, I fix the bug too, even with other WithViewStore in the upper hierarchy:
public struct CoordinatorState: IndexedRouterState, Equatable {
    public static func == (lhs: CoordinatorState, rhs: CoordinatorState) -> Bool {
        lhs.routes.count == rhs.routes.count
    }
    // [...]

That said, I'm not very confident with this simple equatability test 😅

  1. So, I've tested another one. The only thing that can change is the internal state of each screen. So I've tried to override the Equatability of my ScreenState instead, to test only the enum case part like so:
public enum ScreenState: Equatable {
    case app(AppState)
    case pushed1(Pushed1State)
    case pushed2(Pushed2State)
    case subCoordinator(SubCoordinatorState)
    
    public static func == (_ lhs: ScreenState, _ rhs: ScreenState) -> Bool {
        switch (lhs, rhs) {
        case (.app, .app),
             (.pushed1, .pushed1),
             (.pushed2, .pushed2),
             (.subCoordinator, .subCoordinator):
            return true
        case (.app, _),
            (.pushed1, _),
            (.pushed2, _),
            (.subCoordinator, _):
            return false
        }
    }
}

And it solves the bug too ! 🎉

What do you think? It requires quite a bit of boilerplate code, and I'm not super confident that it will not generate some missed updates in my sub views, later on, that could cause some weird bug in the future when I'd forgot about this "trick" 🤔

FredericRuaudel avatar Jun 14 '22 03:06 FredericRuaudel

To go further, I've tried two other things:

  1. Override the equality of my SubCoordinator screen state instead of my Coordinator's one, like so:
public enum SubScreenState: Equatable {
    case pushed1(Pushed1State)
    case pushed2(Pushed2State)
    
    public static func == (_ lhs: Self, _ rhs: Self) -> Bool {
        switch (lhs, rhs) {
        case (.pushed1, .pushed1),
            (.pushed2, .pushed2):
            return true
        case (.pushed1, _),
            (.pushed2, _):
            return false
        }
    }
}

But it doesn't work ❌

  1. I've made my Coordinator's ScreenState equality more specific by just ignoring the state associated with my SubCoordinator screen, like so:
public static func == (_ lhs: ScreenState, _ rhs: ScreenState) -> Bool {
        switch (lhs, rhs) {
        case (.subCoordinator, .subCoordinator):   // <------- I ignore the sub state of just this one
            return true
        case let (.app(lhsVal), .app(rhsVal)):  // <----- for other I keep the state equality
            return lhsVal == rhsVal
        case let (.pushed1(lhsVal), .pushed1(rhsVal)):
            return lhsVal == rhsVal
        case let (.pushed2(lhsVal), .pushed2(rhsVal)):
            return lhsVal == rhsVal
        case (.app, _),
            (.pushed1, _),
            (.pushed2, _),
            (.subCoordinator, _):
            return false
        }
    }

And it works ✅

Still not confident about the potential missed updates, but it narrows a bit more where the issue comes from. If we could find a way to avoid this more precisely in the library, it would be great... But I don't know if it's possible...What do you think?

FredericRuaudel avatar Jun 14 '22 04:06 FredericRuaudel

Here is another clue about this issue:

To get the bug we need this route pattern:

Router(
  routes: [
    [0]: Route.sheet(…),
    [1]: Route.push(
      SubRouter(
        routes: [
          [0]: Route.sheet(…),
          [1]: Route.push(…)

And what is interesting is that if I nest another similar pattern inside my current one like this:

[AppView -(push)-> [Pushed1View -(push)-> Pushed2View]]
                   [Pushed1View -(modal)-> [ModalView -(push)-> [Pushed3View -(push)-> Pushed4View]]]

where:

  • for the first pattern, we have AppView -> Pushed1View as the first Route.sheet / Route.push and Pushed1View -> Pushed2View as the second,
  • and for the nested second pattern, we have ModalView -> Pushed3View as the first Route.sheet / Route.push and Pushed3View -> Pushed4View as the second.

Then, if I do nothing special, I'll get the bug when I push either Pushed2View or Pushed4View But, if I override the equality of the first router's screen state (as shown in my previous comment), then it solves both pattern! Which means, I don't need to override the equality of my sub router that hold ModalView as its root. 🤷‍♂️

Hope I'm clear... If not, I can update my sample code to show this if you need to.

FredericRuaudel avatar Jun 14 '22 05:06 FredericRuaudel

@FredericRuaudel Thank you for posting your attempts and possible workarounds. 🙏 Have you found any other solutions to this? I am experiencing it too, but none of your suggestions seem to work for me.

We have a Router and a SubRouter, too, but regardless of what I return in the equality check, it always pops back exactly like in your videos.

ChaosCoder avatar Aug 19 '22 15:08 ChaosCoder

@ChaosCoder, sorry, I didn't. I used my last suggestion and didn't get any more trouble since then.

FredericRuaudel avatar Aug 23 '22 03:08 FredericRuaudel