Skip to content

[go_router] Fixes an issue where route future does not complete when … #6596

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

Merged
merged 1 commit into from
Apr 24, 2024
Merged

[go_router] Fixes an issue where route future does not complete when … #6596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 @@
## 13.2.5

- Fixes an issue where route future does not complete when popping shell route.

## 13.2.4

- Updates examples to use uri.path instead of uri.toString() for accessing the current location.
Expand Down
8 changes: 6 additions & 2 deletions packages/go_router/lib/src/delegate.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,12 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
}

void _completeRouteMatch(Object? result, RouteMatchBase match) {
if (match is ImperativeRouteMatch) {
match.complete(result);
RouteMatchBase walker = match;
Copy link
Member

Choose a reason for hiding this comment

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

i think it's more clear to walk the tree in _handlePopPage instead of _completeRouteMatch

Copy link
Member

Choose a reason for hiding this comment

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

it's odd that the match here is not a final match, you still have to walk the tree.

but if you walk it right after assigning the match, it looks more clear?

 bool _handlePopPage(Route<Object?> route, Object? result) {
    final Page<Object?> page = route.settings as Page<Object?>;
    final RouteMatchBase match = _pageToRouteMatchBase[page]!;
+++
walk the tree 
+++
    return widget.onPopPageWithRouteMatch(route, result, match);
  }
``

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The match is the correct match we want to remove, but it can be a shell route match, which means we want to remove the entire branch of that shell route match. Also in this case it should only have one leaf node, otherwise the pop would not remove the shell route match in the first place.

The problem is that the completer is stored in the leaf node of that shell route match, that's why i have to find the leave node here.

I think put it in here is correct, we do want to complete and remove the entire shell route branch. as of how we complete this branch is the implementation detail in this method

Copy link
Member

Choose a reason for hiding this comment

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

thanks for the explanation!

while (walker is ShellRouteMatch) {
walker = walker.matches.last;
}
if (walker is ImperativeRouteMatch) {
walker.complete(result);
}
currentConfiguration = currentConfiguration.remove(match);
notifyListeners();
Expand Down
2 changes: 1 addition & 1 deletion packages/go_router/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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: 13.2.4
version: 13.2.5
repository: https://github.com/flutter/packages/tree/main/packages/go_router
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+go_router%22

Expand Down
47 changes: 47 additions & 0 deletions packages/go_router/test/go_router_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3462,6 +3462,53 @@ void main() {
expect(find.text('Screen B'), findsOneWidget);
});

testWidgets('can complete leaf route', (WidgetTester tester) async {
Future<bool?>? routeFuture;
final List<RouteBase> routes = <RouteBase>[
GoRoute(
path: '/',
builder: (BuildContext context, GoRouterState state) {
return Scaffold(
body: TextButton(
onPressed: () async {
routeFuture = context.push<bool>('/a');
},
child: const Text('press'),
),
);
},
),
ShellRoute(
builder: (BuildContext context, GoRouterState state, Widget child) {
return Scaffold(
body: child,
);
},
routes: <RouteBase>[
GoRoute(
path: '/a',
builder: (BuildContext context, GoRouterState state) {
return const Scaffold(
body: Text('Screen A'),
);
},
),
],
),
];

final GoRouter router = await createRouter(routes, tester);
expect(find.text('press'), findsOneWidget);

await tester.tap(find.text('press'));
await tester.pumpAndSettle();
expect(find.text('Screen A'), findsOneWidget);

router.pop<bool>(true);
final bool? result = await routeFuture;
expect(result, isTrue);
});

testWidgets(
'Pops from the correct Navigator when the Android back button is pressed',
(WidgetTester tester) async {
Expand Down