Skip to content

[go_router] Avoid logging when debugLogDiagnostics is false #4875

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
auto-submit merged 7 commits into from
Sep 28, 2023
Merged

[go_router] Avoid logging when debugLogDiagnostics is false #4875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7c4d47f
fix: Do not log when it is disabled
ValentinVignal Sep 9, 2023
41192e9
test: Add some tests for logging
ValentinVignal Sep 9, 2023
f16e21e
doc: Update version and changelog
ValentinVignal Sep 9, 2023
cc423ea
doc: Add more comment
ValentinVignal Sep 9, 2023
afe31d1
Merge remote-tracking branch 'upstream/main' into go-router/do-not-lo…
ValentinVignal Sep 22, 2023
3f46ae8
Merge remote-tracking branch 'upstream/main' into go-router/do-not-lo…
ValentinVignal Sep 26, 2023
0f87377
Merge branch 'main' into go-router/do-not-log-when-it-is-disabled
chunhtai Sep 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/go_router/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 11.1.2

- Fixes a bug where the known routes and initial route were logged even when `debugLogDiagnostics` was set to `false`.

## 11.1.1

- Fixes a missing `{@end-tool}` doc directive tag for `GoRoute.name`.
Expand Down
6 changes: 3 additions & 3 deletions packages/go_router/lib/src/builder.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
8 changes: 4 additions & 4 deletions packages/go_router/lib/src/configuration.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class RouteConfiguration {
routes, <GlobalKey<NavigatorState>>[navigatorKey])) {
assert(_debugCheckStatefulShellBranchDefaultLocations(routes));
_cacheNameToPath('', routes);
log.info(debugKnownRoutes());
log(debugKnownRoutes());
}

static bool _debugCheckPath(List<RouteBase> routes, bool isTopLevel) {
Expand Down Expand Up @@ -234,7 +234,7 @@ class RouteConfiguration {
Map<String, dynamic> queryParameters = const <String, dynamic>{},
}) {
assert(() {
log.info('getting location for name: '
log('getting location for name: '
'"$name"'
'${pathParameters.isEmpty ? '' : ', pathParameters: $pathParameters'}'
'${queryParameters.isEmpty ? '' : ', queryParameters: $queryParameters'}');
Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -522,7 +522,7 @@ class RouteConfiguration {

redirects.add(newMatch);

log.info('redirecting to $newMatch');
log('redirecting to $newMatch');
}

String _formatRedirectionHistory(List<RouteMatchList> redirections) {
Expand Down
17 changes: 15 additions & 2 deletions packages/go_router/lib/src/logging.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,35 @@

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');

/// Whether or not the logging is enabled.
bool _enabled = false;

/// Logs the message if logging is enabled.
void log(String message) {
Copy link
Contributor

@chunhtai chunhtai Sep 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use log may be ambiguous if we later add other level for some reason.

should this just set logger.level = Level.OFF when set enabled = false?

Copy link
Contributor Author

@ValentinVignal ValentinVignal Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use log may be ambiguous if we later add other level for some reason.

We could later on change the signature to

void log(String message, [Level level = Level.INFO]);

should this just set logger.level = Level.OFF when set enabled = false?

This works too.
Unfortunately, doing so would force the user to set hierarchicalLoggingEnabled to true. If not, logging throws:

══╡ EXCEPTION CAUGHT BY WIDGETS LIBRARY ╞═══════════════════════════════════════════════════════════
The following UnsupportedError was thrown building MyApp(dirty):
Unsupported operation: Please set "hierarchicalLoggingEnabled" to true if you want to change the
level on a non-root logger.

I'm not sure that's acceptable to force the user to do that?

Copy link
Contributor

@chunhtai chunhtai Sep 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, yeah this is fine

if (_enabled) {
logger.info(message);
}
}

StreamSubscription<LogRecord>? _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
Expand Down
2 changes: 1 addition & 1 deletion packages/go_router/lib/src/parser.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ class GoRouteInformationParser extends RouteInformationParser<RouteMatchList> {
// 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(
Expand Down
16 changes: 8 additions & 8 deletions packages/go_router/lib/src/router.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ class GoRouter implements RouterConfig<RouteMatchList> {
);

assert(() {
log.info('setting initial location $initialLocation');
log('setting initial location $initialLocation');
return true;
}());
}
Expand Down Expand Up @@ -338,13 +338,13 @@ class GoRouter implements RouterConfig<RouteMatchList> {
/// 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(),
matchList: matchList,
Expand Down Expand Up @@ -376,7 +376,7 @@ class GoRouter implements RouterConfig<RouteMatchList> {
/// it as the same page. The page key will be reused. This will preserve the
/// state and not run any page animation.
Future<T?> push<T extends Object?>(String location, {Object? extra}) async {
log.info('pushing $location');
log('pushing $location');
return routeInformationProvider.push<T>(
location,
base: routerDelegate.currentConfiguration,
Expand Down Expand Up @@ -409,7 +409,7 @@ class GoRouter implements RouterConfig<RouteMatchList> {
/// state and not run any page animation.
Future<T?> pushReplacement<T extends Object?>(String location,
{Object? extra}) {
log.info('pushReplacement $location');
log('pushReplacement $location');
return routeInformationProvider.pushReplacement<T>(
location,
base: routerDelegate.currentConfiguration,
Expand Down Expand Up @@ -448,7 +448,7 @@ class GoRouter implements RouterConfig<RouteMatchList> {
/// * [pushReplacement] which replaces the top-most page of the page stack but
/// always uses a new page key.
Future<T?> replace<T>(String location, {Object? extra}) {
log.info('replace $location');
log('replace $location');
return routeInformationProvider.replace<T>(
location,
base: routerDelegate.currentConfiguration,
Expand Down Expand Up @@ -486,7 +486,7 @@ class GoRouter implements RouterConfig<RouteMatchList> {
/// of any GoRoute under it.
void pop<T extends Object?>([T? result]) {
assert(() {
log.info('popping ${routerDelegate.currentConfiguration.uri}');
log('popping ${routerDelegate.currentConfiguration.uri}');
return true;
}());
routerDelegate.pop<T>(result);
Expand All @@ -495,7 +495,7 @@ class GoRouter implements RouterConfig<RouteMatchList> {
/// Refresh the route.
void refresh() {
assert(() {
log.info('refreshing ${routerDelegate.currentConfiguration.uri}');
log('refreshing ${routerDelegate.currentConfiguration.uri}');
return true;
}());
routeInformationProvider.notifyListeners();
Expand Down
2 changes: 1 addition & 1 deletion packages/go_router/pubspec.yaml
View file
Open in desktop
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: 11.1.1
version: 11.1.2
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
61 changes: 57 additions & 4 deletions packages/go_router/test/logging_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<void, LogRecord>((LogRecord r) {}, count: 2));
final StreamSubscription<LogRecord> subscription = logger.onRecord.listen(
expectAsync1<void, LogRecord>((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<LogRecord> subscription =
Logger.root.onRecord.listen(
expectAsync1((LogRecord data) {}, count: 0),
);
addTearDown(subscription.cancel);
GoRouter(
routes: <RouteBase>[
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<String> logs = <String>[];
Logger.root.onRecord.listen(
(LogRecord event) => logs.add(event.message),
);
GoRouter(
debugLogDiagnostics: true,
routes: <RouteBase>[
GoRoute(
path: '/',
builder: (_, GoRouterState state) => const Text('home'),
),
],
);

expect(
logs,
const <String>[
'Full paths for routes:\n => /\n',
'setting initial location null'
],
);
},
);
}