[go_router]: add optional completer for replaced routes #6787
Conversation
| @@ -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( | |||
There was a problem hiding this comment.
just took a quick glance, should we make this return future instead of passing in a completer?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
…e-lost-v2 Issue 141251 push replacement future lost v2
…e-lost-v2 [go_router]: Update pubspec
| @@ -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: 14.1.4 | |||
| version: 14.1.5 | |||
There was a problem hiding this comment.
This is a new api should bump minor version
| @@ -62,6 +63,10 @@ class RouteInformationState<T> { | |||
| /// [NavigatingType.restore]. | |||
| final Completer<T?>? completer; | |||
|
|
|||
| /// The completer that needs to be completed when route is replaced. | |||
There was a problem hiding this comment.
| /// The completer that needs to be completed when route is replaced. | |
| /// The completer that will be completed when route is replaced. |
| @@ -62,6 +63,10 @@ class RouteInformationState<T> { | |||
| /// [NavigatingType.restore]. | |||
| final Completer<T?>? completer; | |||
|
|
|||
| /// The completer that needs to be completed when route is replaced. | |||
| /// [completer] is ignored when replacing routes. | |||
There was a problem hiding this comment.
maybe add only only used when type is NavigatingType.push or NavigatingType.pushReplacement
NavigatingType.replace
|
Hi @pawlowskim Are you still planning on coming back to this pr? |
|
Closing the pr for now. feel free to reopen once you have time |
When route from the stack is replaced, its completer is lost, and causes futures in routes chain to never finish. This change add optional completer that can be passed when pushing / replacing routes that will be completed when route is replaced.
It fixes: flutter/flutter#141251
This PR is continuation of: #6451
CC @chunhtai
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).