From dff39d7b48e098cff055f472d116e662b358f3c0 Mon Sep 17 00:00:00 2001 From: chunhtai <47866232+chunhtai@users.noreply.github.com> Date: Tue, 5 Apr 2022 09:31:10 -0700 Subject: [PATCH] [go_router]refactor runtime check to assert (#1362) --- packages/go_router/CHANGELOG.md | 4 + packages/go_router/lib/src/go_route.dart | 67 ++++++------- .../go_router/lib/src/go_route_match.dart | 41 ++++---- .../go_router/lib/src/go_router_delegate.dart | 96 ++++++++----------- packages/go_router/pubspec.yaml | 2 +- packages/go_router/test/go_route_test.dart | 6 +- .../test/go_router_delegate_test.dart | 2 +- packages/go_router/test/go_router_test.dart | 20 ++-- 8 files changed, 110 insertions(+), 128 deletions(-) diff --git a/packages/go_router/CHANGELOG.md b/packages/go_router/CHANGELOG.md index b7e3c52d1ae9..f1481a929a9f 100644 --- a/packages/go_router/CHANGELOG.md +++ b/packages/go_router/CHANGELOG.md @@ -1,3 +1,7 @@ +## 3.0.7 + +- Refactors runtime checks to assertions. + ## 3.0.6 - Exports inherited_go_router.dart file. diff --git a/packages/go_router/lib/src/go_route.dart b/packages/go_router/lib/src/go_route.dart index dbe588290b2e..a50831a230cf 100644 --- a/packages/go_router/lib/src/go_route.dart +++ b/packages/go_router/lib/src/go_route.dart @@ -21,51 +21,38 @@ class GoRoute { this.builder = _invalidBuilder, this.routes = const [], this.redirect = _noRedirection, - }) { - if (path.isEmpty) { - throw Exception('GoRoute path cannot be empty'); - } - - if (name != null && name!.isEmpty) { - throw Exception('GoRoute name cannot be empty'); - } - - if (pageBuilder == null && - builder == _invalidBuilder && - redirect == _noRedirection) { - throw Exception( - 'GoRoute builder parameter not set\n' - 'See gorouter.dev/redirection#considerations for details', - ); - } - + }) : assert(path.isNotEmpty, 'GoRoute path cannot be empty'), + assert(name == null || name.isNotEmpty, 'GoRoute name cannot be empty'), + assert( + pageBuilder != null || + builder != _invalidBuilder || + redirect != _noRedirection, + 'GoRoute builder parameter not set\nSee gorouter.dev/redirection#considerations for details') { // cache the path regexp and parameters _pathRE = patternToRegExp(path, _pathParams); - // check path params - final Map> groupedParams = - _pathParams.groupListsBy((String p) => p); - final Map> dupParams = - Map>.fromEntries( - groupedParams.entries - .where((MapEntry> e) => e.value.length > 1), - ); - if (dupParams.isNotEmpty) { - throw Exception( - 'duplicate path params: ${dupParams.keys.join(', ')}', + assert(() { + // check path params + final Map> groupedParams = + _pathParams.groupListsBy((String p) => p); + final Map> dupParams = + Map>.fromEntries( + groupedParams.entries + .where((MapEntry> e) => e.value.length > 1), ); - } - - // check sub-routes - for (final GoRoute route in routes) { - // check paths - if (route.path != '/' && - (route.path.startsWith('/') || route.path.endsWith('/'))) { - throw Exception( - 'sub-route path may not start or end with /: ${route.path}', - ); + assert(dupParams.isEmpty, + 'duplicate path params: ${dupParams.keys.join(', ')}'); + + // check sub-routes + for (final GoRoute route in routes) { + // check paths + assert( + route.path == '/' || + (!route.path.startsWith('/') && !route.path.endsWith('/')), + 'sub-route path may not start or end with /: ${route.path}'); } - } + return true; + }()); } final List _pathParams = []; diff --git a/packages/go_router/lib/src/go_route_match.dart b/packages/go_router/lib/src/go_route_match.dart index e4890436396e..345aa5dfccad 100644 --- a/packages/go_router/lib/src/go_route_match.dart +++ b/packages/go_router/lib/src/go_route_match.dart @@ -25,14 +25,14 @@ class GoRouteMatch { }) : assert(subloc.startsWith('/')), assert(Uri.parse(subloc).queryParameters.isEmpty), assert(fullpath.startsWith('/')), - assert(Uri.parse(fullpath).queryParameters.isEmpty) { - if (kDebugMode) { - for (final MapEntry p in encodedParams.entries) { - assert(p.value == Uri.encodeComponent(Uri.decodeComponent(p.value)), - 'encodedParams[${p.key}] is not encoded properly: "${p.value}"'); - } - } - } + assert(Uri.parse(fullpath).queryParameters.isEmpty), + assert(() { + for (final MapEntry p in encodedParams.entries) { + assert(p.value == Uri.encodeComponent(Uri.decodeComponent(p.value)), + 'encodedParams[${p.key}] is not encoded properly: "${p.value}"'); + } + return true; + }()); // ignore: public_member_api_docs factory GoRouteMatch.matchNamed({ @@ -45,22 +45,21 @@ class GoRouteMatch { }) { assert(route.name != null); assert(route.name!.toLowerCase() == name.toLowerCase()); - - // check that we have all the params we need - final List paramNames = []; - patternToRegExp(fullpath, paramNames); - for (final String paramName in paramNames) { - if (!params.containsKey(paramName)) { - throw Exception('missing param "$paramName" for $fullpath'); + assert(() { + // check that we have all the params we need + final List paramNames = []; + patternToRegExp(fullpath, paramNames); + for (final String paramName in paramNames) { + assert(params.containsKey(paramName), + 'missing param "$paramName" for $fullpath'); } - } - // check that we have don't have extra params - for (final String key in params.keys) { - if (!paramNames.contains(key)) { - throw Exception('unknown param "$key" for $fullpath'); + // check that we have don't have extra params + for (final String key in params.keys) { + assert(paramNames.contains(key), 'unknown param "$key" for $fullpath'); } - } + return true; + }()); final Map encodedParams = { for (final MapEntry param in params.entries) diff --git a/packages/go_router/lib/src/go_router_delegate.dart b/packages/go_router/lib/src/go_router_delegate.dart index 791fb77b8ef7..a0cdb59dc1b3 100644 --- a/packages/go_router/lib/src/go_router_delegate.dart +++ b/packages/go_router/lib/src/go_router_delegate.dart @@ -38,14 +38,14 @@ class GoRouterDelegate extends RouterDelegate required this.debugLogDiagnostics, required this.routerNeglect, this.restorationScopeId, - }) { - // check top-level route paths are valid - for (final GoRoute route in routes) { - if (!route.path.startsWith('/')) { - throw Exception('top-level path must start with "/": ${route.path}'); - } - } - + }) : assert(() { + // check top-level route paths are valid + for (final GoRoute route in routes) { + assert(route.path.startsWith('/'), + 'top-level path must start with "/": ${route.path}'); + } + return true; + }()) { // cache the set of named routes for fast lookup _cacheNamedRoutes(routes, '', _namedMatches); @@ -110,10 +110,8 @@ class GoRouterDelegate extends RouterDelegate if (route.name != null) { final String name = route.name!.toLowerCase(); - if (namedFullpaths.containsKey(name)) { - throw Exception('duplication fullpaths for name "$name":' - '${namedFullpaths[name]!.fullpath}, $fullpath'); - } + assert(!namedFullpaths.containsKey(name), + 'duplication fullpaths for name "$name":${namedFullpaths[name]!.fullpath}, $fullpath'); // we only have a partial match until we have a location; // we're really only caching the route and fullpath at this point @@ -154,12 +152,9 @@ class GoRouterDelegate extends RouterDelegate params: params, queryParams: queryParams, ); - if (match == null) { - throw Exception('unknown route name: $name'); - } - - assert(identical(match.queryParams, queryParams)); - return _addQueryParams(match.subloc, queryParams); + assert(match != null, 'unknown route name: $name'); + assert(identical(match!.queryParams, queryParams)); + return _addQueryParams(match!.subloc, queryParams); } /// Navigate to the given location. @@ -179,12 +174,8 @@ class GoRouterDelegate extends RouterDelegate /// Pop the top page off the GoRouter's page stack. void pop() { _matches.remove(_matches.last); - if (_matches.isEmpty) { - throw Exception( - 'have popped the last page off of the stack; ' - 'there are no pages left to show', - ); - } + assert(_matches.isNotEmpty, + 'have popped the last page off of the stack; there are no pages left to show'); notifyListeners(); } @@ -301,29 +292,28 @@ class GoRouterDelegate extends RouterDelegate return false; } - if (Uri.tryParse(redir) == null) { - throw Exception('invalid redirect: $redir'); - } - - if (redirects.contains(redir)) { - redirects.add(redir); - final String msg = - 'redirect loop detected: ${redirects.join(' => ')}'; - throw Exception(msg); - } + assert(Uri.tryParse(redir) != null, 'invalid redirect: $redir'); + + assert( + !redirects.contains(redir), + 'redirect loop detected: ${[ + ...redirects, + redir + ].join(' => ')}'); + assert( + redirects.length < redirectLimit, + 'too many redirects: ${[ + ...redirects, + redir + ].join(' => ')}'); redirects.add(redir); - if (redirects.length - 1 > redirectLimit) { - final String msg = 'too many redirects: ${redirects.join(' => ')}'; - throw Exception(msg); - } - log.info('redirecting to $redir'); return true; } // keep looping till we're done redirecting - for (;;) { + while (true) { final String loc = redirects.last; // check for top-level redirect @@ -449,23 +439,20 @@ class GoRouterDelegate extends RouterDelegate extra: extra, ).toList(); - if (matchStacks.isEmpty) { - throw Exception('no routes for location: $location'); - } + assert(matchStacks.isNotEmpty, 'no routes for location: $location'); + assert(() { + if (matchStacks.length > 1) { + final StringBuffer sb = StringBuffer() + ..writeln('too many routes for location: $location'); - if (matchStacks.length > 1) { - final StringBuffer sb = StringBuffer() - ..writeln('too many routes for location: $location'); + for (final List stack in matchStacks) { + sb.writeln( + '\t${stack.map((GoRouteMatch m) => m.route.path).join(' => ')}'); + } - for (final List stack in matchStacks) { - sb.writeln( - '\t${stack.map((GoRouteMatch m) => m.route.path).join(' => ')}'); + assert(false, sb.toString()); } - throw Exception(sb.toString()); - } - - if (kDebugMode) { assert(matchStacks.length == 1); final GoRouteMatch match = matchStacks.first.last; final String loc1 = _addQueryParams(match.subloc, match.queryParams); @@ -475,7 +462,8 @@ class GoRouterDelegate extends RouterDelegate // NOTE: match the lower case, since subloc is canonicalized to match the // path case whereas the location can be any case assert(loc1.toLowerCase() == loc2.toLowerCase(), '$loc1 != $loc2'); - } + return true; + }()); return matchStacks.first; } diff --git a/packages/go_router/pubspec.yaml b/packages/go_router/pubspec.yaml index 038fa1078b39..48a8987ad163 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: 3.0.6 +version: 3.0.7 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_route_test.dart b/packages/go_router/test/go_route_test.dart index 63b2912c08b2..c20d224fee36 100644 --- a/packages/go_router/test/go_route_test.dart +++ b/packages/go_router/test/go_route_test.dart @@ -7,6 +7,10 @@ import 'package:go_router/go_router.dart'; void main() { test('throws when a builder is not set', () { - expect(() => GoRoute(path: '/'), throwsException); + expect(() => GoRoute(path: '/'), throwsA(isAssertionError)); + }); + + test('throws when a path is empty', () { + expect(() => GoRoute(path: ''), throwsA(isAssertionError)); }); } diff --git a/packages/go_router/test/go_router_delegate_test.dart b/packages/go_router/test/go_router_delegate_test.dart index d5778569dd66..a7153327bf4b 100644 --- a/packages/go_router/test/go_router_delegate_test.dart +++ b/packages/go_router/test/go_router_delegate_test.dart @@ -45,7 +45,7 @@ void main() { () => delegate ..pop() ..pop(), - throwsException, + throwsA(isAssertionError), ); }); }); diff --git a/packages/go_router/test/go_router_test.dart b/packages/go_router/test/go_router_test.dart index 718aa6e4b988..bc0804efd06f 100644 --- a/packages/go_router/test/go_router_test.dart +++ b/packages/go_router/test/go_router_test.dart @@ -56,7 +56,7 @@ void main() { test('empty path', () { expect(() { GoRoute(path: ''); - }, throwsException); + }, throwsA(isAssertionError)); }); test('leading / on sub-route', () { @@ -71,7 +71,7 @@ void main() { ), ], ); - }, throwsException); + }, throwsA(isAssertionError)); }); test('trailing / on sub-route', () { @@ -86,7 +86,7 @@ void main() { ), ], ); - }, throwsException); + }, throwsA(isAssertionError)); }); test('lack of leading / on top-level route', () { @@ -95,7 +95,7 @@ void main() { GoRoute(path: 'foo', builder: _dummy), ]; _router(routes); - }, throwsException); + }, throwsA(isAssertionError)); }); test('match no routes', () { @@ -460,13 +460,13 @@ void main() { expect(() { _router(routes); - }, throwsException); + }, throwsA(isAssertionError)); }); test('empty name', () { expect(() { GoRoute(name: '', path: '/'); - }, throwsException); + }, throwsA(isAssertionError)); }); test('match no routes', () { @@ -476,7 +476,7 @@ void main() { ]; final GoRouter router = _router(routes); router.goNamed('work'); - }, throwsException); + }, throwsA(isAssertionError)); }); test('match 2nd top level route', () { @@ -612,7 +612,7 @@ void main() { expect(() { final GoRouter router = _router(routes); router.goNamed('person', params: {'fid': 'f2'}); - }, throwsException); + }, throwsA(isAssertionError)); }); test('match case insensitive w/ params', () { @@ -661,7 +661,7 @@ void main() { expect(() { final GoRouter router = _router(routes); router.goNamed('family'); - }, throwsException); + }, throwsA(isAssertionError)); }); test('too many params', () { @@ -677,7 +677,7 @@ void main() { final GoRouter router = _router(routes); router.goNamed('family', params: {'fid': 'f2', 'pid': 'p1'}); - }, throwsException); + }, throwsA(isAssertionError)); }); test('sparsely named routes', () {