[go_router] Reduces excessive rebuilds due to inherited look up. (#4227)

fixes https://github.com/flutter/flutter/issues/123570

packages/go_router/CHANGELOG.md

@@ -1,3 +1,12 @@
+## 9.0.0
+
+- **BREAKING CHANGE**:
+  - Removes GoRouter.location and GoRouter.canPop. Use GoRouterState.of().location and
+    Navigator.of().canPop instead.
+  - GoRouter does not `extends` ChangeNotifier.
+  - [Migration guide](https://flutter.dev/go/go-router-v9-breaking-changes)
+- Reduces excessive rebuilds due to inherited look up.
+
 ## 8.2.0
 
 - Adds onException to GoRouter constructor.

packages/go_router/README.md

@@ -37,6 +37,7 @@ See the API documentation for details on the following topics:
 - [Error handling](https://pub.dev/documentation/go_router/latest/topics/Error%20handling-topic.html)
 
 ## Migration guides
+- [Migrating to 9.0.0](https://flutter.dev/go/go-router-v9-breaking-changes).
 - [Migrating to 8.0.0](https://flutter.dev/go/go-router-v8-breaking-changes).
 - [Migrating to 7.0.0](https://flutter.dev/go/go-router-v7-breaking-changes).
 - [Migrating to 6.0.0](https://flutter.dev/go/go-router-v6-breaking-changes)

packages/go_router/lib/src/information_provider.dart

@@ -133,6 +133,14 @@ class GoRouteInformationProvider extends RouteInformationProvider
   RouteInformation get value => _value;
   RouteInformation _value;
 
+  @override
+  // TODO(chunhtai): remove this ignore once package minimum dart version is
+  // above 3.
+  // ignore: unnecessary_overrides
+  void notifyListeners() {
+    super.notifyListeners();
+  }
+
   void _setValue(String location, Object state) {
     final bool shouldNotify =
         _value.location != location || _value.state != state;

packages/go_router/lib/src/misc/inherited_router.dart

@@ -11,13 +11,13 @@ import '../router.dart';
 ///
 /// Used for to find the current GoRouter in the widget tree. This is useful
 /// when routing from anywhere in your app.
-class InheritedGoRouter extends InheritedNotifier<GoRouter> {
+class InheritedGoRouter extends InheritedWidget {
   /// Default constructor for the inherited go router.
   const InheritedGoRouter({
     required super.child,
     required this.goRouter,
     super.key,
-  }) : super(notifier: goRouter);
+  });
 
   /// The [GoRouter] that is made available to the widget tree.
   final GoRouter goRouter;
@@ -27,4 +27,7 @@ class InheritedGoRouter extends InheritedNotifier<GoRouter> {
     super.debugFillProperties(properties);
     properties.add(DiagnosticsProperty<GoRouter>('goRouter', goRouter));
   }
+
+  @override
+  bool updateShouldNotify(covariant InheritedWidget oldWidget) => false;
 }

packages/go_router/lib/src/router.dart

@@ -62,7 +62,7 @@ typedef GoExceptionHandler = void Function(
 /// {@category Deep linking}
 /// {@category Error handling}
 /// {@category Named routes}
-class GoRouter extends ChangeNotifier implements RouterConfig<RouteMatchList> {
+class GoRouter implements RouterConfig<RouteMatchList> {
   /// Default constructor to configure a GoRouter with a routes builder
   /// and an error page builder.
   ///
@@ -152,7 +152,6 @@ class GoRouter extends ChangeNotifier implements RouterConfig<RouteMatchList> {
       builderWithNav: (BuildContext context, Widget child) =>
           InheritedGoRouter(goRouter: this, child: child),
     );
-    routerDelegate.addListener(_handleStateMayChange);
 
     assert(() {
       log.info('setting initial location $initialLocation');
@@ -296,34 +295,9 @@ class GoRouter extends ChangeNotifier implements RouterConfig<RouteMatchList> {
   @override
   late final GoRouteInformationParser routeInformationParser;
 
-  /// Gets the current location.
-  // TODO(chunhtai): deprecates this once go_router_builder is migrated to
-  // GoRouterState.of.
-  String get location => _location;
-  String _location = '/';
-
   /// Returns `true` if there is at least two or more route can be pop.
   bool canPop() => routerDelegate.canPop();
 
-  void _handleStateMayChange() {
-    final String newLocation;
-    if (routerDelegate.currentConfiguration.isNotEmpty &&
-        routerDelegate.currentConfiguration.matches.last
-            is ImperativeRouteMatch) {
-      newLocation = (routerDelegate.currentConfiguration.matches.last
-              as ImperativeRouteMatch)
-          .matches
-          .uri
-          .toString();
-    } else {
-      newLocation = routerDelegate.currentConfiguration.uri.toString();
-    }
-    if (_location != newLocation) {
-      _location = newLocation;
-      notifyListeners();
-    }
-  }
-
   /// Get a location from route name and parameters.
   /// This is useful for redirecting to a named location.
   String namedLocation(
@@ -488,7 +462,7 @@ class GoRouter extends ChangeNotifier implements RouterConfig<RouteMatchList> {
   /// of any GoRoute under it.
   void pop<T extends Object?>([T? result]) {
     assert(() {
-      log.info('popping $location');
+      log.info('popping ${routerDelegate.currentConfiguration.uri}');
       return true;
     }());
     routerDelegate.pop<T>(result);
@@ -497,7 +471,7 @@ class GoRouter extends ChangeNotifier implements RouterConfig<RouteMatchList> {
   /// Refresh the route.
   void refresh() {
     assert(() {
-      log.info('refreshing $location');
+      log.info('refreshing ${routerDelegate.currentConfiguration.uri}');
       return true;
     }());
     routeInformationProvider.notifyListeners();
@@ -507,27 +481,25 @@ class GoRouter extends ChangeNotifier implements RouterConfig<RouteMatchList> {
   ///
   /// This method throws when it is called during redirects.
   static GoRouter of(BuildContext context) {
-    final InheritedGoRouter? inherited =
-        context.dependOnInheritedWidgetOfExactType<InheritedGoRouter>();
+    final GoRouter? inherited = maybeOf(context);
     assert(inherited != null, 'No GoRouter found in context');
-    return inherited!.goRouter;
+    return inherited!;
   }
 
   /// The current GoRouter in the widget tree, if any.
   ///
   /// This method returns null when it is called during redirects.
   static GoRouter? maybeOf(BuildContext context) {
-    final InheritedGoRouter? inherited =
-        context.dependOnInheritedWidgetOfExactType<InheritedGoRouter>();
+    final InheritedGoRouter? inherited = context
+        .getElementForInheritedWidgetOfExactType<InheritedGoRouter>()
+        ?.widget as InheritedGoRouter?;
     return inherited?.goRouter;
   }
 
-  @override
+  /// Disposes resource created by this object.
   void dispose() {
     routeInformationProvider.dispose();
-    routerDelegate.removeListener(_handleStateMayChange);
     routerDelegate.dispose();
-    super.dispose();
   }
 
   String _effectiveInitialLocation(String? initialLocation) {

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: 8.2.0
+version: 9.0.0
 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
 

packages/go_router/test/extension_test.dart

@@ -5,6 +5,7 @@
 import 'package:flutter/material.dart';
 import 'package:flutter_test/flutter_test.dart';
 import 'package:go_router/go_router.dart';
+import 'package:go_router/src/match.dart';
 
 void main() {
   group('replaceNamed', () {
@@ -37,7 +38,10 @@ void main() {
       final GoRouter router = await createGoRouter(tester);
       await tester.tap(find.text('Settings'));
       await tester.pumpAndSettle();
-      expect(router.location, '/page-0/settings?search=notification');
+      final ImperativeRouteMatch routeMatch = router
+          .routerDelegate.currentConfiguration.last as ImperativeRouteMatch;
+      expect(routeMatch.matches.uri.toString(),
+          '/page-0/settings?search=notification');
     });
   });
 }

packages/go_router/test/go_router_test.dart

@@ -258,42 +258,6 @@ void main() {
       expect(find.byType(DummyScreen), findsOneWidget);
     });
 
-    testWidgets('can access GoRouter parameters from builder',
-        (WidgetTester tester) async {
-      final List<GoRoute> routes = <GoRoute>[
-        GoRoute(path: '/', redirect: (_, __) => '/1'),
-        GoRoute(
-            path: '/:id',
-            builder: (BuildContext context, GoRouterState state) {
-              return Text(GoRouter.of(context).location);
-            }),
-      ];
-
-      final GoRouter router = await createRouter(routes, tester);
-      expect(find.text('/1'), findsOneWidget);
-      router.go('/123?id=456');
-      await tester.pumpAndSettle();
-      expect(find.text('/123?id=456'), findsOneWidget);
-    });
-
-    testWidgets('can access GoRouter parameters from error builder',
-        (WidgetTester tester) async {
-      final List<GoRoute> routes = <GoRoute>[
-        GoRoute(path: '/', builder: dummy),
-      ];
-
-      final GoRouter router = await createRouter(routes, tester,
-          errorBuilder: (BuildContext context, GoRouterState state) {
-        return Text(GoRouter.of(context).location);
-      });
-      router.go('/123?id=456');
-      await tester.pumpAndSettle();
-      expect(find.text('/123?id=456'), findsOneWidget);
-      router.go('/1234?id=456');
-      await tester.pumpAndSettle();
-      expect(find.text('/1234?id=456'), findsOneWidget);
-    });
-
     testWidgets('repeatedly pops imperative route does not crash',
         (WidgetTester tester) async {
       // Regression test for https://github.com/flutter/flutter/issues/123369.
@@ -616,7 +580,11 @@ void main() {
       // NOTE: match the lower case, since location is canonicalized to match the
       // path case whereas the location can be any case; so long as the path
       // produces a match regardless of the location case, we win!
-      expect(router.location.toLowerCase(), loc.toLowerCase());
+      expect(
+          router.routerDelegate.currentConfiguration.uri
+              .toString()
+              .toLowerCase(),
+          loc.toLowerCase());
 
       expect(matches, hasLength(1));
       expect(find.byType(FamilyScreen), findsOneWidget);
@@ -1680,7 +1648,8 @@ void main() {
         return state.matchedLocation == '/login' ? null : '/login';
       });
 
-      expect(router.location, '/login');
+      expect(
+          router.routerDelegate.currentConfiguration.uri.toString(), '/login');
       expect(redirected, isTrue);
 
       redirected = false;
@@ -1688,7 +1657,8 @@ void main() {
       await sendPlatformUrl('/dummy', tester);
 
       await tester.pumpAndSettle();
-      expect(router.location, '/login');
+      expect(
+          router.routerDelegate.currentConfiguration.uri.toString(), '/login');
       expect(redirected, isTrue);
     });
 
@@ -1716,11 +1686,12 @@ void main() {
         return state.location;
       });
 
-      expect(router.location, '/');
+      expect(router.routerDelegate.currentConfiguration.uri.toString(), '/');
       // Directly set the url through platform message.
       await sendPlatformUrl('/dummy', tester);
       await tester.pumpAndSettle();
-      expect(router.location, '/dummy');
+      expect(
+          router.routerDelegate.currentConfiguration.uri.toString(), '/dummy');
     });
 
     testWidgets('top-level redirect w/ named routes',
@@ -1756,7 +1727,8 @@ void main() {
                 ? null
                 : state.namedLocation('login'),
       );
-      expect(router.location, '/login');
+      expect(
+          router.routerDelegate.currentConfiguration.uri.toString(), '/login');
     });
 
     testWidgets('route-level redirect', (WidgetTester tester) async {
@@ -1784,7 +1756,8 @@ void main() {
       final GoRouter router = await createRouter(routes, tester);
       router.go('/dummy');
       await tester.pump();
-      expect(router.location, '/login');
+      expect(
+          router.routerDelegate.currentConfiguration.uri.toString(), '/login');
     });
 
     testWidgets('top-level redirect take priority over route level',
@@ -1826,7 +1799,8 @@ void main() {
       await sendPlatformUrl('/dummy', tester);
 
       await tester.pumpAndSettle();
-      expect(router.location, '/login');
+      expect(
+          router.routerDelegate.currentConfiguration.uri.toString(), '/login');
       expect(redirected, isTrue);
     });
 
@@ -1860,7 +1834,8 @@ void main() {
       final GoRouter router = await createRouter(routes, tester);
       router.go('/dummy');
       await tester.pump();
-      expect(router.location, '/login');
+      expect(
+          router.routerDelegate.currentConfiguration.uri.toString(), '/login');
     });
 
     testWidgets('multiple mixed redirect', (WidgetTester tester) async {
@@ -1890,7 +1865,7 @@ void main() {
               state.matchedLocation == '/dummy1' ? '/dummy2' : null);
       router.go('/dummy1');
       await tester.pump();
-      expect(router.location, '/');
+      expect(router.routerDelegate.currentConfiguration.uri.toString(), '/');
     });
 
     testWidgets('top-level redirect loop', (WidgetTester tester) async {
@@ -2012,7 +1987,7 @@ void main() {
         tester,
         initialLocation: '/dummy',
       );
-      expect(router.location, '/');
+      expect(router.routerDelegate.currentConfiguration.uri.toString(), '/');
     });
 
     testWidgets('top-level redirect state', (WidgetTester tester) async {
@@ -2314,7 +2289,8 @@ void main() {
       await sendPlatformUrl('/dummy/dummy2', tester);
 
       await tester.pumpAndSettle();
-      expect(router.location, '/other');
+      expect(
+          router.routerDelegate.currentConfiguration.uri.toString(), '/other');
     });
   });
 
@@ -2340,7 +2316,8 @@ void main() {
         tester,
         initialLocation: '/dummy',
       );
-      expect(router.location, '/dummy');
+      expect(
+          router.routerDelegate.currentConfiguration.uri.toString(), '/dummy');
     });
 
     testWidgets('initial location with extra', (WidgetTester tester) async {
@@ -2366,7 +2343,8 @@ void main() {
         initialLocation: '/dummy',
         initialExtra: 'extra',
       );
-      expect(router.location, '/dummy');
+      expect(
+          router.routerDelegate.currentConfiguration.uri.toString(), '/dummy');
       expect(find.byKey(const ValueKey<Object?>('extra')), findsOneWidget);
     });
 
@@ -2389,7 +2367,7 @@ void main() {
         tester,
         initialLocation: '/dummy',
       );
-      expect(router.location, '/');
+      expect(router.routerDelegate.currentConfiguration.uri.toString(), '/');
     });
 
     testWidgets(
@@ -2465,7 +2443,7 @@ void main() {
         final RouteMatchList matches =
             router.routerDelegate.currentConfiguration;
 
-        expect(router.location, loc);
+        expect(router.routerDelegate.currentConfiguration.uri.toString(), loc);
         expect(matches.matches, hasLength(1));
         expect(find.byType(FamilyScreen), findsOneWidget);
         expect(matches.pathParameters['fid'], fid);
@@ -2495,7 +2473,7 @@ void main() {
         final RouteMatchList matches =
             router.routerDelegate.currentConfiguration;
 
-        expect(router.location, loc);
+        expect(router.routerDelegate.currentConfiguration.uri.toString(), loc);
         expect(matches.matches, hasLength(1));
         expect(find.byType(FamilyScreen), findsOneWidget);
         expect(matches.uri.queryParameters['fid'], fid);
@@ -2729,11 +2707,11 @@ void main() {
       await tester.pumpAndSettle();
       final RouteMatchList matches = router.routerDelegate.currentConfiguration;
 
-      expect(router.location, loc);
       expect(matches.matches, hasLength(2));
       expect(find.byType(PersonScreen), findsOneWidget);
       final ImperativeRouteMatch imperativeRouteMatch =
           matches.matches.last as ImperativeRouteMatch;
+      expect(imperativeRouteMatch.matches.uri.toString(), loc);
       expect(imperativeRouteMatch.matches.pathParameters['fid'], fid);
       expect(imperativeRouteMatch.matches.pathParameters['pid'], pid);
     });
@@ -2799,7 +2777,7 @@ void main() {
       await tester.pumpAndSettle();
       RouteMatchList matches = router.routerDelegate.currentConfiguration;
 
-      expect(router.location, loc);
+      expect(router.routerDelegate.currentConfiguration.uri.toString(), loc);
       expect(matches.matches, hasLength(4));
       expect(find.byType(PersonScreen), findsOneWidget);
       expect(matches.pathParameters['fid'], fid);
@@ -2861,7 +2839,8 @@ void main() {
           router.routerDelegate.currentConfiguration.matches;
 
       expect(matches, hasLength(1));
-      expectLocationWithQueryParams(router.location);
+      expectLocationWithQueryParams(
+          router.routerDelegate.currentConfiguration.uri.toString());
       expect(
         tester.widget<DummyScreen>(find.byType(DummyScreen)),
         isA<DummyScreen>().having(
@@ -2912,7 +2891,8 @@ void main() {
         router.routerDelegate.currentConfiguration.matches;
 
     expect(matches, hasLength(1));
-    expectLocationWithQueryParams(router.location);
+    expectLocationWithQueryParams(
+        router.routerDelegate.currentConfiguration.uri.toString());
     expect(
       tester.widget<DummyScreen>(find.byType(DummyScreen)),
       isA<DummyScreen>().having(
@@ -2962,7 +2942,8 @@ void main() {
         router.routerDelegate.currentConfiguration.matches;
 
     expect(matches, hasLength(1));
-    expectLocationWithQueryParams(router.location);
+    expectLocationWithQueryParams(
+        router.routerDelegate.currentConfiguration.uri.toString());
     expect(
       tester.widget<DummyScreen>(find.byType(DummyScreen)),
       isA<DummyScreen>().having(

packages/go_router/test/inherited_test.dart

@@ -25,7 +25,7 @@ void main() {
       expect(shouldNotify, false);
     });
 
-    test('updates when goRouter changes', () {
+    test('does not update even when goRouter changes', () {
       final GoRouter oldGoRouter = GoRouter(
         routes: <GoRoute>[
           GoRoute(
@@ -46,7 +46,7 @@ void main() {
         oldGoRouter: oldGoRouter,
         newGoRouter: newGoRouter,
       );
-      expect(shouldNotify, true);
+      expect(shouldNotify, false);
     });
   });
 

packages/go_router/test/rebuild_test.dart

@@ -0,0 +1,67 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import 'package:flutter/material.dart';
+import 'package:flutter_test/flutter_test.dart';
+import 'package:go_router/go_router.dart';
+
+import 'test_helpers.dart';
+
+void main() {
+  testWidgets('GoRouter.push does not trigger unnecessary rebuilds',
+      (WidgetTester tester) async {
+    final List<GoRoute> routes = <GoRoute>[
+      GoRoute(
+          path: '/', builder: (BuildContext context, __) => const HomePage()),
+      GoRoute(
+          path: '/1',
+          builder: (BuildContext context, __) {
+            return ElevatedButton(
+              onPressed: () {
+                context.push('/1');
+              },
+              child: const Text('/1'),
+            );
+          }),
+    ];
+
+    await createRouter(routes, tester);
+    expect(find.text('/'), findsOneWidget);
+    // first build
+    expect(HomePage.built, isTrue);
+
+    HomePage.built = false;
+    // Should not be built again afterward.
+    await tester.tap(find.text('/'));
+    await tester.pumpAndSettle();
+    expect(find.text('/1'), findsOneWidget);
+    expect(HomePage.built, isFalse);
+
+    await tester.tap(find.text('/1'));
+    await tester.pumpAndSettle();
+    expect(find.text('/1'), findsOneWidget);
+    expect(HomePage.built, isFalse);
+
+    await tester.tap(find.text('/1'));
+    await tester.pumpAndSettle();
+    expect(find.text('/1'), findsOneWidget);
+    expect(HomePage.built, isFalse);
+  });
+}
+
+class HomePage extends StatelessWidget {
+  const HomePage({super.key});
+
+  static bool built = false;
+  @override
+  Widget build(BuildContext context) {
+    built = true;
+    return ElevatedButton(
+      onPressed: () {
+        context.push('/1');
+      },
+      child: const Text('/'),
+    );
+  }
+}