Canonical way for popping outer `ImplicitNavigator` in nested scenarios

[btrautmann]

So I'm playing with ImplicitNavigator to get a feel for the sweet spot between imperative APIs and the behemoth that is GoRouter, and one thing I'm not finding intuitive is handling completion of "flows" when in a nested ImplicitNavigator scenario. Given the following "flow":

sealed class RootFlowStep {}

base class Start extends RootFlowStep {}

base class AddFix extends RootFlowStep {}

base class ViewFixDetails extends RootFlowStep {
  final String fixId;

  ViewFixDetails(this.fixId);
}

base class Settings extends RootFlowStep {}

class RootFlow extends StatelessWidget {
  final ValueNotifier<RootFlowStep> _flow = ValueNotifier(Start());
  RootFlow({super.key});

  @override
  Widget build(BuildContext context) {
    return MultiProvider(
      providers: [
        ChangeNotifierProvider.value(value: _flow),
      ],
      child: ImplicitNavigator.fromValueNotifier(
        valueNotifier: _flow,
        builder: (
          BuildContext context,
          RootFlowStep value,
          Animation<double> animation,
          Animation<double> secondaryAnimation,
        ) {
          switch (value) {
            case Start():
              return const FixesPage();
            case AddFix():
              return AddFixFlow(onComplete: () {
                // TODO: Best way to show `Start()` here?
              });
            case ViewFixDetails():
              return FixDetailsFlow(
                fixId: value.fixId,
                onComplete: () {
                  // TODO: Best way to show `Start()` here?
                },
              );
            case Settings():
              return const SettingsPage();
          }
        },
      ),
    );
  }
}

In the above, AddFix and ViewFixDetails() are both "sub-flows" that use their own ImplicitNavigators to show various "screens" that allow users to take action. Once they are "done", onComplete is invoked. It makes sense to me to have the parent flow (in this case RootFlow) own the completion behavior, but doing so is proving to be finicky.

Attempt 1:

Originally I simply did _flow.value = Start() but this results in a new page being added to the stack, such that the resulting stack is:

RootFlow (Start) => AddFixFlow (_) => Start

Attempt 2:

Second, I tried ImplicitNavigator.of(context).pop() but this results in a pop being called on the AddFixFlow's ImplicitNavigator and the user is simply sent back a single page.

Attempt 3:

For funsies, I gave ImplicitNavigator.of(context, root: true).pop() a whirl, but this had no real effect (visually at least)

---

So, what's the "correct" way to do this, and/or how should I be thinking about this differently?

[btrautmann]

Actually, putting aside the question of popping from the outer ImplicitNavigator, I'm noticing that just the code above produces a bug where there are 2 FixesPages 🤔 :

From what I can tell, this is because it's added in both initState and didUpdateWidget.

[caseycrogers]

So the critical piece missing here is the getDepth parameter. Implicit navigator uses depth to determine if a new page should be added to the stack or if it should replace the stack. You can think of a change in value as roughly equivalent to (pseudocode):

final newRoute = ImplicitNavigatorRoute(value: value, depth: getDepth(value));
if (newRoute.depth == null) {
  Navigator.of(context).push(newRoute);
} else {
  Navigator.of(context).pushAndRemoveUntil(
    newRoute,
    (route) => (route as ImplicitNavigatorRoute).depth < newRoute.depth,
  );
}

So here's my understanding of your desired outcome:

1. User opens the app and is greeted with RootFlow (start)
2. User taps AddFixFlow
3. User taps Finish which SHOULD take them back to RootFlow (start)

Instead (under attempt 1) you get the root appended to the end instead of popping back to the original root:
RootFlow (Start) => AddFixFlow (_) => RootFlow (Start)

This is because you haven't specified getDepth so whenever the state changes, implicit navigator assumes the new depth value is null, and null is a special case that basically means "Always append".

If you add the following to your root level navigator and follow the steps from attempt 1, everything should just work!

  getDepth: (RootFlowStep step) {
    // You could alternatively give each `step` class an `index` attribute and just return it.
    // This is even easier with enums because they inherently have an ordering.
    // I don't understand sealed classes well yet, so maybe there's a cleaner way
    // to do this.
    return switch (step) {
      Start _ => 0,
      AddFix _ => 1,
      ViewFixDetails _ => 2,
    };
  },

Now, when you update the value to start, Implicit nav sees that start is depth zero, so it pops all the extra pages off the top.

My action items from this (let me know if you think these will help):

1. The README is wayy too information dense, I've been meaning for awhile to rewrite it to frontload the important information. Namely, I think my discussion of app style and browser style navigation really needs to be removed or put at the bottom because it's really not relevant to 90% of users.
2. Maybe I should make getDepth and depth required arguments, but keep them nullable so it's much more clear that they're important? I could add kAlwaysAppend as a convenience for getDepth that always returns null to make it easier to navigate the API.
3. Good catch on the bug, I'll look into it and fix it. All my widget tests just check that the top page is correct-they don't check that the entire page stack is as expected. I'll go through and restructure them to check the entire page stack to catch bugs like this.
   Also note that getDepth will get rid of this bug for your specific case-but I'll fix it for the situations where devs actually do want append-only behavior.

[btrautmann]

So the critical piece missing here is the getDepth parameter

Doh! I knew it had something to do with depth but totally overlooked the parameter. To your point on the README, I think you're right about refactoring it in such a way that the most common stuff is super obvious and more complex use cases are nested/abstracted out for those who know what they're looking for (FWIW I could've done a better job here with regards to API exploration!)

Maybe I should make getDepth and depth required arguments, but keep them nullable so it's much more clear that they're important?

I have to noodle on this a bit more, I think, but right now I'm leaning towards liking the idea of getDepth being be required. It feels pretty common/basic to require some idea of depth with the implicitness of the package itself. OTOH, it's one of those things that once you run into it once, you probably(?) don't forget it again.

PS what I ended up with (for now) is the following:

abstract class FlowStep {
  final int depth;

  FlowStep({required this.depth});
}

I then had RootFlowStep extend that, and all subclasses of RootFlowStep declare their depth:

base class Start extends RootFlowStep {
  Start() : super(depth: 0);
}

base class AddFix extends RootFlowStep {
  AddFix() : super(depth: 1);
}

base class ViewFixDetails extends RootFlowStep {
  final String fixId;

  ViewFixDetails(this.fixId) : super(depth: 1);
}

base class Settings extends RootFlowStep {
  Settings() : super(depth: 1);
}

It's interesting to note here that basically all non-root screens or sub-flows have a depth of 1. It makes sense, but I wonder if it's a foot gun in the future if the flow gets refactored and I forget to change depths of the various steps? For instance, if I wanted to shim a step in between Start and Settings (like an FTUX screen or something, idk), then Settings should become 2 and the FTUX screen should become 1. Actually that case would be even more complicated, because presumably that FTUX displaying would be conditional, so I'd need to have my depth be conditional as well. Obviously still more fiddling here to do on my part!

It's probably obvious but for completeness I'll just add that my navigator definition became:

ImplicitNavigator.fromValueNotifier(
  valueNotifier: _flow,
  getDepth: (step) => step.depth,
  builder: ...
)

@caseycrogers Let me know how you wanna handle this ticket. Happy to keep it open to address the README, getDepth parameter, and double-stack-entry bugs, but I'd also be happy to file those separately and close this one out. Just lemme know!

[caseycrogers]

Leave this one open and I'll close it out when I get the action items done!

As for the depth situation, yeah this is definitely a bit of a foot gun. In the ultra simple case, enums work very nicely to enforce this out of the box. There may be a more clever way to do this for anything that isn't a super basic scenario.

I lean towards putting a switch in getDepth or defining a static ExtensionOnMyBaseClass.getDepth function. This is nice because unlike defining a depth field on each sub classes independently, it's all in one place so when you add a new class the spot you go to add a depth for the new class is the same place where you'll go to update all the other depths, if applicable. This still relies on you not making a mistake, but it at least pushes you in the right direction.

There are some ways you could write a more opinionated system (eg intuiting the depth from the object inheritance hierarchy)-and I could imagine something like a code generator doing that here, but in the end that'd reduce flexibility a lot.