From dd1e99d94cbb2ca9721f2d2ddbc69b8aa7783312 Mon Sep 17 00:00:00 2001 From: cedvdb Date: Thu, 3 Oct 2024 17:08:29 +0200 Subject: [PATCH 01/11] rebase --- packages/go_router/CHANGELOG.md | 3 ++ packages/go_router/lib/src/configuration.dart | 7 --- packages/go_router/lib/src/path_utils.dart | 20 +++---- packages/go_router/pubspec.yaml | 2 +- packages/go_router/test/go_router_test.dart | 54 ------------------- packages/go_router/test/path_utils_test.dart | 14 ++--- 6 files changed, 15 insertions(+), 85 deletions(-) diff --git a/packages/go_router/CHANGELOG.md b/packages/go_router/CHANGELOG.md index ef70f93afed4..9701d7dce74e 100644 --- a/packages/go_router/CHANGELOG.md +++ b/packages/go_router/CHANGELOG.md @@ -1,3 +1,6 @@ +## 14.2.9 +- Relax route path requirements. Both root and child routes can now start with '/'. + ## 14.2.8 - Updated custom_stateful_shell_route example to better support swiping in TabView as well as demonstration of the use of PageView. diff --git a/packages/go_router/lib/src/configuration.dart b/packages/go_router/lib/src/configuration.dart index 8be984ecb6f5..ecb8382b0a67 100644 --- a/packages/go_router/lib/src/configuration.dart +++ b/packages/go_router/lib/src/configuration.dart @@ -36,13 +36,6 @@ class RouteConfiguration { for (final RouteBase route in routes) { late bool subRouteIsTopLevel; if (route is GoRoute) { - if (isTopLevel) { - assert(route.path.startsWith('/'), - 'top-level path must start with "/": $route'); - } else { - assert(!route.path.startsWith('/') && !route.path.endsWith('/'), - 'sub-route path may not start or end with "/": $route'); - } subRouteIsTopLevel = false; } else if (route is ShellRouteBase) { subRouteIsTopLevel = isTopLevel; diff --git a/packages/go_router/lib/src/path_utils.dart b/packages/go_router/lib/src/path_utils.dart index 872f7786b263..4684c75badc4 100644 --- a/packages/go_router/lib/src/path_utils.dart +++ b/packages/go_router/lib/src/path_utils.dart @@ -102,20 +102,14 @@ Map extractPathParameters( /// Concatenates two paths. /// -/// e.g: pathA = /a, pathB = c/d, concatenatePaths(pathA, pathB) = /a/c/d. +/// e.g: pathA = /a, pathB = /c/d, concatenatePaths(pathA, pathB) = /a/c/d. +/// or: pathA = a, pathB = c/d, concatenatePaths(pathA, pathB) = /a/c/d. String concatenatePaths(String parentPath, String childPath) { - // at the root, just return the path - if (parentPath.isEmpty) { - assert(childPath.startsWith('/')); - assert(childPath == '/' || !childPath.endsWith('/')); - return childPath; - } - - // not at the root, so append the parent path - assert(childPath.isNotEmpty); - assert(!childPath.startsWith('/')); - assert(!childPath.endsWith('/')); - return '${parentPath == '/' ? '' : parentPath}/$childPath'; + final Iterable segments = [ + ...parentPath.split('/'), + ...childPath.split('/') + ].where((String segment) => segment.isNotEmpty); + return '/${segments.join('/')}'; } /// Builds an absolute path for the provided route. diff --git a/packages/go_router/pubspec.yaml b/packages/go_router/pubspec.yaml index 7c494f15aea1..e27cb29dbb35 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: 14.2.8 +version: 14.2.9 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 diff --git a/packages/go_router/test/go_router_test.dart b/packages/go_router/test/go_router_test.dart index 10e6bb58d5bd..d694abb9e018 100644 --- a/packages/go_router/test/go_router_test.dart +++ b/packages/go_router/test/go_router_test.dart @@ -90,60 +90,6 @@ void main() { expect(router.routerDelegate.currentConfiguration.uri.path, '/b'); }); - test('empty path', () { - expect(() { - GoRoute(path: ''); - }, throwsA(isAssertionError)); - }); - - test('leading / on sub-route', () { - expect(() { - GoRouter( - routes: [ - GoRoute( - path: '/', - builder: dummy, - routes: [ - GoRoute( - path: '/foo', - builder: dummy, - ), - ], - ), - ], - ); - }, throwsA(isAssertionError)); - }); - - test('trailing / on sub-route', () { - expect(() { - GoRouter( - routes: [ - GoRoute( - path: '/', - builder: dummy, - routes: [ - GoRoute( - path: 'foo/', - builder: dummy, - ), - ], - ), - ], - ); - }, throwsA(isAssertionError)); - }); - - testWidgets('lack of leading / on top-level route', - (WidgetTester tester) async { - await expectLater(() async { - final List routes = [ - GoRoute(path: 'foo', builder: dummy), - ]; - await createRouter(routes, tester); - }, throwsA(isAssertionError)); - }); - testWidgets('match no routes', (WidgetTester tester) async { final List routes = [ GoRoute(path: '/', builder: dummy), diff --git a/packages/go_router/test/path_utils_test.dart b/packages/go_router/test/path_utils_test.dart index a495e4d1a052..5b0b1234329b 100644 --- a/packages/go_router/test/path_utils_test.dart +++ b/packages/go_router/test/path_utils_test.dart @@ -79,17 +79,11 @@ void main() { expect(result, expected); } - void verifyThrows(String pathA, String pathB) { - expect( - () => concatenatePaths(pathA, pathB), throwsA(isA())); - } - verify('/a', 'b/c', '/a/b/c'); verify('/', 'b', '/b'); - verifyThrows('/a', '/b'); - verifyThrows('/a', '/'); - verifyThrows('/', '/'); - verifyThrows('/', ''); - verifyThrows('', ''); + verify('/a', '/b/c/', '/a/b/c'); + verify('/a', 'b/c', '/a/b/c'); + verify('/', '/', '/'); + verify('', '', '/'); }); } From a7a29e3765f7e9199c06c61e77f4233baaab815f Mon Sep 17 00:00:00 2001 From: cedvdb Date: Sat, 14 Sep 2024 00:17:20 +0200 Subject: [PATCH 02/11] add tests --- packages/go_router/test/go_router_test.dart | 64 +++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/packages/go_router/test/go_router_test.dart b/packages/go_router/test/go_router_test.dart index d694abb9e018..439197db5bde 100644 --- a/packages/go_router/test/go_router_test.dart +++ b/packages/go_router/test/go_router_test.dart @@ -5448,6 +5448,70 @@ void main() { ), ); }); + + testWidgets('should allow route paths without leading /', + (WidgetTester tester) async { + final List routes = [ + GoRoute( + path: '/', // root cannot be empty (existing assert) + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + routes: [ + GoRoute( + path: 'child-route', + builder: (BuildContext context, GoRouterState state) => + const Text('/child-route'), + routes: [ + GoRoute( + path: 'grand-child-route', + builder: (BuildContext context, GoRouterState state) => + const Text('/grand-child-route'), + ), + ], + ) + ], + ), + ]; + + final GoRouter router = await createRouter(routes, tester, + initialLocation: '/child-route/grand-child-route'); + final RouteMatchList matches = router.routerDelegate.currentConfiguration; + expect(matches.matches, hasLength(3)); + expect(matches.uri.toString(), '/child-route/grand-child-route'); + expect(find.text('/grand-child-route'), findsOneWidget); + }); + + testWidgets('should allow route paths with leading /', + (WidgetTester tester) async { + final List routes = [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + routes: [ + GoRoute( + path: '/child-route', + builder: (BuildContext context, GoRouterState state) => + const Text('/child-route'), + routes: [ + GoRoute( + path: '/grand-child-route', + builder: (BuildContext context, GoRouterState state) => + const Text('/grand-child-route'), + ), + ], + ) + ], + ), + ]; + + final GoRouter router = await createRouter(routes, tester, + initialLocation: '/child-route/grand-child-route'); + final RouteMatchList matches = router.routerDelegate.currentConfiguration; + expect(matches.matches, hasLength(3)); + expect(matches.uri.toString(), '/child-route/grand-child-route'); + expect(find.text('/grand-child-route'), findsOneWidget); + }); } class TestInheritedNotifier extends InheritedNotifier> { From 35e56e70717cc1c7134789228399632650665da5 Mon Sep 17 00:00:00 2001 From: cedvdb Date: Sun, 15 Sep 2024 15:08:14 +0200 Subject: [PATCH 03/11] tests passing --- packages/go_router/lib/src/match.dart | 12 ++++-------- packages/go_router/lib/src/route.dart | 12 +++++++++--- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/packages/go_router/lib/src/match.dart b/packages/go_router/lib/src/match.dart index 4d24416284ec..696f9d4c50d9 100644 --- a/packages/go_router/lib/src/match.dart +++ b/packages/go_router/lib/src/match.dart @@ -203,6 +203,7 @@ abstract class RouteMatchBase with Diagnosticable { final RegExpMatch? regExpMatch = route.matchPatternAsPrefix(remainingLocation); + if (regExpMatch == null) { return _empty; } @@ -555,13 +556,9 @@ class RouteMatchList with Diagnosticable { /// [RouteMatchA(), RouteMatchB(), RouteMatchC()] /// ``` static String _generateFullPath(Iterable matches) { - final StringBuffer buffer = StringBuffer(); - bool addsSlash = false; + String fullPath = ''; for (final RouteMatchBase match in matches .where((RouteMatchBase match) => match is! ImperativeRouteMatch)) { - if (addsSlash) { - buffer.write('/'); - } final String pathSegment; if (match is RouteMatch) { pathSegment = match.route.path; @@ -571,10 +568,9 @@ class RouteMatchList with Diagnosticable { assert(false, 'Unexpected match type: $match'); continue; } - buffer.write(pathSegment); - addsSlash = pathSegment.isNotEmpty && (addsSlash || pathSegment != '/'); + fullPath = concatenatePaths(fullPath, pathSegment); } - return buffer.toString(); + return fullPath; } /// Returns true if there are no matches. diff --git a/packages/go_router/lib/src/route.dart b/packages/go_router/lib/src/route.dart index 0183f91ea543..3c35e774fa74 100644 --- a/packages/go_router/lib/src/route.dart +++ b/packages/go_router/lib/src/route.dart @@ -261,7 +261,7 @@ class GoRoute extends RouteBase { /// - [path] and [name] cannot be empty strings. /// - One of either [builder] or [pageBuilder] must be provided. GoRoute({ - required this.path, + required String path, this.name, this.builder, this.pageBuilder, @@ -275,6 +275,7 @@ class GoRoute extends RouteBase { 'builder, pageBuilder, or redirect must be provided'), assert(onExit == null || pageBuilder != null || builder != null, 'if onExit is provided, one of pageBuilder or builder must be provided'), + path = path, super._() { // cache the path regexp and parameters _pathRE = patternToRegExp(path, pathParameters); @@ -432,8 +433,13 @@ class GoRoute extends RouteBase { // TODO(chunhtai): move all regex related help methods to path_utils.dart. /// Match this route against a location. - RegExpMatch? matchPatternAsPrefix(String loc) => - _pathRE.matchAsPrefix(loc) as RegExpMatch?; + RegExpMatch? matchPatternAsPrefix(String loc) { + if (!loc.startsWith('/') && _pathRE.hasMatch('/$loc')) { + loc = '/$loc'; + } + return _pathRE.matchAsPrefix('/$loc') as RegExpMatch? ?? + _pathRE.matchAsPrefix(loc) as RegExpMatch?; + } /// Extract the path parameters from a match. Map extractPathParams(RegExpMatch match) => From 5e7e549d94a45a98ac86a2557924739c5a8bbcde Mon Sep 17 00:00:00 2001 From: cedvdb Date: Sun, 15 Sep 2024 15:20:36 +0200 Subject: [PATCH 04/11] restore ending / assert --- packages/go_router/lib/src/configuration.dart | 4 ++++ packages/go_router/lib/src/route.dart | 3 +-- packages/go_router/test/go_router_test.dart | 19 +++++++++++++++++++ 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/packages/go_router/lib/src/configuration.dart b/packages/go_router/lib/src/configuration.dart index ecb8382b0a67..51c1726973f9 100644 --- a/packages/go_router/lib/src/configuration.dart +++ b/packages/go_router/lib/src/configuration.dart @@ -36,6 +36,10 @@ class RouteConfiguration { for (final RouteBase route in routes) { late bool subRouteIsTopLevel; if (route is GoRoute) { + if (route.path != '/') { + assert(!route.path.endsWith('/'), + 'route path may not end with "/" except for the top "/" root. Found: $route'); + } subRouteIsTopLevel = false; } else if (route is ShellRouteBase) { subRouteIsTopLevel = isTopLevel; diff --git a/packages/go_router/lib/src/route.dart b/packages/go_router/lib/src/route.dart index 3c35e774fa74..23dab162e498 100644 --- a/packages/go_router/lib/src/route.dart +++ b/packages/go_router/lib/src/route.dart @@ -261,7 +261,7 @@ class GoRoute extends RouteBase { /// - [path] and [name] cannot be empty strings. /// - One of either [builder] or [pageBuilder] must be provided. GoRoute({ - required String path, + required this.path, this.name, this.builder, this.pageBuilder, @@ -275,7 +275,6 @@ class GoRoute extends RouteBase { 'builder, pageBuilder, or redirect must be provided'), assert(onExit == null || pageBuilder != null || builder != null, 'if onExit is provided, one of pageBuilder or builder must be provided'), - path = path, super._() { // cache the path regexp and parameters _pathRE = patternToRegExp(path, pathParameters); diff --git a/packages/go_router/test/go_router_test.dart b/packages/go_router/test/go_router_test.dart index 439197db5bde..cf9420624aba 100644 --- a/packages/go_router/test/go_router_test.dart +++ b/packages/go_router/test/go_router_test.dart @@ -90,6 +90,25 @@ void main() { expect(router.routerDelegate.currentConfiguration.uri.path, '/b'); }); + test('trailing / on sub-route', () { + expect(() { + GoRouter( + routes: [ + GoRoute( + path: '/', + builder: dummy, + routes: [ + GoRoute( + path: 'foo/', + builder: dummy, + ), + ], + ), + ], + ); + }, throwsA(isAssertionError)); + }); + testWidgets('match no routes', (WidgetTester tester) async { final List routes = [ GoRoute(path: '/', builder: dummy), From 1c9374694ce0a01d3d123be3c52951c65e3a23f8 Mon Sep 17 00:00:00 2001 From: cedvdb Date: Sun, 15 Sep 2024 15:21:30 +0200 Subject: [PATCH 05/11] fix typo --- packages/go_router/lib/src/configuration.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/go_router/lib/src/configuration.dart b/packages/go_router/lib/src/configuration.dart index 51c1726973f9..cc671066218d 100644 --- a/packages/go_router/lib/src/configuration.dart +++ b/packages/go_router/lib/src/configuration.dart @@ -38,7 +38,7 @@ class RouteConfiguration { if (route is GoRoute) { if (route.path != '/') { assert(!route.path.endsWith('/'), - 'route path may not end with "/" except for the top "/" root. Found: $route'); + 'route path may not end with "/" except for the top "/" route. Found: $route'); } subRouteIsTopLevel = false; } else if (route is ShellRouteBase) { From fb78383af482badee409f78a3353d503e9e932e6 Mon Sep 17 00:00:00 2001 From: cedvdb Date: Sun, 15 Sep 2024 15:30:20 +0200 Subject: [PATCH 06/11] resolve analyze --- packages/go_router/test/go_router_test.dart | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/go_router/test/go_router_test.dart b/packages/go_router/test/go_router_test.dart index cf9420624aba..6d130ce38b1b 100644 --- a/packages/go_router/test/go_router_test.dart +++ b/packages/go_router/test/go_router_test.dart @@ -5475,12 +5475,12 @@ void main() { path: '/', // root cannot be empty (existing assert) builder: (BuildContext context, GoRouterState state) => const HomeScreen(), - routes: [ + routes: [ GoRoute( path: 'child-route', builder: (BuildContext context, GoRouterState state) => const Text('/child-route'), - routes: [ + routes: [ GoRoute( path: 'grand-child-route', builder: (BuildContext context, GoRouterState state) => @@ -5507,12 +5507,12 @@ void main() { path: '/', builder: (BuildContext context, GoRouterState state) => const HomeScreen(), - routes: [ + routes: [ GoRoute( path: '/child-route', builder: (BuildContext context, GoRouterState state) => const Text('/child-route'), - routes: [ + routes: [ GoRoute( path: '/grand-child-route', builder: (BuildContext context, GoRouterState state) => From a5a6e50864aea430084261617a600f2c9d92427e Mon Sep 17 00:00:00 2001 From: cedvdb Date: Sun, 15 Sep 2024 15:35:24 +0200 Subject: [PATCH 07/11] remove unnecessary code --- packages/go_router/lib/src/route.dart | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/go_router/lib/src/route.dart b/packages/go_router/lib/src/route.dart index 23dab162e498..6fa9b8bb7489 100644 --- a/packages/go_router/lib/src/route.dart +++ b/packages/go_router/lib/src/route.dart @@ -433,9 +433,6 @@ class GoRoute extends RouteBase { // TODO(chunhtai): move all regex related help methods to path_utils.dart. /// Match this route against a location. RegExpMatch? matchPatternAsPrefix(String loc) { - if (!loc.startsWith('/') && _pathRE.hasMatch('/$loc')) { - loc = '/$loc'; - } return _pathRE.matchAsPrefix('/$loc') as RegExpMatch? ?? _pathRE.matchAsPrefix(loc) as RegExpMatch?; } From b2a0df7b39ef8798fd3efd8bce4c2109336e9f67 Mon Sep 17 00:00:00 2001 From: cedvdb Date: Sun, 15 Sep 2024 15:47:06 +0200 Subject: [PATCH 08/11] restore empty path test --- packages/go_router/test/go_router_test.dart | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/go_router/test/go_router_test.dart b/packages/go_router/test/go_router_test.dart index 6d130ce38b1b..4e142602263c 100644 --- a/packages/go_router/test/go_router_test.dart +++ b/packages/go_router/test/go_router_test.dart @@ -90,6 +90,12 @@ void main() { expect(router.routerDelegate.currentConfiguration.uri.path, '/b'); }); + test('empty path', () { + expect(() { + GoRoute(path: ''); + }, throwsA(isAssertionError)); + }); + test('trailing / on sub-route', () { expect(() { GoRouter( From e03887c9fd2b76d5b738ea7db64cebc8df14257d Mon Sep 17 00:00:00 2001 From: cedvdb Date: Mon, 16 Sep 2024 22:20:52 +0200 Subject: [PATCH 09/11] add redirect to test --- packages/go_router/test/go_router_test.dart | 28 +++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/packages/go_router/test/go_router_test.dart b/packages/go_router/test/go_router_test.dart index 4e142602263c..e2c2d1b5cfc1 100644 --- a/packages/go_router/test/go_router_test.dart +++ b/packages/go_router/test/go_router_test.dart @@ -5492,6 +5492,11 @@ void main() { builder: (BuildContext context, GoRouterState state) => const Text('/grand-child-route'), ), + GoRoute( + path: 'redirected-grand-child-route', + redirect: (BuildContext context, GoRouterState state) => + '/child-route', + ), ], ) ], @@ -5500,10 +5505,17 @@ void main() { final GoRouter router = await createRouter(routes, tester, initialLocation: '/child-route/grand-child-route'); - final RouteMatchList matches = router.routerDelegate.currentConfiguration; + RouteMatchList matches = router.routerDelegate.currentConfiguration; expect(matches.matches, hasLength(3)); expect(matches.uri.toString(), '/child-route/grand-child-route'); expect(find.text('/grand-child-route'), findsOneWidget); + + router.go('/child-route/redirected-grand-child-route'); + await tester.pumpAndSettle(); + matches = router.routerDelegate.currentConfiguration; + expect(matches.matches, hasLength(2)); + expect(matches.uri.toString(), '/child-route'); + expect(find.text('/child-route'), findsOneWidget); }); testWidgets('should allow route paths with leading /', @@ -5524,6 +5536,11 @@ void main() { builder: (BuildContext context, GoRouterState state) => const Text('/grand-child-route'), ), + GoRoute( + path: '/redirected-grand-child-route', + redirect: (BuildContext context, GoRouterState state) => + '/child-route', + ), ], ) ], @@ -5532,10 +5549,17 @@ void main() { final GoRouter router = await createRouter(routes, tester, initialLocation: '/child-route/grand-child-route'); - final RouteMatchList matches = router.routerDelegate.currentConfiguration; + RouteMatchList matches = router.routerDelegate.currentConfiguration; expect(matches.matches, hasLength(3)); expect(matches.uri.toString(), '/child-route/grand-child-route'); expect(find.text('/grand-child-route'), findsOneWidget); + + router.go('/child-route/redirected-grand-child-route'); + await tester.pumpAndSettle(); + matches = router.routerDelegate.currentConfiguration; + expect(matches.matches, hasLength(2)); + expect(matches.uri.toString(), '/child-route'); + expect(find.text('/child-route'), findsOneWidget); }); } From 640b7895a1327ccd0246e2a56a505b47772c8f4b Mon Sep 17 00:00:00 2001 From: cedvdb Date: Thu, 3 Oct 2024 17:20:59 +0200 Subject: [PATCH 10/11] add return to new line --- packages/go_router/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/go_router/CHANGELOG.md b/packages/go_router/CHANGELOG.md index 9701d7dce74e..5403cfe9c7b6 100644 --- a/packages/go_router/CHANGELOG.md +++ b/packages/go_router/CHANGELOG.md @@ -1,4 +1,5 @@ ## 14.2.9 + - Relax route path requirements. Both root and child routes can now start with '/'. ## 14.2.8 From cf7de603d5d02f1e85466d13a886b56ad8a7a033 Mon Sep 17 00:00:00 2001 From: cedvdb Date: Thu, 3 Oct 2024 17:24:47 +0200 Subject: [PATCH 11/11] improve changelog --- packages/go_router/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/go_router/CHANGELOG.md b/packages/go_router/CHANGELOG.md index 5403cfe9c7b6..5729e196428c 100644 --- a/packages/go_router/CHANGELOG.md +++ b/packages/go_router/CHANGELOG.md @@ -1,6 +1,6 @@ ## 14.2.9 -- Relax route path requirements. Both root and child routes can now start with '/'. +- Relaxes route path requirements. Both root and child routes can now start with or without '/'. ## 14.2.8