Skip to content
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

Jump to bottom

[go_router] Use the correct configuration to build the state passed to the onExit #6623

Merged
merged 6 commits into from
May 14, 2024
Merged

[go_router] Use the correct configuration to build the state passed to the onExit #6623

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c75b949
fix: Use correct config for the on exit
ValentinVignal Apr 29, 2024
4a9dfd1
test: Add tests around state and on exit
ValentinVignal Apr 29, 2024
7aee926
docs: Update version number
ValentinVignal Apr 29, 2024
743c99b
Merge remote-tracking branch 'upstream/main' into go-router/use-corre…
ValentinVignal Apr 30, 2024
560d3f4
refactor: Remove commented code
ValentinVignal Apr 30, 2024
bb8ea59
Merge remote-tracking branch 'upstream/main' into go-router/use-corre…
ValentinVignal May 13, 2024
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 @@
## 14.0.3

- Fixes correctness of the state provided in the `onExit`.

## 14.0.2

- Fixes unwanted logs when `hierarchicalLoggingEnabled` was set to `true`.
Expand Down
6 changes: 1 addition & 5 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 @@ -233,7 +233,6 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
exitingMatches.length - 1,
context: navigatorContext,
matches: exitingMatches,
configuration: configuration,
).then<void>((bool exit) {
if (!exit) {
return SynchronousFuture<void>(null);
Expand All @@ -254,7 +253,6 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
int index, {
required BuildContext context,
required List<RouteMatch> matches,
required RouteMatchList configuration,
}) {
if (index < 0) {
return SynchronousFuture<bool>(true);
Expand All @@ -266,7 +264,6 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
index - 1,
context: context,
matches: matches,
configuration: configuration,
);
}

Expand All @@ -276,15 +273,14 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
index - 1,
context: context,
matches: matches,
configuration: configuration,
);
}
return SynchronousFuture<bool>(false);
}

final FutureOr<bool> exitFuture = goRoute.onExit!(
context,
match.buildState(_configuration, configuration),
match.buildState(_configuration, currentConfiguration),
Copy link
Contributor

Choose a reason for hiding this comment

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

so the difference is whether we use the new RouteMatchList after the change vs old RouteMatchList?

I think for redirect we use the new RouteMatchList. it seems to previous implementation is more inline with the redirect API no?

Copy link
Contributor

Choose a reason for hiding this comment

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

I maybe wrong here. do you have use case that using the old routematchList is more preferred? on the other hand, I plan to redesign redirect, so maybe we should just provide both before and after?

Copy link
Contributor Author

@ValentinVignal ValentinVignal Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I found one in #6614. I was making go_router_builder use onExit in some tests failed https://github.com/flutter/packages/pull/6614/checks?check_run_id=24284539541.

The issue was when using .go(). The onExit had the state of the new page, not the previous page.

The test I wrote in this PR

'It should provide the correct path parameters to the onExit callback during a go'

fails without my changes.

On the other hand, I plan to redesign redirect, so maybe we should just provide both before and after?

Sure, do you want me to change something then?

Copy link
Contributor

Choose a reason for hiding this comment

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

I will need to redesign the onExit in the future. It also has issue with popscope and cupertinobackswipe. Thing may become a lot different after I resolve all these issues.

After thinking about it more, I think we should just keep it simple for now. I think what you have is fine.

);
if (exitFuture is bool) {
return handleOnExitResult(exitFuture);
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: 14.0.2
version: 14.0.3
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
159 changes: 158 additions & 1 deletion packages/go_router/test/on_exit_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ void main() {
expect(await router.routerDelegate.popRoute(), false);
});

testWidgets('It should provide the correct state to the onExit callback',
testWidgets('It should provide the correct uri to the onExit callback',
(WidgetTester tester) async {
final UniqueKey home = UniqueKey();
final UniqueKey page1 = UniqueKey();
Expand Down Expand Up @@ -314,4 +314,161 @@ void main() {
expect(find.byKey(home), findsOneWidget);
expect(onExitState1.uri.toString(), '/1');
});

testWidgets(
'It should provide the correct path parameters to the onExit callback',
(WidgetTester tester) async {
final UniqueKey page0 = UniqueKey();
final UniqueKey page1 = UniqueKey();
final UniqueKey page2 = UniqueKey();
final UniqueKey page3 = UniqueKey();
late final GoRouterState onExitState1;
late final GoRouterState onExitState2;
late final GoRouterState onExitState3;
final List<GoRoute> routes = <GoRoute>[
GoRoute(
path: '/route-0/:id0',
builder: (BuildContext context, GoRouterState state) =>
DummyScreen(key: page0),
),
GoRoute(
path: '/route-1/:id1',
builder: (BuildContext context, GoRouterState state) =>
DummyScreen(key: page1),
onExit: (BuildContext context, GoRouterState state) {
onExitState1 = state;
return true;
},
),
GoRoute(
path: '/route-2/:id2',
builder: (BuildContext context, GoRouterState state) =>
DummyScreen(key: page2),
onExit: (BuildContext context, GoRouterState state) {
onExitState2 = state;
return true;
},
),
GoRoute(
path: '/route-3/:id3',
builder: (BuildContext context, GoRouterState state) {
return DummyScreen(key: page3);
},
onExit: (BuildContext context, GoRouterState state) {
onExitState3 = state;
return true;
},
),
];

final GoRouter router = await createRouter(
routes,
tester,
initialLocation: '/route-0/0?param0=0',
);
unawaited(router.push('/route-1/1?param1=1'));
unawaited(router.push('/route-2/2?param2=2'));
unawaited(router.push('/route-3/3?param3=3'));

await tester.pumpAndSettle();
expect(find.byKey(page3), findsOne);

router.pop();
await tester.pumpAndSettle();
expect(find.byKey(page2), findsOne);
expect(onExitState3.uri.toString(), '/route-3/3?param3=3');
expect(onExitState3.pathParameters, const <String, String>{'id3': '3'});
expect(onExitState3.fullPath, '/route-3/:id3');

router.pop();
await tester.pumpAndSettle();
expect(find.byKey(page1), findsOne);
expect(onExitState2.uri.toString(), '/route-2/2?param2=2');
expect(onExitState2.pathParameters, const <String, String>{'id2': '2'});
expect(onExitState2.fullPath, '/route-2/:id2');

router.pop();
await tester.pumpAndSettle();
expect(find.byKey(page0), findsOne);
expect(onExitState1.uri.toString(), '/route-1/1?param1=1');
expect(onExitState1.pathParameters, const <String, String>{'id1': '1'});
expect(onExitState1.fullPath, '/route-1/:id1');
},
);

testWidgets(
'It should provide the correct path parameters to the onExit callback during a go',
(WidgetTester tester) async {
final UniqueKey page0 = UniqueKey();
final UniqueKey page1 = UniqueKey();
final UniqueKey page2 = UniqueKey();
final UniqueKey page3 = UniqueKey();
late final GoRouterState onExitState0;
late final GoRouterState onExitState1;
late final GoRouterState onExitState2;
final List<GoRoute> routes = <GoRoute>[
GoRoute(
path: '/route-0/:id0',
builder: (BuildContext context, GoRouterState state) =>
DummyScreen(key: page0),
onExit: (BuildContext context, GoRouterState state) {
onExitState0 = state;
return true;
},
),
GoRoute(
path: '/route-1/:id1',
builder: (BuildContext context, GoRouterState state) =>
DummyScreen(key: page1),
onExit: (BuildContext context, GoRouterState state) {
onExitState1 = state;
return true;
},
),
GoRoute(
path: '/route-2/:id2',
builder: (BuildContext context, GoRouterState state) =>
DummyScreen(key: page2),
onExit: (BuildContext context, GoRouterState state) {
onExitState2 = state;
return true;
},
),
GoRoute(
path: '/route-3/:id3',
builder: (BuildContext context, GoRouterState state) {
return DummyScreen(key: page3);
},
),
];

final GoRouter router = await createRouter(
routes,
tester,
initialLocation: '/route-0/0?param0=0',
);
expect(find.byKey(page0), findsOne);

router.go('/route-1/1?param1=1');
await tester.pumpAndSettle();
expect(find.byKey(page1), findsOne);
expect(onExitState0.uri.toString(), '/route-0/0?param0=0');
expect(onExitState0.pathParameters, const <String, String>{'id0': '0'});
expect(onExitState0.fullPath, '/route-0/:id0');

router.go('/route-2/2?param2=2');
await tester.pumpAndSettle();
expect(find.byKey(page2), findsOne);
expect(onExitState1.uri.toString(), '/route-1/1?param1=1');
expect(onExitState1.pathParameters, const <String, String>{'id1': '1'});
expect(onExitState1.fullPath, '/route-1/:id1');

router.go('/route-3/3?param3=3');
await tester.pumpAndSettle();
expect(find.byKey(page3), findsOne);
expect(onExitState2.uri.toString(), '/route-2/2?param2=2');
expect(onExitState2.pathParameters, const <String, String>{'id2': '2'});
expect(onExitState2.fullPath, '/route-2/:id2');
},
);
}