From d1f5f87019354a62573b685a424155db89a39415 Mon Sep 17 00:00:00 2001
From: Valentin Vignal <32538273+ValentinVignal@users.noreply.github.com>
Date: Thu, 17 Nov 2022 00:22:30 +0800
Subject: [PATCH] [go_router] Make `replace` use `pop` and `push` to generate a
 new `pageKey` (#2747)

* :bug: Use pop and push in replace to generate a new pageKey

* :white_check_mark: Test that replace creates a new page key

* :arrow_up: Increase the version number

* :recycle: Move the asserts to the router deleguate

* Wrap _debugAssertMatchListNotEmpty in an assert

* Update packages/go_router/lib/src/delegate.dart

Co-authored-by: John Ryan <ryjohn@google.com>
---
 packages/go_router/CHANGELOG.md            |  4 +++
 packages/go_router/lib/src/delegate.dart   | 28 +++++++++++++++++++--
 packages/go_router/lib/src/matching.dart   | 11 --------
 packages/go_router/pubspec.yaml            |  2 +-
 packages/go_router/test/delegate_test.dart | 29 ++++++++++++++++++++++
 5 files changed, 60 insertions(+), 14 deletions(-)

diff --git a/packages/go_router/CHANGELOG.md b/packages/go_router/CHANGELOG.md
index 7653429b0b..5c448b027a 100644
--- a/packages/go_router/CHANGELOG.md
+++ b/packages/go_router/CHANGELOG.md
@@ -1,3 +1,7 @@
+## 5.1.8
+
+- Fixes a bug with `replace` where it was not generated a new `pageKey`.
+
 ## 5.1.7
 
 - Adds documentation using dartdoc topics
diff --git a/packages/go_router/lib/src/delegate.dart b/packages/go_router/lib/src/delegate.dart
index ae15383de5..1077358a6f 100644
--- a/packages/go_router/lib/src/delegate.dart
+++ b/packages/go_router/lib/src/delegate.dart
@@ -45,6 +45,18 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
   final bool routerNeglect;
 
   RouteMatchList _matchList = RouteMatchList.empty();
+
+  /// Stores the number of times each route route has been pushed.
+  ///
+  /// This is used to generate a unique key for each route.
+  ///
+  /// For example, it would could be equal to:
+  /// ```dart
+  /// {
+  ///   'family': 1,
+  ///   'family/:fid': 2,
+  /// }
+  /// ```
   final Map<String, int> _pushCounts = <String, int>{};
   final RouteConfiguration _configuration;
 
@@ -136,9 +148,21 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
     return navigatorKey.currentState?.canPop() ?? false;
   }
 
+  void _debugAssertMatchListNotEmpty() {
+    assert(
+      _matchList.isNotEmpty,
+      'You have popped the last page off of the stack,'
+      ' there are no pages left to show',
+    );
+  }
+
   /// Pop the top page off the GoRouter's page stack.
   void pop() {
     _matchList.pop();
+    assert(() {
+      _debugAssertMatchListNotEmpty();
+      return true;
+    }());
     notifyListeners();
   }
 
@@ -147,8 +171,8 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
   /// See also:
   /// * [push] which pushes the given location onto the page stack.
   void replace(RouteMatch match) {
-    _matchList.matches.last = match;
-    notifyListeners();
+    _matchList.pop();
+    push(match); // [push] will notify the listeners.
   }
 
   /// For internal use; visible for testing only.
diff --git a/packages/go_router/lib/src/matching.dart b/packages/go_router/lib/src/matching.dart
index 1775c710f4..36add3af3f 100644
--- a/packages/go_router/lib/src/matching.dart
+++ b/packages/go_router/lib/src/matching.dart
@@ -74,14 +74,10 @@ class RouteMatchList {
   void pop() {
     _matches.removeLast();
 
-    _debugAssertNotEmpty();
-
     // Also pop ShellRoutes when there are no subsequent route matches
     while (_matches.isNotEmpty && _matches.last.route is ShellRoute) {
       _matches.removeLast();
     }
-
-    _debugAssertNotEmpty();
   }
 
   /// An optional object provided by the app during navigation.
@@ -98,13 +94,6 @@ class RouteMatchList {
 
   /// Returns the error that this match intends to display.
   Exception? get error => matches.first.error;
-
-  void _debugAssertNotEmpty() {
-    assert(
-        _matches.isNotEmpty,
-        'You have popped the last page off of the stack,'
-        ' there are no pages left to show');
-  }
 }
 
 /// An error that occurred during matching.
diff --git a/packages/go_router/pubspec.yaml b/packages/go_router/pubspec.yaml
index ff285716af..2adf1acd41 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: 5.1.7
+version: 5.1.8
 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/delegate_test.dart b/packages/go_router/test/delegate_test.dart
index f60ce65fd9..61110b4ef3 100644
--- a/packages/go_router/test/delegate_test.dart
+++ b/packages/go_router/test/delegate_test.dart
@@ -154,6 +154,35 @@ void main() {
         );
       },
     );
+    testWidgets(
+      'It should return different pageKey when replace is called',
+      (WidgetTester tester) async {
+        final GoRouter goRouter = await createGoRouter(tester);
+        expect(goRouter.routerDelegate.matches.matches.length, 1);
+        expect(
+          goRouter.routerDelegate.matches.matches[0].pageKey,
+          null,
+        );
+
+        goRouter.push('/a');
+        await tester.pumpAndSettle();
+
+        expect(goRouter.routerDelegate.matches.matches.length, 2);
+        expect(
+          goRouter.routerDelegate.matches.matches.last.pageKey,
+          const Key('/a-p1'),
+        );
+
+        goRouter.replace('/a');
+        await tester.pumpAndSettle();
+
+        expect(goRouter.routerDelegate.matches.matches.length, 2);
+        expect(
+          goRouter.routerDelegate.matches.matches.last.pageKey,
+          const Key('/a-p2'),
+        );
+      },
+    );
   });
 
   group('replaceNamed', () {