From fc3709d70f17564b80514495333473813bb51ad2 Mon Sep 17 00:00:00 2001 From: ValentinVignal Date: Wed, 31 Jan 2024 17:05:25 +0800 Subject: [PATCH 1/5] bug: Don't log if hierarchicalLoggingEnabled is true --- packages/go_router/lib/src/logging.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/go_router/lib/src/logging.dart b/packages/go_router/lib/src/logging.dart index 7f0a8ce5a7a..f5770fa1745 100644 --- a/packages/go_router/lib/src/logging.dart +++ b/packages/go_router/lib/src/logging.dart @@ -28,7 +28,7 @@ StreamSubscription? _subscription; void setLogging({bool enabled = false}) { _subscription?.cancel(); _enabled = enabled; - if (!enabled) { + if (!enabled || hierarchicalLoggingEnabled) { return; } From c3a079449b6879f265c635f78f80107de24c6f18 Mon Sep 17 00:00:00 2001 From: ValentinVignal Date: Wed, 31 Jan 2024 17:08:50 +0800 Subject: [PATCH 2/5] chore: Update the version --- 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 36d5e840187..b7aea253c13 100644 --- a/packages/go_router/CHANGELOG.md +++ b/packages/go_router/CHANGELOG.md @@ -1,3 +1,7 @@ +## 13.1.1 + +- Fixes unwanted logs when `hierarchicalLoggingEnabled` was set to `true`. + ## 13.1.0 - Adds `topRoute` to `GoRouterState` diff --git a/packages/go_router/pubspec.yaml b/packages/go_router/pubspec.yaml index 770107232ee..103733d8f98 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: 13.1.0 +version: 13.1.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 From 5e65ecb5cb45baa972616800252ebf2ab37b1bfd Mon Sep 17 00:00:00 2001 From: ValentinVignal Date: Tue, 9 Apr 2024 09:41:11 +0800 Subject: [PATCH 3/5] fix: Remove bad batch --- packages/go_router/lib/src/delegate.dart | 2 -- packages/go_router/lib/src/route_data.dart | 9 --------- 2 files changed, 11 deletions(-) diff --git a/packages/go_router/lib/src/delegate.dart b/packages/go_router/lib/src/delegate.dart index 6d96944f659..3b01b9e381c 100644 --- a/packages/go_router/lib/src/delegate.dart +++ b/packages/go_router/lib/src/delegate.dart @@ -72,8 +72,6 @@ class GoRouterDelegate extends RouterDelegate // This should be the only place where the last GoRoute exit the screen. final GoRoute lastRoute = currentConfiguration.last.route; if (lastRoute.onExit != null && navigatorKey.currentContext != null) { - walker.buildState(_configuration, _configuration.); - return !(await lastRoute.onExit!(navigatorKey.currentContext!)); } return false; diff --git a/packages/go_router/lib/src/route_data.dart b/packages/go_router/lib/src/route_data.dart index 6ca5f1070f4..26a6fc42a96 100644 --- a/packages/go_router/lib/src/route_data.dart +++ b/packages/go_router/lib/src/route_data.dart @@ -63,11 +63,6 @@ abstract class GoRouteData extends RouteData { /// Corresponds to [GoRoute.redirect]. FutureOr redirect(BuildContext context, GoRouterState state) => null; - /// Called when this route is removed from GoRouter's route history. - /// - /// Corresponds to [GoRoute.onExit]. - FutureOr onExit(BuildContext context) => true; - /// A helper function used by generated code. /// /// Should not be used directly. @@ -111,16 +106,12 @@ abstract class GoRouteData extends RouteData { FutureOr redirect(BuildContext context, GoRouterState state) => factoryImpl(state).redirect(context, state); - FutureOr onExit(BuildContext context) => - factoryImpl(state).onExit(context); - return GoRoute( path: path, name: name, builder: builder, pageBuilder: pageBuilder, redirect: redirect, - onExit: onExit, routes: routes, parentNavigatorKey: parentNavigatorKey, ); From 55e67307c20109863a4d2275421b1c96cff83805 Mon Sep 17 00:00:00 2001 From: ValentinVignal Date: Sat, 13 Apr 2024 23:10:42 +0800 Subject: [PATCH 4/5] refactor: Add mockable developerLog method --- packages/go_router/lib/src/logging.dart | 32 +++++++++++++++++-------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/packages/go_router/lib/src/logging.dart b/packages/go_router/lib/src/logging.dart index f5770fa1745..4f116422181 100644 --- a/packages/go_router/lib/src/logging.dart +++ b/packages/go_router/lib/src/logging.dart @@ -47,16 +47,28 @@ void setLogging({bool enabled = false}) { ), ); } else { - developer.log( - e.message, - time: e.time, - sequenceNumber: e.sequenceNumber, - level: e.level.value, - name: e.loggerName, - zone: e.zone, - error: e.error, - stackTrace: e.stackTrace, - ); + _developerLogFunction(e); } }); } + +void _developerLog(LogRecord record) { + developer.log( + record.message, + time: record.time, + sequenceNumber: record.sequenceNumber, + level: record.level.value, + name: record.loggerName, + zone: record.zone, + error: record.error, + stackTrace: record.stackTrace, + ); +} + +/// A function that can be set during test to mock the developer log function. +@visibleForTesting +void Function(LogRecord)? testDeveloperLog; + +/// The function used to log messages. +void Function(LogRecord) get _developerLogFunction => + testDeveloperLog ?? _developerLog; From 213841feb44823166d3932f77c015074f0a4c487 Mon Sep 17 00:00:00 2001 From: ValentinVignal Date: Sat, 13 Apr 2024 23:10:49 +0800 Subject: [PATCH 5/5] test: Update the tests --- packages/go_router/test/logging_test.dart | 51 ++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/packages/go_router/test/logging_test.dart b/packages/go_router/test/logging_test.dart index 3ae24561751..1303f2909db 100644 --- a/packages/go_router/test/logging_test.dart +++ b/packages/go_router/test/logging_test.dart @@ -11,6 +11,13 @@ import 'package:go_router/src/logging.dart'; import 'package:logging/logging.dart'; void main() { + tearDown(() { + // Reset the logging state + hierarchicalLoggingEnabled = false; + + // Reset the developer log function. + testDeveloperLog = null; + }); test('setLogging does not clear listeners', () { final StreamSubscription subscription = logger.onRecord.listen( expectAsync1((LogRecord r) {}, count: 2), @@ -26,6 +33,7 @@ void main() { testWidgets( 'It should not log anything the if debugLogDiagnostics is false', (WidgetTester tester) async { + testDeveloperLog = expectAsync1((LogRecord data) {}, count: 0); final StreamSubscription subscription = Logger.root.onRecord.listen( expectAsync1((LogRecord data) {}, count: 0), @@ -43,8 +51,48 @@ void main() { ); testWidgets( - 'It should not log the known routes and the initial route if debugLogDiagnostics is true', + 'It should log the known routes and the initial route if debugLogDiagnostics is true', (WidgetTester tester) async { + testDeveloperLog = expectAsync1( + (LogRecord data) {}, + count: 2, + reason: 'Go router should log the 2 events', + ); + 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' + ], + reason: 'Go router should have sent the 2 events to the logger', + ); + }, + ); + + testWidgets( + 'Go router should not log itself the known routes but send the events to the logger when hierarchicalLoggingEnabled is true', + (WidgetTester tester) async { + testDeveloperLog = expectAsync1( + (LogRecord data) {}, + count: 0, + reason: 'Go router should log the events itself', + ); + hierarchicalLoggingEnabled = true; + final List logs = []; Logger.root.onRecord.listen( (LogRecord event) => logs.add(event.message), @@ -65,6 +113,7 @@ void main() { 'Full paths for routes:\n => /\n', 'setting initial location null' ], + reason: 'Go router should have sent the 2 events to the logger', ); }, );