[go_router] Allows redirect only GoRoute to be part of RouteMatchList (flutter#4315)

fixes flutter#114807

Author: chunhtai

packages/go_router/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 9.0.1
+
+- Allows redirect only GoRoute to be part of RouteMatchList.
+
 ## 9.0.0
 
 - **BREAKING CHANGE**:

packages/go_router/lib/src/builder.dart

@@ -88,6 +88,8 @@ class RouteBuilder {
       // empty box until then.
       return const SizedBox.shrink();
     }
+    assert(
+        matchList.isError || !(matchList.last.route as GoRoute).redirectOnly);
     return builderWithNav(
       context,
       Builder(
@@ -212,8 +214,12 @@ class RouteBuilder {
       if (route is GoRoute) {
         page =
             _buildPageForGoRoute(context, state, match, route, pagePopContext);
-
-        keyToPages.putIfAbsent(routeNavKey, () => <Page<Object?>>[]).add(page);
+        assert(page != null || route.redirectOnly);
+        if (page != null) {
+          keyToPages
+              .putIfAbsent(routeNavKey, () => <Page<Object?>>[])
+              .add(page);
+        }
 
         _buildRecursive(context, matchList, startIndex + 1, pagePopContext,
             routerNeglect, keyToPages, navigatorKey, registry);
@@ -275,8 +281,6 @@ class RouteBuilder {
     if (page != null) {
       registry[page] = state;
       pagePopContext._setRouteMatchForPage(page, match);
-    } else {
-      throw GoError('Unsupported route type $route');
     }
   }
 
@@ -344,36 +348,30 @@ class RouteBuilder {
   }
 
   /// Builds a [Page] for [GoRoute]
-  Page<Object?> _buildPageForGoRoute(BuildContext context, GoRouterState state,
+  Page<Object?>? _buildPageForGoRoute(BuildContext context, GoRouterState state,
       RouteMatch match, GoRoute route, _PagePopContext pagePopContext) {
-    Page<Object?>? page;
-
     // Call the pageBuilder if it's non-null
     final GoRouterPageBuilder? pageBuilder = route.pageBuilder;
     if (pageBuilder != null) {
-      page = pageBuilder(context, state);
-      if (page is NoOpPage) {
-        page = null;
+      final Page<Object?> page = pageBuilder(context, state);
+      if (page is! NoOpPage) {
+        return page;
       }
     }
-
-    // Return the result of the route's builder() or pageBuilder()
-    return page ??
-        buildPage(context, state, Builder(builder: (BuildContext context) {
-          return _callGoRouteBuilder(context, state, route);
-        }));
+    return _callGoRouteBuilder(context, state, route);
   }
 
   /// Calls the user-provided route builder from the [GoRoute].
-  Widget _callGoRouteBuilder(
+  Page<Object?>? _callGoRouteBuilder(
       BuildContext context, GoRouterState state, GoRoute route) {
     final GoRouterWidgetBuilder? builder = route.builder;
 
     if (builder == null) {
-      throw GoError('No routeBuilder provided to GoRoute: $route');
+      return null;
     }
-
-    return builder(context, state);
+    return buildPage(context, state, Builder(builder: (BuildContext context) {
+      return builder(context, state);
+    }));
   }
 
   /// Builds a [Page] for [ShellRouteBase]

packages/go_router/lib/src/match.dart

@@ -263,8 +263,11 @@ class RouteMatchList {
     assert(index != -1);
     newMatches.removeRange(index, newMatches.length);
 
-    // Also pop ShellRoutes when there are no subsequent route matches
-    while (newMatches.isNotEmpty && newMatches.last.route is ShellRouteBase) {
+    // Also pop ShellRoutes that have no subsequent route matches and GoRoutes
+    // that only have redirect.
+    while (newMatches.isNotEmpty &&
+        (newMatches.last.route is ShellRouteBase ||
+            (newMatches.last.route as GoRoute).redirectOnly)) {
       newMatches.removeLast();
     }
     // Removing ImperativeRouteMatch should not change uri and pathParameters.

packages/go_router/lib/src/parser.dart

@@ -98,6 +98,14 @@ class GoRouteInformationParser extends RouteInformationParser<RouteMatchList> {
       if (matchList.isError && onParserException != null) {
         return onParserException!(context, matchList);
       }
+
+      assert(() {
+        if (matchList.isNotEmpty) {
+          assert(!(matchList.last.route as GoRoute).redirectOnly,
+              'A redirect-only route must redirect to location different from itself.\n The offending route: ${matchList.last.route}');
+        }
+        return true;
+      }());
       return _updateRouteMatchList(
         matchList,
         baseRouteMatchList: state.baseRouteMatchList,

packages/go_router/lib/src/route.dart

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 import 'package:collection/collection.dart';
+import 'package:flutter/foundation.dart';
 import 'package:flutter/widgets.dart';
 import 'package:meta/meta.dart';
 
@@ -96,7 +97,7 @@ import 'typedefs.dart';
 /// ///
 /// See [main.dart](https://github.com/flutter/packages/blob/main/packages/go_router/example/lib/main.dart)
 @immutable
-abstract class RouteBase {
+abstract class RouteBase with Diagnosticable {
   const RouteBase._({
     required this.routes,
     required this.parentNavigatorKey,
@@ -118,6 +119,15 @@ abstract class RouteBase {
     return routes.expand(
         (RouteBase e) => <RouteBase>[e, ...routesRecursively(e.routes)]);
   }
+
+  @override
+  void debugFillProperties(DiagnosticPropertiesBuilder properties) {
+    super.debugFillProperties(properties);
+    if (parentNavigatorKey != null) {
+      properties.add(DiagnosticsProperty<GlobalKey<NavigatorState>>(
+          'parentNavKey', parentNavigatorKey));
+    }
+  }
 }
 
 /// A route that is displayed visually above the matching parent route using the
@@ -157,6 +167,11 @@ class GoRoute extends RouteBase {
     _pathRE = patternToRegExp(path, pathParameters);
   }
 
+  /// Whether this [GoRoute] only redirects to another route.
+  ///
+  /// If this is true, this route must redirect location other than itself.
+  bool get redirectOnly => pageBuilder == null && builder == null;
+
   /// Optional name of the route.
   ///
   /// If used, a unique string name must be provided and it can not be empty.
@@ -323,8 +338,12 @@ class GoRoute extends RouteBase {
   final List<String> pathParameters = <String>[];
 
   @override
-  String toString() {
-    return 'GoRoute(name: $name, path: $path)';
+  void debugFillProperties(DiagnosticPropertiesBuilder properties) {
+    super.debugFillProperties(properties);
+    properties.add(StringProperty('name', name));
+    properties.add(StringProperty('path', path));
+    properties.add(
+        FlagProperty('redirect', value: redirectOnly, ifTrue: 'Redirect Only'));
   }
 
   late final RegExp _pathRE;
@@ -338,6 +357,21 @@ abstract class ShellRouteBase extends RouteBase {
       {required super.routes, required super.parentNavigatorKey})
       : super._();
 
+  static void _debugCheckSubRouteParentNavigatorKeys(
+      List<RouteBase> subRoutes, GlobalKey<NavigatorState> navigatorKey) {
+    for (final RouteBase route in subRoutes) {
+      assert(
+          route.parentNavigatorKey == null ||
+              route.parentNavigatorKey == navigatorKey,
+          "sub-route's parent navigator key must either be null or has the same navigator key as parent's key");
+      if (route is GoRoute && route.redirectOnly) {
+        // This route does not produce a page, need to check its sub-routes
+        // instead.
+        _debugCheckSubRouteParentNavigatorKeys(route.routes, navigatorKey);
+      }
+    }
+  }
+
   /// Attempts to build the Widget representing this shell route.
   ///
   /// Returns null if this shell route does not build a Widget, but instead uses
@@ -506,12 +540,11 @@ class ShellRoute extends ShellRouteBase {
   })  : assert(routes.isNotEmpty),
         navigatorKey = navigatorKey ?? GlobalKey<NavigatorState>(),
         super._() {
-    for (final RouteBase route in routes) {
-      if (route is GoRoute) {
-        assert(route.parentNavigatorKey == null ||
-            route.parentNavigatorKey == navigatorKey);
-      }
-    }
+    assert(() {
+      ShellRouteBase._debugCheckSubRouteParentNavigatorKeys(
+          routes, this.navigatorKey);
+      return true;
+    }());
   }
 
   /// The widget builder for a shell route.
@@ -576,6 +609,13 @@ class ShellRoute extends ShellRouteBase {
   @override
   Iterable<GlobalKey<NavigatorState>> get _navigatorKeys =>
       <GlobalKey<NavigatorState>>[navigatorKey];
+
+  @override
+  void debugFillProperties(DiagnosticPropertiesBuilder properties) {
+    super.debugFillProperties(properties);
+    properties.add(DiagnosticsProperty<GlobalKey<NavigatorState>>(
+        'navigatorKey', navigatorKey));
+  }
 }
 
 /// A route that displays a UI shell with separate [Navigator]s for its
@@ -828,6 +868,13 @@ class StatefulShellRoute extends ShellRouteBase {
     }
     return true;
   }
+
+  @override
+  void debugFillProperties(DiagnosticPropertiesBuilder properties) {
+    super.debugFillProperties(properties);
+    properties.add(DiagnosticsProperty<Iterable<GlobalKey<NavigatorState>>>(
+        'navigatorKeys', _navigatorKeys));
+  }
 }
 
 /// Representation of a separate branch in a stateful navigation tree, used to
@@ -854,7 +901,13 @@ class StatefulShellBranch {
     this.initialLocation,
     this.restorationScopeId,
     this.observers,
-  }) : navigatorKey = navigatorKey ?? GlobalKey<NavigatorState>();
+  }) : navigatorKey = navigatorKey ?? GlobalKey<NavigatorState>() {
+    assert(() {
+      ShellRouteBase._debugCheckSubRouteParentNavigatorKeys(
+          routes, this.navigatorKey);
+      return true;
+    }());
+  }
 
   /// The [GlobalKey] to be used by the [Navigator] built for this branch.
   ///

packages/go_router/pubspec.yaml

@@ -1,7 +1,7 @@
 name: go_router
 description: A declarative router for Flutter based on Navigation 2 supporting
   deep linking, data-driven routes and more
-version: 9.0.0
+version: 9.0.1
 repository: https://github.com/flutter/packages/tree/main/packages/go_router
 issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+go_router%22
 

packages/go_router/test/go_route_test.dart

@@ -155,4 +155,127 @@ void main() {
     expect(find.text('Screen D'), findsOneWidget);
     expect(find.text('Screen C'), findsOneWidget);
   });
+
+  test('ShellRoute parent navigator key throw if not match', () async {
+    final GlobalKey<NavigatorState> key1 = GlobalKey<NavigatorState>();
+    final GlobalKey<NavigatorState> key2 = GlobalKey<NavigatorState>();
+    bool hasError = false;
+    try {
+      ShellRoute(
+        navigatorKey: key1,
+        builder: (_, __, Widget child) => child,
+        routes: <RouteBase>[
+          ShellRoute(
+            parentNavigatorKey: key2,
+            builder: (_, __, Widget child) => child,
+            routes: <RouteBase>[
+              GoRoute(
+                path: '1',
+                builder: (_, __) => const Text('/route/1'),
+              ),
+            ],
+          ),
+        ],
+      );
+    } on AssertionError catch (_) {
+      hasError = true;
+    }
+    expect(hasError, isTrue);
+  });
+
+  group('Redirect only GoRoute', () {
+    testWidgets('can redirect to subroute', (WidgetTester tester) async {
+      final GoRouter router = await createRouter(
+        <RouteBase>[
+          GoRoute(
+            path: '/',
+            builder: (_, __) => const Text('home'),
+            routes: <RouteBase>[
+              GoRoute(
+                path: 'route',
+                redirect: (_, __) => '/route/1',
+                routes: <RouteBase>[
+                  GoRoute(
+                    path: '1',
+                    builder: (_, __) => const Text('/route/1'),
+                  ),
+                ],
+              ),
+            ],
+          ),
+        ],
+        tester,
+      );
+      expect(find.text('home'), findsOneWidget);
+
+      router.go('/route');
+      await tester.pumpAndSettle();
+      // Should redirect to /route/1 without error.
+      expect(find.text('/route/1'), findsOneWidget);
+
+      router.pop();
+      await tester.pumpAndSettle();
+      // Should go back directly to home page.
+      expect(find.text('home'), findsOneWidget);
+    });
+
+    testWidgets('throw if redirect to itself.', (WidgetTester tester) async {
+      final GoRouter router = await createRouter(
+        <RouteBase>[
+          GoRoute(
+            path: '/',
+            builder: (_, __) => const Text('home'),
+            routes: <RouteBase>[
+              GoRoute(
+                path: 'route',
+                redirect: (_, __) => '/route',
+                routes: <RouteBase>[
+                  GoRoute(
+                    path: '1',
+                    builder: (_, __) => const Text('/route/1'),
+                  ),
+                ],
+              ),
+            ],
+          ),
+        ],
+        tester,
+      );
+      expect(find.text('home'), findsOneWidget);
+
+      router.go('/route');
+      await tester.pumpAndSettle();
+      // Should redirect to /route/1 without error.
+      expect(tester.takeException(), isAssertionError);
+    });
+
+    testWidgets('throw if sub route does not conform with parent navigator key',
+        (WidgetTester tester) async {
+      final GlobalKey<NavigatorState> key1 = GlobalKey<NavigatorState>();
+      final GlobalKey<NavigatorState> key2 = GlobalKey<NavigatorState>();
+      bool hasError = false;
+      try {
+        ShellRoute(
+          navigatorKey: key1,
+          builder: (_, __, Widget child) => child,
+          routes: <RouteBase>[
+            GoRoute(
+              path: '/',
+              redirect: (_, __) => '/route',
+              routes: <RouteBase>[
+                GoRoute(
+                  parentNavigatorKey: key2,
+                  path: 'route',
+                  builder: (_, __) => const Text('/route/1'),
+                ),
+              ],
+            ),
+          ],
+        );
+      } on AssertionError catch (_) {
+        hasError = true;
+      }
+      expect(hasError, isTrue);
+    });
+  });
 }