Skip to content

Commit

Permalink
[go_router] Relax subroute path requirements (allow root and child ro…
Browse files Browse the repository at this point in the history
…utes to have the same path forms ) (flutter#7647)

Before this change `GoRoute` constructor allowed Schroedinger routes which were neither in a valid state nor an invalid state until they were used. EG: `GoRoute(path: '/some-route')`  was valid as a top route but invalid as a child route. This prevents routes from being moved around and prevents building upon go router. 

To solve this I see two solution:

  1. Breaking change: Require all routes to start with `/` (or none)
  2. Non breaking: Allow all routes to start or not with `/`

Since I did not want to introduce a breaking change I followed option 2, which allows all routes to be of the form `/some-route` or `some-route`. However, breaking changes aside, I believe it would be better to use solution 1 as it's more predictible.

@chunhtai What do you think about those options ? 

Related issue: Fix flutter/flutter#145867
  • Loading branch information
cedvdb authored Oct 3, 2024
1 parent 429650f commit 6058294
Show file tree
Hide file tree
Showing 8 changed files with 115 additions and 69 deletions.
4 changes: 4 additions & 0 deletions packages/go_router/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 14.2.9

- Relaxes route path requirements. Both root and child routes can now start with or without '/'.

## 14.2.8

- Updated custom_stateful_shell_route example to better support swiping in TabView as well as demonstration of the use of PageView.
Expand Down
9 changes: 3 additions & 6 deletions packages/go_router/lib/src/configuration.dart
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,9 @@ 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');
if (route.path != '/') {
assert(!route.path.endsWith('/'),
'route path may not end with "/" except for the top "/" route. Found: $route');
}
subRouteIsTopLevel = false;
} else if (route is ShellRouteBase) {
Expand Down
12 changes: 4 additions & 8 deletions packages/go_router/lib/src/match.dart
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,7 @@ abstract class RouteMatchBase with Diagnosticable {

final RegExpMatch? regExpMatch =
route.matchPatternAsPrefix(remainingLocation);

if (regExpMatch == null) {
return _empty;
}
Expand Down Expand Up @@ -555,13 +556,9 @@ class RouteMatchList with Diagnosticable {
/// [RouteMatchA(), RouteMatchB(), RouteMatchC()]
/// ```
static String _generateFullPath(Iterable<RouteMatchBase> 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;
Expand All @@ -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.
Expand Down
20 changes: 7 additions & 13 deletions packages/go_router/lib/src/path_utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -102,20 +102,14 @@ Map<String, String> 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<String> segments = <String>[
...parentPath.split('/'),
...childPath.split('/')
].where((String segment) => segment.isNotEmpty);
return '/${segments.join('/')}';
}

/// Builds an absolute path for the provided route.
Expand Down
6 changes: 4 additions & 2 deletions packages/go_router/lib/src/route.dart
Original file line number Diff line number Diff line change
Expand Up @@ -432,8 +432,10 @@ 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) {
return _pathRE.matchAsPrefix('/$loc') as RegExpMatch? ??
_pathRE.matchAsPrefix(loc) as RegExpMatch?;
}

/// Extract the path parameters from a match.
Map<String, String> extractPathParams(RegExpMatch match) =>
Expand Down
2 changes: 1 addition & 1 deletion packages/go_router/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -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

Expand Down
117 changes: 88 additions & 29 deletions packages/go_router/test/go_router_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -96,25 +96,6 @@ void main() {
}, throwsA(isAssertionError));
});

test('leading / on sub-route', () {
expect(() {
GoRouter(
routes: <RouteBase>[
GoRoute(
path: '/',
builder: dummy,
routes: <GoRoute>[
GoRoute(
path: '/foo',
builder: dummy,
),
],
),
],
);
}, throwsA(isAssertionError));
});

test('trailing / on sub-route', () {
expect(() {
GoRouter(
Expand All @@ -134,16 +115,6 @@ void main() {
}, throwsA(isAssertionError));
});

testWidgets('lack of leading / on top-level route',
(WidgetTester tester) async {
await expectLater(() async {
final List<GoRoute> routes = <GoRoute>[
GoRoute(path: 'foo', builder: dummy),
];
await createRouter(routes, tester);
}, throwsA(isAssertionError));
});

testWidgets('match no routes', (WidgetTester tester) async {
final List<GoRoute> routes = <GoRoute>[
GoRoute(path: '/', builder: dummy),
Expand Down Expand Up @@ -5502,6 +5473,94 @@ void main() {
),
);
});

testWidgets('should allow route paths without leading /',
(WidgetTester tester) async {
final List<GoRoute> routes = <GoRoute>[
GoRoute(
path: '/', // root cannot be empty (existing assert)
builder: (BuildContext context, GoRouterState state) =>
const HomeScreen(),
routes: <RouteBase>[
GoRoute(
path: 'child-route',
builder: (BuildContext context, GoRouterState state) =>
const Text('/child-route'),
routes: <RouteBase>[
GoRoute(
path: 'grand-child-route',
builder: (BuildContext context, GoRouterState state) =>
const Text('/grand-child-route'),
),
GoRoute(
path: 'redirected-grand-child-route',
redirect: (BuildContext context, GoRouterState state) =>
'/child-route',
),
],
)
],
),
];

final GoRouter router = await createRouter(routes, tester,
initialLocation: '/child-route/grand-child-route');
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 /',
(WidgetTester tester) async {
final List<GoRoute> routes = <GoRoute>[
GoRoute(
path: '/',
builder: (BuildContext context, GoRouterState state) =>
const HomeScreen(),
routes: <RouteBase>[
GoRoute(
path: '/child-route',
builder: (BuildContext context, GoRouterState state) =>
const Text('/child-route'),
routes: <RouteBase>[
GoRoute(
path: '/grand-child-route',
builder: (BuildContext context, GoRouterState state) =>
const Text('/grand-child-route'),
),
GoRoute(
path: '/redirected-grand-child-route',
redirect: (BuildContext context, GoRouterState state) =>
'/child-route',
),
],
)
],
),
];

final GoRouter router = await createRouter(routes, tester,
initialLocation: '/child-route/grand-child-route');
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);
});
}

class TestInheritedNotifier extends InheritedNotifier<ValueNotifier<String>> {
Expand Down
14 changes: 4 additions & 10 deletions packages/go_router/test/path_utils_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -79,17 +79,11 @@ void main() {
expect(result, expected);
}

void verifyThrows(String pathA, String pathB) {
expect(
() => concatenatePaths(pathA, pathB), throwsA(isA<AssertionError>()));
}

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('', '', '/');
});
}

0 comments on commit 6058294

Please sign in to comment.