Skip to content

[go_router]: add optional completer for replaced routes #6787

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

[go_router]: add optional completer for replaced routes #6787

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d3a7cc2
[go_router]: add optional completer for replaced routes
pawlowskim May 24, 2024
bb6880b
[go_router]: Update changelog
AleksanderBoltryk Jun 11, 2024
e8160c1
Merge pull request #1 from Olosss/issue-141251-push-replacement-futur…
pawlowskim Jun 13, 2024
6fb4b62
[go_router]: Update pubspec
AleksanderBoltryk Jun 13, 2024
426b705
Merge pull request #2 from Olosss/issue-141251-push-replacement-futur…
pawlowskim Jun 13, 2024
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
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 14.1.5

- Add optional completer for replaced routes ([#141251](https://github.com/flutter/flutter/issues/141251))

## 14.1.4

- Fixes a URL in `navigation.md`.
Expand Down
32 changes: 26 additions & 6 deletions packages/go_router/lib/src/information_provider.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ class RouteInformationState<T> {
RouteInformationState({
this.extra,
this.completer,
this.onReplaceCompleter,
this.baseRouteMatchList,
required this.type,
}) : assert((type == NavigatingType.go || type == NavigatingType.restore) ==
Expand All @@ -62,6 +63,10 @@ class RouteInformationState<T> {
/// [NavigatingType.restore].
final Completer<T?>? completer;

/// The completer that needs to be completed when route is replaced.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// The completer that needs to be completed when route is replaced.
/// The completer that will be completed when route is replaced.

/// [completer] is ignored when replacing routes.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add only only used when type is NavigatingType.push or NavigatingType.pushReplacement
NavigatingType.replace

final Completer<T?>? onReplaceCompleter;

/// The base route match list to push on top to.
///
/// This is only null if [type] is [NavigatingType.go].
Expand Down Expand Up @@ -146,8 +151,12 @@ class GoRouteInformationProvider extends RouteInformationProvider
}

/// Pushes the `location` as a new route on top of `base`.
Future<T?> push<T>(String location,
{required RouteMatchList base, Object? extra}) {
Future<T?> push<T>(
String location, {
required RouteMatchList base,
Object? extra,
Completer<T?>? onReplaceCompleter,
}) {
final Completer<T?> completer = Completer<T?>();
_setValue(
location,
Expand All @@ -156,6 +165,7 @@ class GoRouteInformationProvider extends RouteInformationProvider
baseRouteMatchList: base,
completer: completer,
type: NavigatingType.push,
onReplaceCompleter: onReplaceCompleter,
),
);
return completer.future;
Expand Down Expand Up @@ -186,8 +196,12 @@ class GoRouteInformationProvider extends RouteInformationProvider

/// Removes the top-most route match from `base` and pushes the `location` as a
/// new route on top.
Future<T?> pushReplacement<T>(String location,
{required RouteMatchList base, Object? extra}) {
Future<T?> pushReplacement<T>(
String location, {
required RouteMatchList base,
Object? extra,
Completer<T?>? onReplaceCompleter,
}) {
final Completer<T?> completer = Completer<T?>();
_setValue(
location,
Expand All @@ -196,14 +210,19 @@ class GoRouteInformationProvider extends RouteInformationProvider
baseRouteMatchList: base,
completer: completer,
type: NavigatingType.pushReplacement,
onReplaceCompleter: onReplaceCompleter,
),
);
return completer.future;
}

/// Replaces the top-most route match from `base` with the `location`.
Future<T?> replace<T>(String location,
{required RouteMatchList base, Object? extra}) {
Future<T?> replace<T>(
String location, {
required RouteMatchList base,
Object? extra,
Completer<T?>? onReplaceCompleter,
}) {
final Completer<T?> completer = Completer<T?>();
_setValue(
location,
Expand All @@ -212,6 +231,7 @@ class GoRouteInformationProvider extends RouteInformationProvider
baseRouteMatchList: base,
completer: completer,
type: NavigatingType.replace,
onReplaceCompleter: onReplaceCompleter,
),
);
return completer.future;
Expand Down
23 changes: 19 additions & 4 deletions packages/go_router/lib/src/match.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,9 +421,12 @@ class ShellRouteMatch extends RouteMatchBase {
/// The route match that represent route pushed through [GoRouter.push].
class ImperativeRouteMatch extends RouteMatch {
/// Constructor for [ImperativeRouteMatch].
ImperativeRouteMatch(
{required super.pageKey, required this.matches, required this.completer})
: super(
ImperativeRouteMatch({
required super.pageKey,
required this.matches,
required this.completer,
this.onReplaceCompleter,
}) : super(
route: _getsLastRouteFromMatches(matches),
matchedLocation: _getsMatchedLocationFromMatches(matches),
);
Expand All @@ -449,6 +452,12 @@ class ImperativeRouteMatch extends RouteMatch {
/// The completer for the future returned by [GoRouter.push].
final Completer<Object?> completer;

/// The completer for the future when [GoRouter.pushReplacement]
/// [GoRouter.replace] replaces the current route.
///
/// Completer from the method call is put into the void when route is replaced.
final Completer<Object?>? onReplaceCompleter;

/// Called when the corresponding [Route] associated with this route match is
/// completed.
void complete([dynamic value]) {
Expand All @@ -466,11 +475,17 @@ class ImperativeRouteMatch extends RouteMatch {
return other is ImperativeRouteMatch &&
completer == other.completer &&
matches == other.matches &&
onReplaceCompleter == other.onReplaceCompleter &&
super == other;
}

@override
int get hashCode => Object.hash(super.hashCode, completer, matches.hashCode);
int get hashCode => Object.hash(
super.hashCode,
completer,
matches.hashCode,
onReplaceCompleter,
);
}

/// The list of [RouteMatchBase] objects.
Expand Down
54 changes: 44 additions & 10 deletions packages/go_router/lib/src/misc/extensions.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:async';

import 'package:flutter/widgets.dart';

import '../router.dart';
Expand Down Expand Up @@ -46,21 +48,31 @@ extension GoRouterHelper on BuildContext {
/// * [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.
Future<T?> push<T extends Object?>(String location, {Object? extra}) =>
GoRouter.of(this).push<T>(location, extra: extra);
Future<T?> push<T extends Object?>(
String location, {
Object? extra,
Completer<T?>? onReplaceCompleter,
}) =>
GoRouter.of(this).push<T>(
location,
extra: extra,
onReplaceCompleter: onReplaceCompleter,
);

/// Navigate to a named route onto the page stack.
Future<T?> pushNamed<T extends Object?>(
String name, {
Map<String, String> pathParameters = const <String, String>{},
Map<String, dynamic> queryParameters = const <String, dynamic>{},
Object? extra,
Completer<T?>? onReplaceCompleter,
}) =>
GoRouter.of(this).pushNamed<T>(
name,
pathParameters: pathParameters,
queryParameters: queryParameters,
extra: extra,
onReplaceCompleter: onReplaceCompleter,
);

/// Returns `true` if there is more than 1 page on the stack.
Expand All @@ -79,8 +91,16 @@ extension GoRouterHelper on BuildContext {
/// * [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);
void pushReplacement(
Copy link
Contributor

Choose a reason for hiding this comment

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

just took a quick glance, should we make this return future instead of passing in a completer?

Copy link

Choose a reason for hiding this comment

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

@chunhtai

If we have page A -> push -> page B -> pushReplacement -> page C,

this still won't solve the problem because although we can track when the completer from pushReplacement finishes on page B by returning a Future, the Future on page A will never complete.

In the _updateRouteMatchList function, in the case of NavigatingType.pushReplacement, the completer responsible for the future in the push method is simply removed and never completed.

Passing a completer through the push method allows us to detect when such a page has been replaced.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the this is intended? the first push will never complete since it was replaced by another page regardless whether you pass in the completer or not

String location, {
Object? extra,
Completer<Object?>? onReplaceCompleter,
}) =>
GoRouter.of(this).pushReplacement(
location,
extra: extra,
onReplaceCompleter: onReplaceCompleter,
);

/// Replaces the top-most page of the page stack with the named route w/
/// optional parameters, e.g. `name='person', pathParameters={'fid': 'f2', 'pid':
Expand All @@ -94,12 +114,14 @@ extension GoRouterHelper on BuildContext {
Map<String, String> pathParameters = const <String, String>{},
Map<String, dynamic> queryParameters = const <String, dynamic>{},
Object? extra,
Completer<Object?>? onReplaceCompleter,
}) =>
GoRouter.of(this).pushReplacementNamed(
name,
pathParameters: pathParameters,
queryParameters: queryParameters,
extra: extra,
onReplaceCompleter: onReplaceCompleter,
);

/// Replaces the top-most page of the page stack with the given one but treats
Expand All @@ -112,8 +134,16 @@ extension GoRouterHelper on BuildContext {
/// * [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<Object?>(location, extra: extra);
void replace(
String location, {
Object? extra,
Completer<Object?>? onReplaceCompleter,
}) =>
GoRouter.of(this).replace<Object?>(
location,
extra: extra,
onReplaceCompleter: onReplaceCompleter,
);

/// Replaces the top-most page with the named route and optional parameters,
/// preserving the page key.
Expand All @@ -131,9 +161,13 @@ extension GoRouterHelper on BuildContext {
Map<String, String> pathParameters = const <String, String>{},
Map<String, dynamic> queryParameters = const <String, dynamic>{},
Object? extra,
Completer<Object?>? onReplaceCompleter,
}) =>
GoRouter.of(this).replaceNamed<Object?>(name,
pathParameters: pathParameters,
queryParameters: queryParameters,
extra: extra);
GoRouter.of(this).replaceNamed<Object?>(
name,
pathParameters: pathParameters,
queryParameters: queryParameters,
extra: extra,
onReplaceCompleter: onReplaceCompleter,
);
}
11 changes: 11 additions & 0 deletions packages/go_router/lib/src/parser.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ class GoRouteInformationParser extends RouteInformationParser<RouteMatchList> {
baseRouteMatchList: state.baseRouteMatchList,
completer: state.completer,
type: state.type,
onReplaceCompleter: state.onReplaceCompleter,
);
});
}
Expand Down Expand Up @@ -182,6 +183,7 @@ class GoRouteInformationParser extends RouteInformationParser<RouteMatchList> {
required RouteMatchList? baseRouteMatchList,
required Completer<Object?>? completer,
required NavigatingType type,
Completer<Object?>? onReplaceCompleter,
}) {
switch (type) {
case NavigatingType.push:
Expand All @@ -190,24 +192,33 @@ class GoRouteInformationParser extends RouteInformationParser<RouteMatchList> {
pageKey: _getUniqueValueKey(),
completer: completer!,
matches: newMatchList,
onReplaceCompleter: onReplaceCompleter,
),
);
case NavigatingType.pushReplacement:
final RouteMatch routeMatch = baseRouteMatchList!.last;
if (routeMatch is ImperativeRouteMatch) {
routeMatch.onReplaceCompleter?.complete();
}
return baseRouteMatchList.remove(routeMatch).push(
ImperativeRouteMatch(
pageKey: _getUniqueValueKey(),
completer: completer!,
matches: newMatchList,
onReplaceCompleter: onReplaceCompleter,
),
);
case NavigatingType.replace:
final RouteMatch routeMatch = baseRouteMatchList!.last;
if (routeMatch is ImperativeRouteMatch) {
routeMatch.onReplaceCompleter?.complete();
}
return baseRouteMatchList.remove(routeMatch).push(
ImperativeRouteMatch(
pageKey: routeMatch.pageKey,
completer: completer!,
matches: newMatchList,
onReplaceCompleter: onReplaceCompleter,
),
);
case NavigatingType.go:
Expand Down
Loading