[go_router] Add maybePop for safe navigation pops#11927
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Introduce maybePop on GoRouter, GoRouterDelegate, and GoRouterHelper to mirror Navigator.maybePop and BackButton while calling restore() when a pop completes synchronously. Adds comprehensive tests for maybePop, restore, canPop, and mixed Navigator.pop usage.
There was a problem hiding this comment.
Code Review
This pull request introduces a maybePop method to GoRouter, GoRouterDelegate, and GoRouterHelper to support popping routes without throwing exceptions when the stack is empty, mirroring Navigator.maybePop. It also adds extensive tests covering the new method, state restoration, and integration scenarios. The review feedback suggests adding a state.mounted check within the asynchronous loop in GoRouterDelegate.maybePop to prevent potential exceptions if a navigator is unmounted during execution.
5742cef to
4160755
Compare
Skip NavigatorState instances that are disposed during the async await gap when iterating the navigator stack.
8d95ecb to
4b39b05
Compare
|
Would appreciate a review when you have time. Thanks! |
|
@muhammadkamel While waiting for full review, please take a look at the current docs for version/CHANGELOG updates; this package now uses batched release, which follows a different process for updating those PRs. |
|
Also please do not remove the pre review checklist |
9122c9a to
ee2a7dd
Compare
go_router uses batch release, so revert direct CHANGELOG.md and pubspec.yaml version updates and add a pending_changelogs entry instead.
ee2a7dd to
b0806eb
Compare
|
Could you please take another look? |
| if (!state.mounted) { | ||
| continue; | ||
| } | ||
| final bool didPop = await state.maybePop<T>(result); |
There was a problem hiding this comment.
This doesn't feel right
This will only return false if all navigators in the widget tree return false. maybePop shouldn't be this aggressive.
I actually not entirely sure what is the correct behavior here. maybePop is scoped to consult only one navigator. but in GoRouter, there is a group of navigators
There was a problem hiding this comment.
Thanks for the feedback — agreed that walking every navigator was too aggressive.
maybePop now consults only the first navigator from _findCurrentNavigators(), matching Navigator.maybePop's single-navigator scope. If that navigator has nothing to pop, we return false and do not fall through to parents (unlike popRoute, which still walks the stack for system back).
One intentional consequence under shell routes: canPop() can be true while maybePop() returns false, because canPop/pop consider any navigator that can pop, while maybePop only asks the current one. Docs and tests now call that out explicitly — please let me know if you'd rather have maybePop follow pop()'s multi-navigator selection instead.
Address review feedback by stopping fall-through across nested navigators, documenting the canPop/pop asymmetry, and covering both shell cases in tests.
Summary
maybePoptoGoRouter,GoRouterDelegate, andGoRouterHelper(context.maybePop()), mirroringNavigator.maybePopand MaterialBackButtonbehavior.pop(),maybePop()returnsfalseinstead of throwing when nothing can be popped.maybePop()callsrestore()to keeprouteInformationProviderin sync withGoRouterDelegate.currentConfiguration(same aspop()).maybePop,restore(),canPop, mixedNavigator.pop/context.popflows, and frameworkBackButtonintegration./coverage/ingo_router.gitignore.Fixes flutter/flutter#115997
Motivation
Apps commonly gate back navigation with
if (context.canPop()) context.pop(). Framework widgets likeBackButtonuseNavigator.maybePop(), which is safe but does not sync GoRouter URL/state.maybePop()provides a single GoRouter-native safe pop API:Test plan
flutter test packages/go_router/test/maybe_pop_test.dartflutter test packages/go_router/test/restore_test.dartflutter test packages/go_router/test/extension_test.dartflutter test packages/go_router/test/delegate_test.dartflutter test packages/go_router/test/router_test.dartPre-Review Checklist
[shared_preferences]///).Footnotes
go_router uses batched release; the changelog entry is in
pending_changelogs/add_maybe_pop.yamlinstead of editingCHANGELOG.mdorpubspec.yamldirectly. ↩ ↩2