From e8670f24cd49b3e5aa3c5cb506b230ba842ccacd Mon Sep 17 00:00:00 2001 From: Chun-Heng Tai Date: Mon, 26 Jun 2023 13:20:44 -0700 Subject: [PATCH 1/2] [go_router] Allows redirect only GoRoute to be part of RouteMatchList --- packages/go_router/CHANGELOG.md | 4 + packages/go_router/lib/src/builder.dart | 38 +++--- packages/go_router/lib/src/match.dart | 7 +- packages/go_router/lib/src/parser.dart | 8 ++ packages/go_router/lib/src/route.dart | 73 +++++++++-- packages/go_router/test/go_route_test.dart | 123 +++++++++++++++++++ packages/go_router/test/matching_test.dart | 5 +- packages/go_router/test/route_data_test.dart | 8 +- 8 files changed, 225 insertions(+), 41 deletions(-) diff --git a/packages/go_router/CHANGELOG.md b/packages/go_router/CHANGELOG.md index c41a7d2fb3bb..43e0ff6fdba0 100644 --- a/packages/go_router/CHANGELOG.md +++ b/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**: diff --git a/packages/go_router/lib/src/builder.dart b/packages/go_router/lib/src/builder.dart index 26d70ae9212a..774616519fc0 100644 --- a/packages/go_router/lib/src/builder.dart +++ b/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, () => >[]).add(page); + assert(page != null || route.redirectOnly); + if (page != null) { + keyToPages + .putIfAbsent(routeNavKey, () => >[]) + .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 _buildPageForGoRoute(BuildContext context, GoRouterState state, + Page? _buildPageForGoRoute(BuildContext context, GoRouterState state, RouteMatch match, GoRoute route, _PagePopContext pagePopContext) { - Page? 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 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? _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] diff --git a/packages/go_router/lib/src/match.dart b/packages/go_router/lib/src/match.dart index 865ab0b4108b..9106b1cf8a19 100644 --- a/packages/go_router/lib/src/match.dart +++ b/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. diff --git a/packages/go_router/lib/src/parser.dart b/packages/go_router/lib/src/parser.dart index 1a9dccae36d0..778d099f7d56 100644 --- a/packages/go_router/lib/src/parser.dart +++ b/packages/go_router/lib/src/parser.dart @@ -98,6 +98,14 @@ class GoRouteInformationParser extends RouteInformationParser { 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, diff --git a/packages/go_router/lib/src/route.dart b/packages/go_router/lib/src/route.dart index 9d33e8866215..b7ba3affb559 100644 --- a/packages/go_router/lib/src/route.dart +++ b/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) => [e, ...routesRecursively(e.routes)]); } + + @override + void debugFillProperties(DiagnosticPropertiesBuilder properties) { + super.debugFillProperties(properties); + if (parentNavigatorKey != null) { + properties.add(DiagnosticsProperty>( + '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 pathParameters = []; @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 subRoutes, GlobalKey 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(), 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> get _navigatorKeys => >[navigatorKey]; + + @override + void debugFillProperties(DiagnosticPropertiesBuilder properties) { + super.debugFillProperties(properties); + properties.add(DiagnosticsProperty>( + '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>>( + '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(); + }) : navigatorKey = navigatorKey ?? GlobalKey() { + assert(() { + ShellRouteBase._debugCheckSubRouteParentNavigatorKeys( + routes, this.navigatorKey); + return true; + }()); + } /// The [GlobalKey] to be used by the [Navigator] built for this branch. /// diff --git a/packages/go_router/test/go_route_test.dart b/packages/go_router/test/go_route_test.dart index f7ffe7001bc1..0ff437c0c1c7 100644 --- a/packages/go_router/test/go_route_test.dart +++ b/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 key1 = GlobalKey(); + final GlobalKey key2 = GlobalKey(); + bool hasError = false; + try { + ShellRoute( + navigatorKey: key1, + builder: (_, __, Widget child) => child, + routes: [ + ShellRoute( + parentNavigatorKey: key2, + builder: (_, __, Widget child) => child, + routes: [ + 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( + [ + GoRoute( + path: '/', + builder: (_, __) => const Text('home'), + routes: [ + GoRoute( + path: 'route', + redirect: (_, __) => '/route/1', + routes: [ + 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( + [ + GoRoute( + path: '/', + builder: (_, __) => const Text('home'), + routes: [ + GoRoute( + path: 'route', + redirect: (_, __) => '/route', + routes: [ + 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 key1 = GlobalKey(); + final GlobalKey key2 = GlobalKey(); + bool hasError = false; + try { + ShellRoute( + navigatorKey: key1, + builder: (_, __, Widget child) => child, + routes: [ + GoRoute( + path: '/', + redirect: (_, __) => '/route', + routes: [ + GoRoute( + parentNavigatorKey: key2, + path: 'route', + builder: (_, __) => const Text('/route/1'), + ), + ], + ), + ], + ); + } on AssertionError catch (_) { + hasError = true; + } + expect(hasError, isTrue); + }); + }); } diff --git a/packages/go_router/test/matching_test.dart b/packages/go_router/test/matching_test.dart index 7784b851d787..640a467b900b 100644 --- a/packages/go_router/test/matching_test.dart +++ b/packages/go_router/test/matching_test.dart @@ -22,9 +22,8 @@ void main() { const Placeholder()), ]; - final GoRouter router = await createRouter(routes, tester); - router.go('/page-0'); - await tester.pumpAndSettle(); + final GoRouter router = + await createRouter(routes, tester, initialLocation: '/page-0'); final RouteMatchList matches = router.routerDelegate.currentConfiguration; expect(matches.toString(), contains('/page-0')); diff --git a/packages/go_router/test/route_data_test.dart b/packages/go_router/test/route_data_test.dart index 15419406f645..43299ec5402c 100644 --- a/packages/go_router/test/route_data_test.dart +++ b/packages/go_router/test/route_data_test.dart @@ -189,9 +189,7 @@ void main() { routes: _routes, ); await tester.pumpWidget(MaterialApp.router( - routeInformationProvider: goRouter.routeInformationProvider, - routeInformationParser: goRouter.routeInformationParser, - routerDelegate: goRouter.routerDelegate, + routerConfig: goRouter, )); expect(find.byKey(const Key('build')), findsNothing); expect(find.byKey(const Key('buildPage')), findsOneWidget); @@ -206,9 +204,7 @@ void main() { routes: _routes, ); await tester.pumpWidget(MaterialApp.router( - routeInformationProvider: goRouter.routeInformationProvider, - routeInformationParser: goRouter.routeInformationParser, - routerDelegate: goRouter.routerDelegate, + routerConfig: goRouter, )); expect(find.byKey(const Key('build')), findsNothing); expect(find.byKey(const Key('buildPage')), findsNothing); From 27deebced9b7d79aba4ea639020331b24b3c960f Mon Sep 17 00:00:00 2001 From: Chun-Heng Tai Date: Mon, 26 Jun 2023 13:22:48 -0700 Subject: [PATCH 2/2] bump version --- packages/go_router/pubspec.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/go_router/pubspec.yaml b/packages/go_router/pubspec.yaml index f977eb998475..83bebb1764da 100644 --- a/packages/go_router/pubspec.yaml +++ b/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