packages icon indicating copy to clipboard operation
packages copied to clipboard

[go_router] Nested stateful navigation with ShellRoute

Open tolo opened this issue 1 year ago • 200 comments

Added functionality for building route configuration with support for preserving state in nested navigators. This change introduces a new shell route class called StatefulShellRoute, that uses separate navigators for its child routes as well as preserving state in each navigation branch. This is convenient when for instance implementing a UI with a BottomNavigationBar, with a persistent navigation state for each tab (i.e. building a Navigator for each tab). An example showcasing a UI with BottomNavigationBar and StatefulShellRoute has also been added (stateful_shell_route.dart).

Other examples of using StatefulShellRoute are also available in these repositories:


Below is a short example of how a `StatefulShellRoute` can be setup:
StatefulShellRoute(
  /// Each separate stateful navigation tree (i.e. Navigator) is represented by
  /// a StatefulShellBranch, which defines the routes that will be placed on that
  /// Navigator. StatefulShellBranch also makes it possible to configure
  /// things like an (optional) Navigator key, the default location (i.e. the
  /// location the branch will be navigated to when loading it for the first time) etc.
  branches: <StatefulShellBranch>[
    StatefulShellBranch(navigatorKey: optionalNavigatorKey, routes: <RouteBase>[
      GoRoute(
        path: '/a',
        builder: (BuildContext context, GoRouterState state) =>
        const RootScreen(label: 'A'),
        routes: <RouteBase>[
          GoRoute(
            path: 'details',
            builder: (BuildContext context, GoRouterState state) =>
            const DetailsScreen(label: 'A'),
          ),
        ],
      ),
    ]),
    /// The default location of a branch will by default be the first of the
    /// configured routes. To configure a different route, provide the
    /// defaultLocation parameter.
    StatefulShellBranch(defaultLocation: '/b/detail', routes: <RouteBase>[
      GoRoute(
        path: '/b',
        builder: (BuildContext context, GoRouterState state) =>
        const RootScreen(label: 'B'),
        routes: <RouteBase>[
          GoRoute(
            path: 'details',
            builder: (BuildContext context, GoRouterState state) =>
            const DetailsScreen(label: 'B'),
          ),
        ],
      ),
    ]),
  ],
  /// Like ShellRoute, the builder builds the navigation shell around the
  /// sub-routes, but with StatefulShellRoute, this navigation shell is able to
  /// maintain the state of the Navigators for each branch. The navigation shell
  /// could for instance use a BottomNavigationBar or similar.
  builder: (BuildContext context, StatefulShellRouteState state, Widget child) =>
      ScaffoldWithNavBar(shellState: state, body: child),
)

This fixes issue flutter/flutter#99124. It also (at least partially) addresses flutter/flutter#112267.

Pre-launch Checklist

  • [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
  • [x] I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • [x] I signed the CLA.
  • [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • [x] I listed at least one issue that this PR fixes in the description above.
  • [x] I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • [x] I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • [x] I updated/added relevant documentation (doc comments with ///).
  • [x] I added new tests to check the change I am making, or this PR is test-exempt.
  • [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

tolo avatar Sep 27 '22 12:09 tolo

Sorry, I found some additional documentation that needed updating, and also decided to improve the example and add an additional unit test.

I was going to answer the question about the assert of the builder field on ShellRoute, but I didn't expect that comment to disappear after my new push (maybe I did something wrong). Anyway, the assert that builder must be null when nestedNavigationBuilder is used is just because they are mutually exclusive (i.e. builder will not be used if nestedNavigationBuilder is set). It would just lead to confusion I you were to be allowed to use them simultaneously, since you would in effect be writing dead code in the passed builder function.

tolo avatar Sep 28 '22 10:09 tolo

cc @johnpryan

chunhtai avatar Sep 28 '22 15:09 chunhtai

I've refactored the implementation a bit now, and decided to implement the new behaviour outside ShellRoute, in a separate route class. Here are some of changes:

  • Created a common base class for routes providing a UI shell: ShellRouteBase.
  • Added PartitionedShellRoute - a shell route class that uses separate ("partitioned") navigators for its sub routes.
  • Added factory constructor stackedNavigation on PartitionedShellRoute for building a partitioned navigation shell based on an index stack, using new widget class StackedNavigationScaffold (see below).
  • Added StackedNavigationScaffold - A scaffold (or shell) that provides support for managing multiple navigators via an IndexStack, in a stateful way that responds to state changes. Using this removes the need for a lot of boilerplate when creating a UI with a BottomNavigationBar for instance.
  • Updated the stateful nested navigation example to use PartitionedShellRoute and StackedNavigationScaffold.

Have a look and see what you think. Need to add some more documentation (and tests), but I wanted to hear what you thought about this implementation first.

tolo avatar Sep 30 '22 03:09 tolo

I've now added more complete documentation with examples to PartitionedShellRoute, and I hope now this PR has reached a state where it could be merged. But I leave that up to you dear reviewers to decide of course. ;)

There are some things I've stumbled over while implementing this, that may be out of scope for this PR but might be something that should be addressed later (some could possibly be adressed in this PR):

  1. The documentation for the builder and pageBuilder fields of ShellRoute (now moved to ShellRouteBase) says that the child argument of the builder will be the Widget built by calling the matching sub-route's builder. This is incorrect - that argument will always contain the nested navigator for the sub routes. Feels like maybe this change could be adressed as part of this PR? The type should ideally be Navigator, but that would of course be a breaking change, so that would have to be addressed later.
  2. When calling pop on GoRouter or GoRouterDelegate, the _matchList of GoRouterDelegate isn't updated fully, i.e. the subloc and fullUriString fields of the RouteMatch for a parent (shell) route will still point to the previous path. Or maybe I'm missing some part of the reasoning here? The issue that I had was that the GoRouterState that was passed to the builder in a shell route didn't correctly identify the current path after pop. So another solution I guess would be to update RouteBuilder to derive the actual current path from the RouteMatchList for instance. Although updating the RouteMatchList feels more "right", if it is to be the source of truth for the current navigation state.

tolo avatar Oct 03 '22 12:10 tolo

  1. The documentation for the builder and pageBuilder fields of ShellRoute (now moved to ShellRouteBase) says that the child argument of the builder will be the Widget built by calling the matching sub-route's builder. This is incorrect - that argument will always contain the nested navigator for the sub routes. Feels like maybe this change could be adressed as part of this PR? The type should ideally be Navigator, but that would of course be a breaking change, so that would have to be addressed later.

Sorry for the spaminess, but I went ahead and fixed item no 1, seemed strange to leave a misleading description in there, especially now that these fields will be used for another class.

Hopefully there won't be any more commits now (unless there are more review issues of course).

One thing I wouldn't mind some more reviewing on is the naming bit. For instance, is PartitionedShellRoute a good name, or is there a better one?

tolo avatar Oct 03 '22 15:10 tolo

One thing I wouldn't mind some more reviewing on is the naming bit. For instance, is PartitionedShellRoute a good name, or is there a better one?

The new names seem reasonable to me. I don't have anything else to recommend. 🙂

bizz84 avatar Oct 04 '22 15:10 bizz84

I left some comments on the top level API, I have not look into details yet.

Great feedback, thanks! And I think you may be right in that the added flexibility of the two constructors in PartitionedShellRoute is perhaps overkill and only leads to confusion. And also the whole stack business can be kept private as you say.

I'm currently refactoring and rethinking some parts. The code below is roughly what it looks like now. I renamed PartitionedShellRoute to StatefulShellRoute. I've also introduced ShellRootRoute, which is a GoRoute subclass that requires a navigatorKey parameter, and doesn't accept a redirect in the constructor. An alternative could be to simply have a separate constructor on GoRoute that enforces the same things, and then simply use parentNavigatorKey.

I've struggled a bit with how to best handle a page builder, since the view hierarchy gets a bit more complicated here compared to ShellRoute. The solution I'm trying on right now is to always require a builder for building the shell, and if you need to customise the page, you specify an additional "pageForShell" function, which is responsible for simply providing a Page for the widget created by builder. I guess it could be called pageBuilder also, but I was looking for a way to differentiate it from that (but documentation/examples can of course cover that also).

StatefulShellRoute(
  builder: (BuildContext context, GoRouterState state, Widget child) =>
      ScaffoldWithNavBar(body: child),
  pageForShell: (BuildContext context, GoRouterState state, Widget shell) => 
      NoTransitionPage(child: shell),
  routes: <ShellRootRoute>[
    ShellRootRoute(
      navigatorKey: _tabANavigatorKey,
      path: '/a',
      builder: (BuildContext context, GoRouterState state) =>
          const RootScreen(label: 'A', detailsPath: '/a/details'),
      routes: <RouteBase>[
        GoRoute(
          path: 'details',
          builder: (BuildContext context, GoRouterState state) =>
              const DetailsScreen(label: 'A'),
        ), 
      ],
    )
  ],
),

I've also refactored our some bits of the StackedNavigationShell classes into separate state classes, associated with StatefulShellRoute instead. The main state is available through an InheritedWidget, via StatefulShellRoute.of(context):

StatefulShellRouteState shellState = StatefulShellRoute.of(context);

...

void _onItemTapped(BuildContext context, ShellRootRouteState routeState) {
  GoRouter.of(context).go(routeState.currentLocation);
}

All-in-all, this solution feels a bit simpler and cleaner, and I was also able to simplify the example a bit.

Will commit some code soon.

tolo avatar Oct 05 '22 22:10 tolo

I've also introduced ShellRootRoute, which is a GoRoute subclass that requires a navigatorKey parameter

It doesn't need to be a RouteBase subclass, it can just be a wrapper of route class to let StatefulShellRoute understand the navigator key to subroute relation. The StatefulShellRoute can unpack it back to routebase in constructor before pass into RouteBase.routes.

chunhtai avatar Oct 05 '22 22:10 chunhtai

It doesn't need to be a RouteBase subclass, it can just be a wrapper of route class to let StatefulShellRoute understand the navigator key to subroute relation.

Yes, I guess you could do it that way too, however using a wrapper involves an additional level of nesting in the routes declaration, and also introduces the usage of a non-Route class in the "route declaration tree". Simply using ShellRootRoute might be a tad simpler and more readable.

The only "downside" I see of requiring direct children of StatefulShellRoute to be of type ShellRootRoute (maybe there is a better name for it), is that this means you cannot use a custom RouteBase subclass as the root of a nested navigation tree. On the other hand, it doesn't feel that strange to have that type of constraint on the API contract for a specialised class like this.

But for added flexibility, we could possibly support both ways of configuring a StatefulShellRoute. Looking at that right now.

Will give this some more thought, and will commit another proposed solution soon.

tolo avatar Oct 06 '22 14:10 tolo

Ok, so I pushed another iteration. PartitionedShellRoute is now refactored and renamed to StatefulShellRoute, pretty much like as described above. There are two constructors, one that accepts a list of items containing a navigator key and root route (ShellNavigationBranchItem), and another that uses the list of child routes (GoRoutes) to achieve the same result, but in a somewhat more convenient manner in the call site. The details about the use of an IndexStack are now hidden away.

Please have a look and see what you think.

tolo avatar Oct 07 '22 14:10 tolo

I would like to add a suggestion as well. sometimes the user does not want to use a Navigator at all e.g. in web templates where the user wants : ListView([ Header / custom appbar , dynamic child based on navigation , Footer ] )

in this case you can't use Navigator since it requires a parent with defined height/width

my suggestion is this: RawShellRoute > the child in the builder is the Raw list of Widgets that need to be displayed (where the last item in the list is the one that is supposed to be displayed) ShellRoute > the child is the Navigator (same as now)

  • Other shell routes in this PR...

ahmednfwela avatar Oct 08 '22 23:10 ahmednfwela

in this case you can't use Navigator since it requires a parent with defined height/width

Interesting, not something I've run into or considered. Anyway, this feels a bit out of scope for this PR, so I recommend you post a separate issue for this particular shell route / templating use case.

tolo avatar Oct 10 '22 12:10 tolo

Trying to decide which constructor(s) leads to the best API for StatefulShellRoute. Right now I have three candidates, and I’d love some feedback here. Perhaps there could be more than one, although it might be preferable to stick with only one if possible. These are the current alternatives I’m considering:

  1. Default constructor, with list of ShellRouteBranch (previously ShellNavigationBranchItem). ShellRouteBranch is just a simple wrapper class around a navigator key and a root route. Although conceptually it is a rather good abstraction for a separate “branch” in the navigation tree, it is perhaps a bit more cumbersome declaration-wise, due to the additional nesting. It also introduces a non-route class as a concept in the route declaration tree (i.e. a route has sub routes, with subroutes etc). In a prior iteration of PartitionedShellRoute, the corresponding class (StackedNavigationItem) also required a rootRoutePath field (“defaultLocation” would have been a better name), but I was able to remove the need for this, and instead deriving this value from the route itself (via RouteConfiguration). However, to open up for the possibility of customising the default location for a specific navigation branch, I’ve added defaultLocation as an optional field in ShellRouteBranch.

  2. StatefulShellRoute.rootRoutes - Constructor that takes list of GoRoutes, representing the root routes. This constructor uses the parentNavigatorKey field of GoRoute as the navigator key for the separate branch in the navigation tree. The only question here is if dedicated class should be used instead of GoRoute (in an example above I used a class called ShellRootRoute), but here I felt it would be beneficial to not introduce too many new classes. The advantage of this constructor is that is doesn’t require any wrapping/nesting and that it “blends in” better in the route declaration tree. The only disadvantage is that the root routes need to be of type GoRoute, and with a parentNavigatorKey set.

  3. StatefulShellRoute.keysWithRootRoutes - Constructor with lists of navigator keys and routes. This constructor sets up the route branches using items at corresponding indices in lists parameters for navigatorKeys and routes, similar to how Map.fromIterables(Iterable<K> keys, Iterable<V> values) works. The advantage of this constructor is that you also don’t need any additional wrapping/nesting, and also that you are able to use any Route(Base) class. The disadvantage is a somewhat inferior structure compared to point 2 above, due to the need for index-matching navigatorKeys and the root routes. This constructor is not currently in the code, but would look like this:

/// Constructs a [StatefulShellRoute] by specifying a list of child routes
/// along with a list of navigator keys ([GlobalKey] to be used by a
/// [Navigator]).
///
/// Each route will be matched with a navigator key at the corresponding index
/// in navigatorKeys. Together, they will represent the root in a stateful
/// route branch, for which a separate Navigator will be created.
factory StatefulShellRoute.keysWithRootRoutes({
  required List<GlobalKey<NavigatorState>> navigatorKeys,
  required List<RouteBase> routes,
  required ShellRouteBuilder builder,
})

My thinking right now is to keep constructor 1 as the default constructor and also including constructor 2, as a more convenient alternative for the most common use cases.

The naming of things is of course still up for discussion. These are the key players that are part of the public API right now:

  • StatefulShellRoute - The route class for implementing stateful/persistent shell route navigation
  • ShellRouteBranch - Representation of a separate branch in a stateful navigation tree
  • StatefulShellRouteState - The current state of a StatefulShellRoute, including the currently active branch and references to ShellRouteBranchState.
  • ShellRouteBranchState - The current state for a particular route branch (ShellRouteBranch)

tolo avatar Oct 10 '22 12:10 tolo

@tolo

For me your first makes most sense. I've been thinking about this as well for a bit.. Would it not make more sense to pass down the children in the StatefulShellRoute.builder? Then the children could be put into a PageView or even a TabView and then the selected index could be derived from the GoRouterState. This way you provide a way to build nice animations for the BottomNavigationBar as well as the TabBar etc.

StatefulShellRoute(
  builder: (BuildContext context, GoRouterState state, List<Widget> children) {
    return ScaffoldWithNavBar(children: children);
  },
  pageBuilder: (BuildContext context, GoRouterState state, Widget shell) {
    return NoTransitionPage(child: shell);
  },
  routes: <ShellRootRoute>[
    ShellRootRoute(
      navigatorKey: _tabANavigatorKey,
      path: '/a',
      builder: (BuildContext context, GoRouterState state) {
        return const RootScreen(label: 'A', detailsPath: '/a/details');
      },
      routes: <RouteBase>[
        GoRoute(
          path: 'details',
          builder: (BuildContext context, GoRouterState state) {
            return const DetailsScreen(label: 'A');
          },
        ), 
      ],
    )
  ],
),

jopmiddelkamp avatar Oct 10 '22 14:10 jopmiddelkamp

Would it not make more sense to pass down the children in the StatefulShellRoute.builder? Then the children could be put into a PageView or even a TabView and then the selected index could be derived from the GoRouterState. This way you provide a way to build nice animations for the BottomNavigationBar as well as the TabBar etc.

Good point! Much of this can already be achieved with the current implementation though (i.e. there is support for animations in BottomNavigationBar, TabBar etc), however if you require a different container than an IndexedStack for the different navigators (such as a PageView), the current implementation is not ideal. I did actually open up for more customisation in a previous iteration, but removed it after feedback. However I do now believe that some kind of customisation will be needed to make this implementation complete. I see two options:

  1. Add a "branchContainerBuilder" to StatefulShellRoute, as an alternative to builder, to be able to customize the container for the navigation branches (i.e. instead of using an IndexedStack).
  2. Create a more basic version of StatefulShellRoute, that opens up for customization. StatefulShellRoute could then inherit that class instead of ShellRouteBase.

Trying out option 1 right now. Also thinking of perhaps dropping the animation support (i.e. transitionBuilder and transitionDuration fields of StatefulShellRoute), and letting that be up to the user to handle instead.

tolo avatar Oct 10 '22 20:10 tolo

The problem with not using a navigator is that the subroute match produce a page list, which i think we should change it. we could force the subroute of the shellroutebase to only have builder and disallow pageBuilder. This will require a lot of new design though.

Alternatively, we can also expose some api in Route class in the flutter/flutter to be able to get the widget body directly without all the modalroute barrier and modalroutesceop stuff.

I also planned on doing some serious route refactoring to get rid of all the inconvenience with page class. For example, there is no way to declaratively remove a page that refuse to pop. There is also some limitation that makes it hard to update route object from page update

chunhtai avatar Oct 10 '22 21:10 chunhtai

  1. Add a "branchContainerBuilder" to StatefulShellRoute, as an alternative to builder, to be able to customize the container for the navigation branches (i.e. instead of using an IndexedStack).
  2. Create a more basic version of StatefulShellRoute, that opens up for customization. StatefulShellRoute could then inherit that class instead of ShellRouteBase.

Trying out option 1 right now. Also thinking of perhaps dropping the animation support (i.e. transitionBuilder and transitionDuration fields of StatefulShellRoute), and letting that be up to the user to handle instead.

Actually, come to think of it, there is no need for an additional builder, as you can access both the child navigators and the current index from StatefulShellRouteState, which you can access from the BuildContext in the builder. Refining things a bit now, to clarify and to avoid unnecessary code being run (when choosing not to use the default IndexedStack).

Will still skip the animation support though - it works for simple fade transitions, but not for anything more advanced really. So better to leave those details up to the user.

tolo avatar Oct 11 '22 13:10 tolo

Refining things a bit now, to clarify and to avoid unnecessary code being run (when choosing not to use the default IndexedStack).

Updated documentation and added some refinements around this in the latest commit, i.e. better support for customising the container for the branch navigators, while at the same time making it easy to use the default (IndexedStack) implementation. Updated sample code to showcase this.

Will still skip the animation support though - it works for simple fade transitions, but not for anything more advanced really. So better to leave those details up to the user.

Animation support is out, which I feel makes sense in order to keep the code more focused on solving the core issue. I've instead added an example on how to add animations in the sample code.

tolo avatar Oct 12 '22 00:10 tolo

Perhaps not strictly related to the main topic (and perhaps more of a clarification), but it was brought to my attention that the sample code (stateful_nested_navigation.dart) uses two nested Scaffolds, and that the documentation for Scaffold notes that this is typically not necessary. The documentation mentions a scenario with TabBar along with a proposed workaround, however that workaround isn’t really helpful for a scenario with nested navigators. Thus, I interpret the wording “typically not necessary” to mean that nested Scaffolds can be ok under some circumstances, and that this is such a case.

Anyway, I hope this proposed solution for nested stateful navigation is starting to near completion now (i.e. time for another round of reviewing).

tolo avatar Oct 13 '22 11:10 tolo

Seems to be some UX glitch in GitHub with re-requesting reviews (or perhaps it's just me 😂), but anyway @chunhtai and @bizz84, would appreciate a re-review.

tolo avatar Oct 13 '22 13:10 tolo

And what will the ShellRoute inside ShellRoute in single screen look like? It was just possible to add routers and ShellRoute to the list earlier, because they inherited from RouteBase. And now StatefulShellRoute has only the GoRoute type.

Carapacik avatar Oct 16 '22 11:10 Carapacik

It was just possible to add routers and ShellRoute to the list earlier, because they inherited from RouteBase. And now StatefulShellRoute has only the GoRoute type.

Actually, the default constructor of StatefulShellRoute does accept routes of any RouteBase based type:

StatefulShellRoute(branches: [
  ShellRouteBranch(
    navigatorKey: _tabANavigatorKey,
    rootRoute: ShellRoute(...),
  ),
  ShellRouteBranch(
    navigatorKey: _tabBNavigatorKey,
    rootRoute: GoRoute(...),
  ),
]),

The constructor that is limited to GoRoutes (StatefulShellRoute.rootRoutes) is only a shorthand/convenience constructor, that would likely suffices in many cases. However, your comment made me realise that the sample code should probably use the default constructors instead of the shorthand one. And of course, there are certainly cases where you might what a ShellRoute below a StatefulShellRoute, for example if you have a TabBar on one (or more) of the pages within a shell using a BottomNavigationBar. Perhaps it could be a good idea to showcase that in the sample code.

tolo avatar Oct 17 '22 15:10 tolo

I think just such an example is needed. So that one of the tabs of the lower navigation has two tabs, for example, side navigation.

Carapacik avatar Oct 17 '22 16:10 Carapacik

Would it also be possible to add the ShellRouteState to the StatefulShellRoute's builder? This way I think we can provide a good solution for supporting better animation options.

      StatefulShellRoute.rootRoutes(
        builder: (BuildContext context, GoRouterState state, ShellRouteState shellState, Widget navigationContainer) {
          return ScaffoldWithNavBar(
            index: shellState.index,
            children: shellState.navigators,
          );
        },

For more context what I'm trying to accomplish see: https://github.com/tolo/flutter_packages/pull/1/files

jopmiddelkamp avatar Oct 20 '22 10:10 jopmiddelkamp

Would it also be possible to add the ShellRouteState to the StatefulShellRoute's builder? This way I think we can provide a good solution for supporting better animation options.

It's certainly possible, but I'm not sure it's needed, since you can get the StatefulShellRouteState from BuildContext, even within the builder function.

StatefulShellRoute.rootRoutes(
  builder: (BuildContext context, GoRouterState state, Widget navigationContainer) {
    final StatefulShellRouteState shellState = StatefulShellRoute.of(context);
    return ScaffoldWithNavBar(
      index: shellState.index,
      children: shellState.navigators,
    );
  },

The sample code (stateful_nested_navigation.dart) mentions this in a block of commented out code, which uses an animated container (_AnimatedRouteBranchContainer) for the navigators.

Or did I perhaps misunderstood what you meant?

tolo avatar Oct 20 '22 11:10 tolo

The sample code (stateful_nested_navigation.dart) mentions this in a block of commented out code, which uses an animated container (_AnimatedRouteBranchContainer) for the navigators.

Or did I perhaps misunderstood what you meant?

Ahh my bad. Missed the line below. final StatefulShellRouteState shellRouteState = StatefulShellRoute.of(context);

jopmiddelkamp avatar Oct 20 '22 11:10 jopmiddelkamp

Now that I've updated my example with the default StatefulShellRoute constructor.

StatefulShellRoute(branches: [
  ShellRouteBranch(
    navigatorKey: _tabANavigatorKey,
    rootRoute: ShellRoute(...),
  ),
  ShellRouteBranch(
    navigatorKey: _tabBNavigatorKey,
    rootRoute: GoRoute(...),
  ),
]),

Only it results in the following error:

Unhandled Exception: 'package:go_router/src/route.dart': Failed assertion: line 630 pos 16: 'route.parentNavigatorKey == null || route.parentNavigatorKey == branches[i].navigatorKey': is not true.

It seems like the navigatorKey of the ShellRouteBranch is not being used properly.

The exception can be reproduced in this code example: https://github.com/jopmiddelkamp/flutter_packages_tolo/blob/nested-persistent-navigation/packages/go_router/example/lib/stateful_nested_navigation_nav_tab.dart

jopmiddelkamp avatar Oct 20 '22 12:10 jopmiddelkamp

Only it results in the following error:

Unhandled Exception: 'package:go_router/src/route.dart': Failed assertion: line 630 pos 16: 'route.parentNavigatorKey == null || route.parentNavigatorKey == branches[i].navigatorKey': is not true.

It seems like the navigatorKey of the ShellRouteBranch is not being used properly.

The exception can be reproduced in this code example: https://github.com/jopmiddelkamp/flutter_packages_tolo/blob/nested-persistent-navigation/packages/go_router/example/lib/stateful_nested_navigation_nav_tab.dart

I wasn't able to reproduce that error, but the app instead crashed on another assert: duplicate named routes (i.e. same name in multiple places).

But I'll do some more checking (and possibly unit testing) around the Navigator keys. I'm also considering using a default value for Navigator keys, much like ShellRoute does.

tolo avatar Oct 21 '22 07:10 tolo

Only it results in the following error:

Unhandled Exception: 'package:go_router/src/route.dart': Failed assertion: line 630 pos 16: 'route.parentNavigatorKey == null || route.parentNavigatorKey == branches[i].navigatorKey': is not true.

It seems like the navigatorKey of the ShellRouteBranch is not being used properly. The exception can be reproduced in this code example: https://github.com/jopmiddelkamp/flutter_packages_tolo/blob/nested-persistent-navigation/packages/go_router/example/lib/stateful_nested_navigation_nav_tab.dart

I wasn't able to reproduce that error, but the app instead crashed on another assert: duplicate named routes (i.e. same name in multiple places).

But I'll do some more checking (and possibly unit testing) around the Navigator keys. I'm also considering using a default value for Navigator keys, much like ShellRoute does.

My apologies not sure what happend before but indeed when I ran the example again today it provided me with the error of duplicate names. Also updated my example a bit more. Almost got it working!!

I'll play around a bit more to see if I can get it to work :) Would it be an idea to maybe add a boolean to the shell to make the shell load all of its child navigators? Then I think with some tweaking of the example we could get it to work quite nicely.

jopmiddelkamp avatar Oct 21 '22 16:10 jopmiddelkamp

My apologies not sure what happend before but indeed when I ran the example again today it provided me with the error of duplicate names. Also updated my example a bit more. Almost got it working!!

No worries 😉 I'm actually also laying the finishing touch on an updated example, using a nested StatefulShellRoute with a TabBar.

I'll play around a bit more to see if I can get it to work :) Would it be an idea to maybe add a boolean to the shell to make the shell load all of its child navigators? Then I think with some tweaking of the example we could get it to work quite nicely.

I did actually consider adding support for eager loading of the child navigators, as I've built such functionality when using BottomNavigationBar in the past. But I kind of put that aside bit, since it perhaps seemed like too much work to implement in GoRouter, and also since for instance animations work fine without any eager loading. And often, this is largely an issue of preloading data and resources, which is work that can be done elsewhere ("lifted up"). However, looking at it again now, there might be a way to make this possible without too much intrusive work I think. So I'll give it another round of thought (or possibly this should be moved to a separate PR).

tolo avatar Oct 22 '22 18:10 tolo