-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[go_router] When replace is called, reuse the same key when possible
#2846
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
Changes from 13 commits
9098a28
b9f2edc
2b5bcba
f85f785
91e5179
7bff073
be76f27
0ea396a
286719f
303fa78
6bb466d
8d28f84
72301c9
b0a30ef
7be1963
1fd6b27
f78050b
e84f996
d6be74d
58853df
799a6c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,7 +50,7 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList> | |
| /// | ||
| /// This is used to generate a unique key for each route. | ||
| /// | ||
| /// For example, it would could be equal to: | ||
| /// For example, it could be equal to: | ||
| /// ```dart | ||
| /// { | ||
| /// 'family': 1, | ||
|
|
@@ -75,15 +75,14 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList> | |
| return false; | ||
| } | ||
|
|
||
| /// Pushes the given location onto the page stack | ||
| void push(RouteMatchList matches) { | ||
| assert(matches.last.route is! ShellRoute); | ||
|
|
||
| ValueKey<String> _getNewKeyForPath(String path) { | ||
| // Remap the pageKey to allow any number of the same page on the stack | ||
| final int count = (_pushCounts[matches.fullpath] ?? 0) + 1; | ||
| _pushCounts[matches.fullpath] = count; | ||
| final ValueKey<String> pageKey = | ||
| ValueKey<String>('${matches.fullpath}-p$count'); | ||
| final int count = (_pushCounts[path] ?? -1) + 1; | ||
| _pushCounts[path] = count; | ||
| return ValueKey<String>('$path-p$count'); | ||
| } | ||
|
|
||
| void _push(RouteMatchList matches, ValueKey<String> pageKey) { | ||
| final ImperativeRouteMatch newPageKeyMatch = ImperativeRouteMatch( | ||
| route: matches.last.route, | ||
| subloc: matches.last.subloc, | ||
|
|
@@ -94,6 +93,21 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList> | |
| ); | ||
|
|
||
| _matchList.push(newPageKeyMatch); | ||
| } | ||
|
|
||
| /// Pushes the given location onto the page stack. | ||
| /// | ||
| /// See also: | ||
| /// * [pushReplacement] which replaces the top-most page of the page stack and | ||
| /// always use a new page key. | ||
| /// * [replace] which replaces the top-most page of the page stack but treats | ||
| /// it as the same page. The page key will be reused. This will preserve the | ||
| /// state and not run any page animation. | ||
| void push(RouteMatchList matches) { | ||
| assert(matches.last.route is! ShellRoute); | ||
|
|
||
| final ValueKey<String> pageKey = _getNewKeyForPath(matches.fullpath); | ||
| _push(matches, pageKey); | ||
| notifyListeners(); | ||
| } | ||
|
|
||
|
|
@@ -146,15 +160,36 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList> | |
| return true; | ||
| } | ||
|
|
||
| /// Replaces the top-most page of the page stack with the given one. | ||
| /// Replaces the top-most page of the page stack with the given one. The page | ||
| /// key of the new page will always be different from the old one. | ||
| /// | ||
| /// See also: | ||
| /// * [push] which pushes the given location onto the page stack. | ||
| /// * [replace] which replaces the top-most page of the page stack but treats | ||
| /// it as the same page. The page key will be reused. This will preserve the | ||
| /// state and not run any page animation. | ||
| void pushReplacement(RouteMatchList matches) { | ||
| assert(matches.last.route is! ShellRoute); | ||
| _matchList.remove(_matchList.last); | ||
| push(matches); // [push] will notify the listeners. | ||
| } | ||
|
|
||
| /// Replaces the top-most page of the page stack with the given one and reuse | ||
|
||
| /// the page key when the path didn't change. | ||
| /// | ||
| /// See also: | ||
| /// * [push] which pushes the given location onto the page stack. | ||
| /// * [pushReplacement] which replaces the top-most page of the page stack but | ||
| /// always uses a new page key. | ||
| void replace(RouteMatchList matches) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method starts to deviate from Navigator.replace, which can replace any route instead of just the top one. One thing I think that may be useful would be replace GoRoute with an other GoRoute. maybe we can use GoRoute.name as here? WDYT? also cc @johnpryan for API feedback
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not trying to re-implement Having this in mind, what do you think we should do in this PR?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the key only matter when replacing the top-most page? My worry is that someone later on may file another feature request to replace the middle of match list, and then we would need to come up with a new api since the function signature here cannot be reused. I also been thinking about opening up the RouteMatchList and all the package level private class. That may open a better way to manage the RouteMatchList. So I think there are two ways to go from here. Either we expand this to be able to replace any GoRoute(or a sub list of RouteBases) with one or more RouteBases. Or we open up the API now and let customers to do their things.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is there a way you prefer over the other? I guess I can try to implement the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hello @chunhtai I tried to look a bit at how it could be possible to make void Navigator.replace({ required Route<dynamic> oldRoute, required Route<T> newRoute})So I guess, from what you said:
void replace(RouteMatchList matchese, {String? routeNameToReplace});
void replace(RouteBase newRoute, {String? routeNameToReplace});There are several issues with this:
About your concerns
The signature of the method The signature of the method void replace(String location, {Object? extra});to void replace(String location, {WhateverType? whateverParameter, Object? extra});What do you think?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am still slightly concern about using a string location to do the replace, but i can't find a better alternative. yeah, I am ok with this approach
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please re-request me for review when this pr is ready.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What approach are you okay with? The one currently implemented in this PR ? (If yes, it would be ready to review) If you are talking about void replace(RouteMatchList matchese, {String? routeNameToReplace});
void replace(RouteBase newRoute, {String? routeNameToReplace});What about
?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the current implementation |
||
| assert(matches.last.route is! ShellRoute); | ||
| final RouteMatch routeMatch = _matchList.last; | ||
| final ValueKey<String> pageKey = routeMatch.pageKey; | ||
| _matchList.remove(routeMatch); | ||
| _push(matches, pageKey); | ||
| notifyListeners(); | ||
| } | ||
|
|
||
| /// For internal use; visible for testing only. | ||
| @visibleForTesting | ||
| RouteMatchList get matches => _matchList; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,13 @@ extension GoRouterHelper on BuildContext { | |
| ); | ||
|
|
||
| /// Push a location onto the page stack. | ||
| /// | ||
| /// See also: | ||
| /// * [pushReplacement] which replaces the top-most page of the page stack and | ||
| /// always uses a new page key. | ||
| /// * [replace] which replaces the top-most page of the page stack but treats | ||
| /// it as the same page. The page key will be reused. This will preserve the | ||
| /// state and not run any page animation. | ||
| void push(String location, {Object? extra}) => | ||
| GoRouter.of(this).push(location, extra: extra); | ||
|
|
||
|
|
@@ -66,7 +73,10 @@ extension GoRouterHelper on BuildContext { | |
| /// | ||
| /// See also: | ||
| /// * [go] which navigates to the location. | ||
| /// * [push] which pushes the location onto the page stack. | ||
| /// * [push] which pushes the given location onto the page stack. | ||
| /// * [replace] which replaces the top-most page of the page stack but treats | ||
| /// it as the same page. The page key will be reused. This will preserve the | ||
| /// state and not run any page animation. | ||
| void pushReplacement(String location, {Object? extra}) => | ||
| GoRouter.of(this).pushReplacement(location, extra: extra); | ||
|
|
||
|
|
@@ -89,4 +99,29 @@ extension GoRouterHelper on BuildContext { | |
| queryParams: queryParams, | ||
| extra: extra, | ||
| ); | ||
|
|
||
| /// Replaces the top-most page of the page stack with the given one. | ||
|
||
| /// | ||
| /// See also: | ||
| /// * [push] which pushes the given location onto the page stack. | ||
| /// * [pushReplacement] which replaces the top-most page of the page stack but | ||
| /// always uses a new page key. | ||
| void replace(String location, {Object? extra}) => | ||
| GoRouter.of(this).replace(location, extra: extra); | ||
|
|
||
| /// Replaces the top-most page of the page stack with the named route w/ | ||
| /// optional parameters, e.g. `name='person', params={'fid': 'f2', 'pid': | ||
chunhtai marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| /// 'p1'}`. | ||
| /// | ||
| /// See also: | ||
| /// * [pushNamed] which pushes the given location onto the page stack. | ||
| /// * [pushReplacementNamed] which replaces the top-most page of the page | ||
| /// stack but always uses a new page key. | ||
| void replaceNamed( | ||
| String name, { | ||
| Map<String, String> params = const <String, String>{}, | ||
| Map<String, dynamic> queryParams = const <String, dynamic>{}, | ||
| Object? extra, | ||
| }) => | ||
| GoRouter.of(this).replaceNamed(name, extra: extra); | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -203,7 +203,14 @@ class GoRouter extends ChangeNotifier implements RouterConfig<RouteMatchList> { | |||||
| ); | ||||||
|
|
||||||
| /// Push a URI location onto the page stack w/ optional query parameters, e.g. | ||||||
| /// `/family/f2/person/p1?color=blue` | ||||||
| /// `/family/f2/person/p1?color=blue`. | ||||||
| /// | ||||||
| /// See also: | ||||||
| /// * [pushReplacement] which replaces the top-most page of the page stack and | ||||||
| /// always use a new page key. | ||||||
| /// * [replace] which replaces the top-most page of the page stack but treats | ||||||
| /// it as the same page. The page key will be reused. This will preserve the | ||||||
| /// state and not run any page animation. | ||||||
| void push(String location, {Object? extra}) { | ||||||
| assert(() { | ||||||
| log.info('pushing $location'); | ||||||
|
|
@@ -239,7 +246,10 @@ class GoRouter extends ChangeNotifier implements RouterConfig<RouteMatchList> { | |||||
| /// | ||||||
| /// See also: | ||||||
| /// * [go] which navigates to the location. | ||||||
| /// * [push] which pushes the location onto the page stack. | ||||||
| /// * [push] which pushes the given location onto the page stack. | ||||||
| /// * [replace] which replaces the top-most page of the page stack but treats | ||||||
| /// it as the same page. The page key will be reused. This will preserve the | ||||||
| /// state and not run any page animation. | ||||||
| void pushReplacement(String location, {Object? extra}) { | ||||||
| routeInformationParser | ||||||
| .parseRouteInformationWithDependencies( | ||||||
|
|
@@ -272,6 +282,45 @@ class GoRouter extends ChangeNotifier implements RouterConfig<RouteMatchList> { | |||||
| ); | ||||||
| } | ||||||
|
|
||||||
| /// Replaces the top-most page of the page stack with the given one. | ||||||
| /// | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here as well |
||||||
| /// See also: | ||||||
| /// * [push] which pushes the given location onto the page stack. | ||||||
| /// * [pushReplacement] which replaces the top-most page of the page stack but | ||||||
| /// always uses a new page key. | ||||||
| void replace(String location, {Object? extra}) { | ||||||
| routeInformationParser | ||||||
| .parseRouteInformationWithDependencies( | ||||||
| RouteInformation(location: location, state: extra), | ||||||
| // TODO(chunhtai): avoid accessing the context directly through global key. | ||||||
| // https://github.com/flutter/flutter/issues/99112 | ||||||
| _routerDelegate.navigatorKey.currentContext!, | ||||||
| ) | ||||||
| .then<void>((RouteMatchList matchList) { | ||||||
| routerDelegate.replace(matchList); | ||||||
| }); | ||||||
| } | ||||||
|
|
||||||
| /// Replaces the top-most page of the page stack with the named route w/ | ||||||
|
||||||
| /// Replaces the top-most page of the page stack with the named route w/ | |
| /// Replaces the top-most page of the page stack with the named route with |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for that. Hopefully docs: Improve the documentation addressed all your comments