From 7c4d47f21e87695dc749e67cedc4e244326aef06 Mon Sep 17 00:00:00 2001 From: ValentinVignal Date: Sat, 9 Sep 2023 18:28:57 +0800 Subject: [PATCH 1/4] fix: Do not log when it is disabled --- packages/go_router/lib/src/builder.dart | 6 +++--- packages/go_router/lib/src/configuration.dart | 8 ++++---- packages/go_router/lib/src/logging.dart | 16 ++++++++++++++-- packages/go_router/lib/src/parser.dart | 2 +- packages/go_router/lib/src/router.dart | 16 ++++++++-------- 5 files changed, 30 insertions(+), 18 deletions(-) diff --git a/packages/go_router/lib/src/builder.dart b/packages/go_router/lib/src/builder.dart index 1e46417b30f1..445931f73efb 100644 --- a/packages/go_router/lib/src/builder.dart +++ b/packages/go_router/lib/src/builder.dart @@ -445,17 +445,17 @@ class RouteBuilder { final Element? elem = context is Element ? context : null; if (elem != null && isMaterialApp(elem)) { - log.info('Using MaterialApp configuration'); + log('Using MaterialApp configuration'); _pageBuilderForAppType = pageBuilderForMaterialApp; _errorBuilderForAppType = (BuildContext c, GoRouterState s) => MaterialErrorScreen(s.error); } else if (elem != null && isCupertinoApp(elem)) { - log.info('Using CupertinoApp configuration'); + log('Using CupertinoApp configuration'); _pageBuilderForAppType = pageBuilderForCupertinoApp; _errorBuilderForAppType = (BuildContext c, GoRouterState s) => CupertinoErrorScreen(s.error); } else { - log.info('Using WidgetsApp configuration'); + log('Using WidgetsApp configuration'); _pageBuilderForAppType = pageBuilderForWidgetApp; _errorBuilderForAppType = (BuildContext c, GoRouterState s) => ErrorScreen(s.error); diff --git a/packages/go_router/lib/src/configuration.dart b/packages/go_router/lib/src/configuration.dart index 618e583b7dfb..4c9c018ed800 100644 --- a/packages/go_router/lib/src/configuration.dart +++ b/packages/go_router/lib/src/configuration.dart @@ -32,7 +32,7 @@ class RouteConfiguration { routes, >[navigatorKey])) { assert(_debugCheckStatefulShellBranchDefaultLocations(routes)); _cacheNameToPath('', routes); - log.info(debugKnownRoutes()); + log(debugKnownRoutes()); } static bool _debugCheckPath(List routes, bool isTopLevel) { @@ -234,7 +234,7 @@ class RouteConfiguration { Map queryParameters = const {}, }) { assert(() { - log.info('getting location for name: ' + log('getting location for name: ' '"$name"' '${pathParameters.isEmpty ? '' : ', pathParameters: $pathParameters'}' '${queryParameters.isEmpty ? '' : ', queryParameters: $queryParameters'}'); @@ -492,7 +492,7 @@ class RouteConfiguration { _addRedirect(redirectHistory, newMatch, previousLocation); return newMatch; } on GoException catch (e) { - log.info('Redirection exception: ${e.message}'); + log('Redirection exception: ${e.message}'); return _errorRouteMatchList(previousLocation, e); } } @@ -522,7 +522,7 @@ class RouteConfiguration { redirects.add(newMatch); - log.info('redirecting to $newMatch'); + log('redirecting to $newMatch'); } String _formatRedirectionHistory(List redirections) { diff --git a/packages/go_router/lib/src/logging.dart b/packages/go_router/lib/src/logging.dart index ccbb09b7f87d..e2d6333d5d23 100644 --- a/packages/go_router/lib/src/logging.dart +++ b/packages/go_router/lib/src/logging.dart @@ -4,22 +4,34 @@ import 'dart:async'; import 'dart:developer' as developer; + import 'package:flutter/foundation.dart'; import 'package:logging/logging.dart'; /// The logger for this package. -final Logger log = Logger('GoRouter'); +@visibleForTesting +final Logger logger = Logger('GoRouter'); + +bool _enabled = false; + +/// Logs the message if logging is enabled. +void log(String message) { + if (_enabled) { + logger.info(message); + } +} StreamSubscription? _subscription; /// Forwards diagnostic messages to the dart:developer log() API. void setLogging({bool enabled = false}) { _subscription?.cancel(); + _enabled = enabled; if (!enabled) { return; } - _subscription = log.onRecord.listen((LogRecord e) { + _subscription = logger.onRecord.listen((LogRecord e) { // use `dumpErrorToConsole` for severe messages to ensure that severe // exceptions are formatted consistently with other Flutter examples and // avoids printing duplicate exceptions diff --git a/packages/go_router/lib/src/parser.dart b/packages/go_router/lib/src/parser.dart index df4d551a8bf5..a13cc0358898 100644 --- a/packages/go_router/lib/src/parser.dart +++ b/packages/go_router/lib/src/parser.dart @@ -89,7 +89,7 @@ class GoRouteInformationParser extends RouteInformationParser { // TODO(chunhtai): remove this ignore and migrate the code // https://github.com/flutter/flutter/issues/124045. // ignore: deprecated_member_use - log.info('No initial matches: ${routeInformation.location}'); + log('No initial matches: ${routeInformation.location}'); } return debugParserFuture = _redirect( diff --git a/packages/go_router/lib/src/router.dart b/packages/go_router/lib/src/router.dart index a5671e2fc9d0..da29c6cc0d7d 100644 --- a/packages/go_router/lib/src/router.dart +++ b/packages/go_router/lib/src/router.dart @@ -158,7 +158,7 @@ class GoRouter implements RouterConfig { ); assert(() { - log.info('setting initial location $initialLocation'); + log('setting initial location $initialLocation'); return true; }()); } @@ -318,13 +318,13 @@ class GoRouter implements RouterConfig { /// Navigate to a URI location w/ optional query parameters, e.g. /// `/family/f2/person/p1?color=blue` void go(String location, {Object? extra}) { - log.info('going to $location'); + log('going to $location'); routeInformationProvider.go(location, extra: extra); } /// Restore the RouteMatchList void restore(RouteMatchList matchList) { - log.info('restoring ${matchList.uri}'); + log('restoring ${matchList.uri}'); routeInformationProvider.restore( matchList.uri.toString(), encodedMatchList: RouteMatchListCodec(configuration).encode(matchList), @@ -356,7 +356,7 @@ class GoRouter implements RouterConfig { /// it as the same page. The page key will be reused. This will preserve the /// state and not run any page animation. Future push(String location, {Object? extra}) async { - log.info('pushing $location'); + log('pushing $location'); return routeInformationProvider.push( location, base: routerDelegate.currentConfiguration, @@ -389,7 +389,7 @@ class GoRouter implements RouterConfig { /// state and not run any page animation. Future pushReplacement(String location, {Object? extra}) { - log.info('pushReplacement $location'); + log('pushReplacement $location'); return routeInformationProvider.pushReplacement( location, base: routerDelegate.currentConfiguration, @@ -428,7 +428,7 @@ class GoRouter implements RouterConfig { /// * [pushReplacement] which replaces the top-most page of the page stack but /// always uses a new page key. Future replace(String location, {Object? extra}) { - log.info('replace $location'); + log('replace $location'); return routeInformationProvider.replace( location, base: routerDelegate.currentConfiguration, @@ -466,7 +466,7 @@ class GoRouter implements RouterConfig { /// of any GoRoute under it. void pop([T? result]) { assert(() { - log.info('popping ${routerDelegate.currentConfiguration.uri}'); + log('popping ${routerDelegate.currentConfiguration.uri}'); return true; }()); routerDelegate.pop(result); @@ -475,7 +475,7 @@ class GoRouter implements RouterConfig { /// Refresh the route. void refresh() { assert(() { - log.info('refreshing ${routerDelegate.currentConfiguration.uri}'); + log('refreshing ${routerDelegate.currentConfiguration.uri}'); return true; }()); routeInformationProvider.notifyListeners(); From 41192e913abc927488cfc2977ad35bd5cf072b3a Mon Sep 17 00:00:00 2001 From: ValentinVignal Date: Sat, 9 Sep 2023 18:29:07 +0800 Subject: [PATCH 2/4] test: Add some tests for logging --- packages/go_router/test/logging_test.dart | 61 +++++++++++++++++++++-- 1 file changed, 57 insertions(+), 4 deletions(-) diff --git a/packages/go_router/test/logging_test.dart b/packages/go_router/test/logging_test.dart index d6e278610fba..3ae245617519 100644 --- a/packages/go_router/test/logging_test.dart +++ b/packages/go_router/test/logging_test.dart @@ -2,17 +2,70 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'dart:async'; + +import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:go_router/go_router.dart'; import 'package:go_router/src/logging.dart'; import 'package:logging/logging.dart'; void main() { test('setLogging does not clear listeners', () { - log.onRecord - .listen(expectAsync1((LogRecord r) {}, count: 2)); + final StreamSubscription subscription = logger.onRecord.listen( + expectAsync1((LogRecord r) {}, count: 2), + ); + addTearDown(subscription.cancel); + setLogging(enabled: true); - log.info('message'); + logger.info('message'); setLogging(); - log.info('message'); + logger.info('message'); }); + + testWidgets( + 'It should not log anything the if debugLogDiagnostics is false', + (WidgetTester tester) async { + final StreamSubscription subscription = + Logger.root.onRecord.listen( + expectAsync1((LogRecord data) {}, count: 0), + ); + addTearDown(subscription.cancel); + GoRouter( + routes: [ + GoRoute( + path: '/', + builder: (_, GoRouterState state) => const Text('home'), + ), + ], + ); + }, + ); + + testWidgets( + 'It should not log the known routes and the initial route if debugLogDiagnostics is true', + (WidgetTester tester) async { + final List logs = []; + Logger.root.onRecord.listen( + (LogRecord event) => logs.add(event.message), + ); + GoRouter( + debugLogDiagnostics: true, + routes: [ + GoRoute( + path: '/', + builder: (_, GoRouterState state) => const Text('home'), + ), + ], + ); + + expect( + logs, + const [ + 'Full paths for routes:\n => /\n', + 'setting initial location null' + ], + ); + }, + ); } From f16e21e9cf4f77a0cf743954279fdd86c91d3101 Mon Sep 17 00:00:00 2001 From: ValentinVignal Date: Sat, 9 Sep 2023 18:30:38 +0800 Subject: [PATCH 3/4] doc: Update version and changelog --- packages/go_router/CHANGELOG.md | 4 ++++ packages/go_router/pubspec.yaml | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/go_router/CHANGELOG.md b/packages/go_router/CHANGELOG.md index ae73f40e0c28..5cee35a12134 100644 --- a/packages/go_router/CHANGELOG.md +++ b/packages/go_router/CHANGELOG.md @@ -1,3 +1,7 @@ +## 10.1.3 + +- Fixes a bug where the known routes and initial route were logged even when `debugLogDiagnostics` was set to `false`. + ## 10.1.2 * Adds pub topics to package metadata. diff --git a/packages/go_router/pubspec.yaml b/packages/go_router/pubspec.yaml index b70f0b23407b..0e4bb0f75f82 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: 10.1.2 +version: 10.1.3 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 From cc423eab17a70432e8485026185e8dd69ac5c656 Mon Sep 17 00:00:00 2001 From: ValentinVignal Date: Sat, 9 Sep 2023 18:32:38 +0800 Subject: [PATCH 4/4] doc: Add more comment --- packages/go_router/lib/src/logging.dart | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/go_router/lib/src/logging.dart b/packages/go_router/lib/src/logging.dart index e2d6333d5d23..3f39e4dc804d 100644 --- a/packages/go_router/lib/src/logging.dart +++ b/packages/go_router/lib/src/logging.dart @@ -12,6 +12,7 @@ import 'package:logging/logging.dart'; @visibleForTesting final Logger logger = Logger('GoRouter'); +/// Whether or not the logging is enabled. bool _enabled = false; /// Logs the message if logging is enabled.