Skip to content

[go_router] Make replace use pop and push to generate a new pageKey #2747

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.

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
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 5.1.4

- Fixes a bug with `replace` where it was not generated a new `pageKey`.

## 5.1.3

- Allows redirection to return same location.
Expand Down
25 changes: 23 additions & 2 deletions packages/go_router/lib/src/delegate.dart
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,18 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
final bool routerNeglect;

RouteMatchList _matchList = RouteMatchList.empty();

/// Stores the number of time 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;

Expand Down Expand Up @@ -136,9 +148,18 @@ 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();
_debugAssertMatchListNotEmpty();
Copy link
Contributor

Choose a reason for hiding this comment

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

you can also wrap this in an assert so that release mode will not even waste time calling into an empty function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is 8e8d187 what you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

notifyListeners();
}

Expand All @@ -147,8 +168,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.
Expand Down
11 changes: 0 additions & 11 deletions packages/go_router/lib/src/matching.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion packages/go_router/pubspec.yaml
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: 5.1.3
version: 5.1.4
Copy link
Contributor

Choose a reason for hiding this comment

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

5.1.3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the newly merged PRs, I'll make it 5.1.6

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
29 changes: 29 additions & 0 deletions packages/go_router/test/delegate_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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', () {
Expand Down