flutter
[go_router] Add `popUntil`
Part of https://github.com/flutter/flutter/issues/144924
Closes https://github.com/flutter/flutter/issues/131625
Pre-launch Checklist
- [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
- [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
- [x] I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use
dart format.) - [x] I signed the CLA.
- [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g.
[shared_preferences] - [x] I linked to at least one issue that this PR fixes in the description above.
- [x] I updated
pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes. - [x] I updated
CHANGELOG.mdto add a description of the change, following repository CHANGELOG style. - [x] I updated/added relevant documentation (doc comments with
///). - [x] I added new tests to check the change I am making, or this PR is test-exempt.
- [x] All existing and new tests are passing.
If you need help, consider asking for advice on the #hackers-new channel on Discord.
@chunhtai @johnpryan @hangyujin , do you have any feedback on this PR?
Yes, but I'm trying to finish onExit first, I want to make sure I get the right logic to retrieve the state there before I do it here too
@chunhtai I left you a message here, I was waiting for your feed back
https://github.com/flutter/packages/pull/6306#discussion_r1618629351
@chunhtai I'm facing an issue with ShellRoute that I'm not really sure how to handle.
I wrote some tests in test: Add some tests and the last one ("It should pop the last 2 routes and the ShellRoute with it") is failing.
It successfully pops /a/b/c but when it tries to pop /a/b (and the shell route with it), it ends up with RouteMatchBase._removeRouteMatchFromList:
https://github.com/flutter/packages/blob/e95fe4a0cd3a08a05a0d0b51423f6bcd3d605d9f/packages/go_router/lib/src/match.dart#L686-L693
with matches being "correct" [RouteMatch('/'), RouteMatch('/a'), ShellRouteMatch].
But target is "outdated",
it is also a ShellRouteMatch, but it still has 2 items in its matches (c didn't get popped) ; unlike the ShellRouteMatch from matches which now has only b in its matches (c correctly got popped).
Because of that https://github.com/flutter/packages/blob/e95fe4a0cd3a08a05a0d0b51423f6bcd3d605d9f/packages/go_router/lib/src/match.dart#L691 is false
and the ShellRouteMatch is never popped.
Looking at the code, the outdated target variable comes from
https://github.com/flutter/packages/blob/e95fe4a0cd3a08a05a0d0b51423f6bcd3d605d9f/packages/go_router/lib/src/builder.dart#L413-L417
The _page variable is only updated in the build method,
https://github.com/flutter/packages/blob/e95fe4a0cd3a08a05a0d0b51423f6bcd3d605d9f/packages/go_router/lib/src/builder.dart#L419-L425
but there is no build run while using popUntil.
This strategy of updating the state in the build method looks suspicious to me, I guess this is to provide a valid context to the different buildPages.
Do you have any suggestions on what we could do here?
@chunhtai would you want to have a chat about it? How should we proceed?
I am still trying to understand the situation.
The _page variable is only updated in the build method,
so I assume this is some timing issue with the code?
This strategy of updating the state in the build method looks suspicious to me, I guess this is to provide a valid context to the different buildPages.
yeah i agree this is probably not a good strategy. Since they are popped within the same frame, the _pageToRouteMatchBase has not yet updated. I wonder if the pop should proactively update the _pageToRouteMatchBase without going through build.
Hi @ValentinVignal can you try this?
I wonder if the pop should proactively update the _pageToRouteMatchBase without going through build.
I've added
bool _handlePopPage(Route<Object?> route, Object? result) {
final Page<Object?> page = route.settings as Page<Object?>;
final RouteMatchBase match = _pageToRouteMatchBase[page]!;
- return widget.onPopPageWithRouteMatch(route, result, match);
+ final bool didPop = widget.onPopPageWithRouteMatch(route, result, match);
+ if (didPop) {
+ _pages?.remove(page);
+ // Update the registery too.
+ _pageToRouteMatchBase.remove(page);
+ }
+ return didPop;
}
https://github.com/flutter/packages/blob/86d15a65d091a3a23db9d549e5ce289bb687ef19/packages/go_router/lib/src/builder.dart#L413-L417
Unfortunately, this is not enough because in the case of the mentioned test, the go router tree is :
GoRouter(
initialLocation: 'a/b/c'
routes: [
GoRoute(
path: '/',
routes: [
ShellRoute(
path: 'a',
routes: [
GoRoute(
path: 'b',
routes: [
GoRoute(path: 'c'),
],
),
],
),
],
),
],
)
The test uses popUntil to pop twice.
During the 1st pop, the _handlePopPage called is the one from the nested navigator of the ShellRoute.
During the 2nd pop, the _handlePopPage called is the one from the root navigator of the ShellRoute (not the same as the first pop). So proactively update the _pageToRouteMatchBase without going through build should actually be done by all the parent _CustomNavigatorState of the one that is actually handling the pop.