-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[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
Merged
auto-submit
merged 21 commits into
flutter:main
from
ValentinVignal:go-router/reuse-same-key-when-replace
Mar 21, 2023
Merged
Changes from all commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
9098a28
:recycle: Use PageKey
ValentinVignal b9f2edc
:sparkles: Add replace method
ValentinVignal 2b5bcba
:whie_check_mark: Update the tests
ValentinVignal f85f785
:arrow_up: Update the version number
ValentinVignal 91e5179
Merge branch 'main' into go-router/reuse-same-key-when-replace
ValentinVignal 7bff073
:sparkles: Add replaceNamed
ValentinVignal be76f27
Merge remote-tracking branch 'upstream/main' into go-router/reuse-sam…
ValentinVignal 0ea396a
Merge remote-tracking branch 'upstream/main' into go-router/reuse-sam…
ValentinVignal 286719f
Merge remote-tracking branch 'upstream/main' into go-router/reuse-sam…
ValentinVignal 303fa78
Merge branch 'main' into go-router/reuse-same-key-when-replace
ValentinVignal 6bb466d
:necktie: Always reuse the same key when using replace
ValentinVignal 8d28f84
Merge remote-tracking branch 'upstream/main' into go-router/reuse-sam…
ValentinVignal 72301c9
docs: Update the doc of replace to mention animation and state
ValentinVignal b0a30ef
Merge branch 'main' into go-router/reuse-same-key-when-replace
ValentinVignal 7be1963
docs: Add more doc to replace and replaceNamed
ValentinVignal 1fd6b27
Merge remote-tracking branch 'upstream/main' into go-router/reuse-sam…
ValentinVignal f78050b
Merge branch 'main' into go-router/reuse-same-key-when-replace
ditman e84f996
Merge remote-tracking branch 'upstream/main' into go-router/reuse-sam…
ValentinVignal d6be74d
Merge remote-tracking branch 'upstream/main' into go-router/reuse-sam…
ValentinVignal 58853df
docs: Improve the documentation
ValentinVignal 799a6c0
Merge remote-tracking branch 'upstream/main' into go-router/reuse-sam…
ValentinVignal File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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,52 @@ class GoRouter extends ChangeNotifier implements RouterConfig<RouteMatchList> { | |
); | ||
} | ||
|
||
/// Replaces the top-most page of the page stack with the given one but treats | ||
/// it as the same page. | ||
/// | ||
/// The page key will be reused. This will preserve the state and not run any | ||
/// page animation. | ||
/// | ||
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 with the named route and optional parameters, | ||
/// preserving the page key. | ||
/// | ||
/// This will preserve the state and not run any page animation. Optional | ||
/// parameters can be providded to the named route, e.g. `name='person', | ||
/// params={'fid': 'f2', 'pid': '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, | ||
}) { | ||
replace( | ||
namedLocation(name, params: params, queryParams: queryParams), | ||
extra: extra, | ||
); | ||
} | ||
|
||
/// Pop the top-most route off the current screen. | ||
/// | ||
/// If the top-most route is a pop up or dialog, this method pops it instead | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
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.
I'm not trying to re-implement
Navigator.replace
API here. The main goal of this PR is to fix flutter/flutter#115902. The solution I came up with was thisreplace
method (which we could name differently, ex:replaceLast
). I don't know whether re-implementingNavigator.replace
would fix the issue (maybe it will not re-use the same key, likepushReplacement
) or not.Having this in mind, what do you think we should do in this PR?
Uh oh!
There was an error while loading. Please reload this page.
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.
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.
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.
Is there a way you prefer over the other? I guess I can try to implement the
replace
method hereThere 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.
Hello @chunhtai I tried to look a bit at how it could be possible to make
replace
a bit likeSo I guess, from what you said:
replace
could have one of the signatures (ifrouteNameToReplace
isnull
, it would replace the top most route).There are several issues with this:
go_router_builder
as all the generated routes are nameless.About your concerns
The signature of the method
replace
of the delegate could be changed easily as it is not opened yet, so there should not be any issues there.The signature of the method
replace
of the router should not be breaking. It could change fromto
What do you think?
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.
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
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.
please re-request me for review when this pr is ready.
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.
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
What about
?
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.
the current implementation