Skip to content

[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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
9098a28
:recycle: Use PageKey
ValentinVignal Dec 23, 2022
b9f2edc
:sparkles: Add replace method
ValentinVignal Dec 23, 2022
2b5bcba
:whie_check_mark: Update the tests
ValentinVignal Dec 23, 2022
f85f785
:arrow_up: Update the version number
ValentinVignal Dec 23, 2022
91e5179
Merge branch 'main' into go-router/reuse-same-key-when-replace
ValentinVignal Jan 9, 2023
7bff073
:sparkles: Add replaceNamed
ValentinVignal Jan 9, 2023
be76f27
Merge remote-tracking branch 'upstream/main' into go-router/reuse-sam…
ValentinVignal Feb 7, 2023
0ea396a
Merge remote-tracking branch 'upstream/main' into go-router/reuse-sam…
ValentinVignal Feb 9, 2023
286719f
Merge remote-tracking branch 'upstream/main' into go-router/reuse-sam…
ValentinVignal Feb 15, 2023
303fa78
Merge branch 'main' into go-router/reuse-same-key-when-replace
ValentinVignal Feb 17, 2023
6bb466d
:necktie: Always reuse the same key when using replace
ValentinVignal Feb 17, 2023
8d28f84
Merge remote-tracking branch 'upstream/main' into go-router/reuse-sam…
ValentinVignal Feb 19, 2023
72301c9
docs: Update the doc of replace to mention animation and state
ValentinVignal Feb 19, 2023
b0a30ef
Merge branch 'main' into go-router/reuse-same-key-when-replace
ValentinVignal Feb 22, 2023
7be1963
docs: Add more doc to replace and replaceNamed
ValentinVignal Feb 22, 2023
1fd6b27
Merge remote-tracking branch 'upstream/main' into go-router/reuse-sam…
ValentinVignal Feb 23, 2023
f78050b
Merge branch 'main' into go-router/reuse-same-key-when-replace
ditman Mar 1, 2023
e84f996
Merge remote-tracking branch 'upstream/main' into go-router/reuse-sam…
ValentinVignal Mar 3, 2023
d6be74d
Merge remote-tracking branch 'upstream/main' into go-router/reuse-sam…
ValentinVignal Mar 5, 2023
58853df
docs: Improve the documentation
ValentinVignal Mar 5, 2023
799a6c0
Merge remote-tracking branch 'upstream/main' into go-router/reuse-sam…
ValentinVignal Mar 21, 2023
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 @@
## 6.4.0

- Adds `replace` method to that replaces the current route with a new one and keeps the same page key. This is useful for when you want to update the query params without changing the page key ([#115902]https://github.com/flutter/flutter/issues/115902).

## 6.3.0

- Aligns Dart and Flutter SDK constraints.
Expand Down
57 changes: 48 additions & 9 deletions packages/go_router/lib/src/delegate.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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();
}

Expand Down Expand Up @@ -148,13 +162,38 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>

/// 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 but treats
/// it as the same page.
///
/// The page key will be reused. This will preserve the state and not run any
/// page animation.
///
/// 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) {
Copy link
Contributor

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

Copy link
Contributor Author

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 this replace method (which we could name differently, ex: replaceLast). I don't know whether re-implementing Navigator.replace would fix the issue (maybe it will not re-use the same key, like pushReplacement) or not.

Having this in mind, what do you think we should do in this PR?

Copy link
Contributor

@chunhtai chunhtai Feb 2, 2023

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Is there a way you prefer over the other? I guess I can try to implement the replace method here

Copy link
Contributor Author

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 like

void Navigator.replace({ required Route<dynamic> oldRoute, required Route<T> newRoute})

So I guess, from what you said:

One thing I think that may be useful would be replace GoRoute with an other GoRoute. maybe we can use GoRoute.name as here

replace could have one of the signatures (if routeNameToReplace is null, it would replace the top most route).

void replace(RouteMatchList matchese, {String? routeNameToReplace});
void replace(RouteBase newRoute, {String? routeNameToReplace});

There are several issues with this:

  • If a route is present multiple times in the history, which one is supposed to be replaced?
  • This solution doesn't work with go_router_builder as all the generated routes are nameless.

About your concerns

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.

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 from

void replace(String location, {Object? extra});

to

void replace(String location, {WhateverType? whateverParameter, Object? extra});

What do you think?

Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I am ok with this approach

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

There are several issues with this:

  • If a route is present multiple times in the history, which one is supposed to be replaced?
  • This solution doesn't work with go_router_builder as all the generated routes are nameless.

?

Copy link
Contributor

Choose a reason for hiding this comment

The 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;
Expand Down
44 changes: 43 additions & 1 deletion packages/go_router/lib/src/misc/extensions.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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);

Expand All @@ -89,4 +99,36 @@ extension GoRouterHelper on BuildContext {
queryParams: queryParams,
extra: extra,
);

/// 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.
///
/// 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 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,
}) =>
GoRouter.of(this).replaceNamed(name, extra: extra);
}
60 changes: 58 additions & 2 deletions packages/go_router/lib/src/router.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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.
///
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand Down
7 changes: 5 additions & 2 deletions packages/go_router/lib/src/state.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

import 'package:flutter/widgets.dart';

import '../go_router.dart';
import 'configuration.dart';
import 'misc/errors.dart';

Expand Down Expand Up @@ -64,7 +63,11 @@ class GoRouterState {
/// The error associated with this sub-route.
final Exception? error;

/// A unique string key for this sub-route, e.g. ValueKey('/family/:fid')
/// A unique string key for this sub-route.
/// E.g.
/// ```dart
/// ValueKey('/family/:fid')
/// ```
final ValueKey<String> pageKey;

/// Gets the [GoRouterState] from context.
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: 6.3.0
version: 6.4.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

Expand Down
Loading