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
[go_router] Return a value on pop #2416
Changes from 10 commits
2f515a8
25577e2
2be6daa
05ab251
9f20a71
90954e1
1750b55
06a2ae5
b77cc10
a5a1df9
977606e
d57d39f
f1bfddd
46d5899
38814a8
8e99376
1e6abfe
87818e2
6de9f2a
121c560
63057b5
2c56459
48688b8
5b94f01
fbc8d0c
dd05959
5e27ae3
0488a2e
10e1181
b7431c8
3fb8ccd
0061c32
5fffdf3
133e621
12c7d13
12eef5e
30449ef
57f7f82
a1397dc
6fe7ae0
5a7418d
f8eab3a
6f9fa08
5d909fe
6ef8dfd
ce29ba7
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 |
---|---|---|
|
@@ -44,13 +44,21 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList> | |
|
||
final GlobalKey<NavigatorState> _key = GlobalKey<NavigatorState>(); | ||
|
||
/// The list of completers for the promises when pushing asynchronous routes. | ||
final Map<String, Completer<dynamic>> _completers = | ||
<String, Completer<dynamic>>{}; | ||
RouteMatchList _matches = RouteMatchList.empty(); | ||
final Map<String, int> _pushCounts = <String, int>{}; | ||
|
||
/// Pushes the given location onto the page stack | ||
void push(RouteMatch match) { | ||
// Remap the pageKey to allow any number of the same page on the stack | ||
/// Pushes the given location onto the page stack with an optional promise. | ||
Future<T?> push<T extends Object?>(RouteMatch match) { | ||
// Remap the pageKey to allow any number of the same page on the stack. | ||
final String fullPath = match.fullpath; | ||
|
||
// Create a completer for the promise and store it in the completers map. | ||
final Completer<T> completer = Completer<T>(); | ||
_completers[fullPath] = completer; | ||
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. It's should be replaced by 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. It will not because T extends Object?, so T is out of the box nullable 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. If I write: I tested with flutter master channel. 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 changed it, hope that fixes it |
||
|
||
final int count = (_pushCounts[fullPath] ?? 0) + 1; | ||
_pushCounts[fullPath] = count; | ||
final ValueKey<String> pageKey = ValueKey<String>('$fullPath-p$count'); | ||
|
@@ -67,15 +75,27 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList> | |
|
||
_matches.push(newPageKeyMatch); | ||
notifyListeners(); | ||
return completer.future; | ||
} | ||
|
||
/// Returns `true` if there is more than 1 page on the stack. | ||
bool canPop() { | ||
return _matches.canPop(); | ||
} | ||
|
||
/// Pop the top page off the GoRouter's page stack. | ||
void pop() { | ||
/// Pop the top page off the GoRouter's page stack and complete a promise if | ||
/// there is one. | ||
void pop<T extends Object?>([T? value]) { | ||
johnpryan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
final RouteMatch last = _matches.last; | ||
|
||
// If there is a promise for this page, complete it. | ||
final Completer<T>? completer = _completers[last.fullpath] as Completer<T>?; | ||
if (completer != null) { | ||
completer.complete(value); | ||
} | ||
|
||
// Remove promise from completers map. | ||
_completers.remove(last.fullpath); | ||
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 completer should be removed from the list as well when users call |
||
_matches.pop(); | ||
notifyListeners(); | ||
} | ||
|
@@ -85,7 +105,17 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList> | |
/// See also: | ||
/// * [push] which pushes the given location onto the page stack. | ||
void replace(RouteMatch match) { | ||
final String lastPath = _matches.last.fullpath; | ||
final Completer<dynamic>? completer = _completers[lastPath]; | ||
|
||
// If there's a promise for the last page, we update it to make it point to | ||
// the new page. | ||
if (completer != null) { | ||
_completers[match.fullpath] = completer; | ||
_completers.remove(lastPath); | ||
} | ||
_matches.matches.last = match; | ||
|
||
notifyListeners(); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -1676,7 +1676,7 @@ void main() { | |||||
title: 'GoRouter Example', | ||||||
), | ||||||
); | ||||||
key.currentContext!.namedLocation( | ||||||
key.currentContext?.namedLocation( | ||||||
name, | ||||||
params: params, | ||||||
queryParams: queryParams, | ||||||
|
@@ -1696,7 +1696,7 @@ void main() { | |||||
title: 'GoRouter Example', | ||||||
), | ||||||
); | ||||||
key.currentContext!.go( | ||||||
key.currentContext?.go( | ||||||
location, | ||||||
extra: extra, | ||||||
); | ||||||
|
@@ -1715,7 +1715,7 @@ void main() { | |||||
title: 'GoRouter Example', | ||||||
), | ||||||
); | ||||||
key.currentContext!.goNamed( | ||||||
key.currentContext?.goNamed( | ||||||
name, | ||||||
params: params, | ||||||
queryParams: queryParams, | ||||||
|
@@ -1738,14 +1738,34 @@ void main() { | |||||
title: 'GoRouter Example', | ||||||
), | ||||||
); | ||||||
key.currentContext!.push( | ||||||
await key.currentContext?.push( | ||||||
location, | ||||||
extra: extra, | ||||||
); | ||||||
expect(router.myLocation, location); | ||||||
expect(router.extra, extra); | ||||||
}); | ||||||
|
||||||
testWidgets('calls [push] on closest GoRouter with a promise', | ||||||
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.
Suggested change
|
||||||
(WidgetTester tester) async { | ||||||
final GoRouterPushAsyncSpy router = GoRouterPushAsyncSpy(routes: routes); | ||||||
await tester.pumpWidget( | ||||||
MaterialApp.router( | ||||||
routeInformationProvider: router.routeInformationProvider, | ||||||
routeInformationParser: router.routeInformationParser, | ||||||
routerDelegate: router.routerDelegate, | ||||||
title: 'GoRouter Example', | ||||||
), | ||||||
); | ||||||
final String? result = await key.currentContext?.push<String>( | ||||||
location, | ||||||
extra: extra, | ||||||
); | ||||||
expect(result, extra); | ||||||
expect(router.myLocation, location); | ||||||
expect(router.extra, extra); | ||||||
}); | ||||||
|
||||||
testWidgets('calls [pushNamed] on closest GoRouter', | ||||||
(WidgetTester tester) async { | ||||||
final GoRouterPushNamedSpy router = GoRouterPushNamedSpy(routes: routes); | ||||||
|
@@ -1757,16 +1777,41 @@ void main() { | |||||
title: 'GoRouter Example', | ||||||
), | ||||||
); | ||||||
key.currentContext!.pushNamed( | ||||||
await key.currentContext?.pushNamed( | ||||||
name, | ||||||
params: params, | ||||||
queryParams: queryParams, | ||||||
extra: extra, | ||||||
); | ||||||
expect(router.extra, extra); | ||||||
expect(router.name, name); | ||||||
expect(router.params, params); | ||||||
expect(router.queryParams, queryParams); | ||||||
}); | ||||||
|
||||||
testWidgets('calls [pushNamed] on closest GoRouter with a promise', | ||||||
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.
Suggested change
|
||||||
(WidgetTester tester) async { | ||||||
final GoRouterPushNamedAsyncSpy router = | ||||||
GoRouterPushNamedAsyncSpy(routes: routes); | ||||||
await tester.pumpWidget( | ||||||
MaterialApp.router( | ||||||
routeInformationProvider: router.routeInformationProvider, | ||||||
routeInformationParser: router.routeInformationParser, | ||||||
routerDelegate: router.routerDelegate, | ||||||
title: 'GoRouter Example', | ||||||
), | ||||||
); | ||||||
final String? result = await key.currentContext?.pushNamed<String>( | ||||||
name, | ||||||
params: params, | ||||||
queryParams: queryParams, | ||||||
extra: extra, | ||||||
); | ||||||
expect(result, extra); | ||||||
expect(router.extra, extra); | ||||||
expect(router.name, name); | ||||||
expect(router.params, params); | ||||||
expect(router.queryParams, queryParams); | ||||||
}); | ||||||
|
||||||
testWidgets('calls [pop] on closest GoRouter', (WidgetTester tester) async { | ||||||
|
@@ -1779,7 +1824,7 @@ void main() { | |||||
title: 'GoRouter Example', | ||||||
), | ||||||
); | ||||||
key.currentContext!.pop(); | ||||||
key.currentContext?.pop(); | ||||||
expect(router.popped, true); | ||||||
}); | ||||||
}); | ||||||
|
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.
It would probably make more sense to store the completers on RouteMatch. We would need RouteMatch to implement
==
andhashCode
and make RouteMatchList check for equality rather than treating them as immutable.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 trying with this approach, the main problem are the go and goNamed, where would I store the completer?
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.
go and goNamed should create a RouteMatch - I'm not sure I understand your question though.
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.
So the problem is that go and goNamed don't create a RouteMatch, but we can create them.
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.
@johnpryan After going into the nature of the go and goNamed methods it doesn't make any sense to implement this into the method, because it basically redirect to a specific path removing all the things below it.
You can read more about it here
I'm pushing changes for your review rn.