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] Implement popUntil #2728

Closed
wants to merge 11 commits into from
Closed

[go_router] Implement popUntil #2728

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4a809f3
:sparkles: Add popUntil
ValentinVignal Oct 22, 2022
0047433
:white_check_mark: Test popUntil
ValentinVignal Oct 22, 2022
4196431
:memo: Update the documentation
ValentinVignal Oct 22, 2022
701d26d
Merge branch 'main' into go-router/pop-until
ValentinVignal Nov 21, 2022
ab21137
Merge remote-tracking branch 'upstream/main' into go-router/pop-until
ValentinVignal Dec 16, 2022
f9e08c2
:recycle: Call pop repeatedly
ValentinVignal Dec 16, 2022
51395cd
:memo: Update the documentation
ValentinVignal Dec 16, 2022
43ec575
:white_check_mark: Update the tests
ValentinVignal Dec 16, 2022
97cbf4e
Merge branch 'main' into go-router/pop-until
ValentinVignal Dec 24, 2022
59b89cd
Merge remote-tracking branch 'upstream/main' into go-router/pop-until
ValentinVignal Feb 7, 2023
2c48f39
:alembic: Use navigator pop until
ValentinVignal Feb 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/go_router/CHANGELOG.md
View file
Open in desktop
@@ -1,3 +1,7 @@
## 5.2.0

- Adds `popUntil` method to `GoRouterDelegate`, `GoRouter` and `GoRouterHelper`.

## 5.1.10

- Fixes link of ShellRoute in README.
Expand Down
12 changes: 12 additions & 0 deletions packages/go_router/lib/src/delegate.dart
View file
Open in desktop
Expand Up @@ -166,6 +166,18 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
notifyListeners();
}

/// Calls [pop] repeatedly until the predicate returns true.
void popUntil(bool Function(RouteMatch) predicate) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can just use go to build up to a certain routematch list? If we do refactor the imperative API this will be obsolete

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean?
Are you suggesting that instead of calling popUntil (as a dev using go_router), I should call go?

I don't think it would do the same right?

If I understood correctly, go is kind of a replaceAll, after using go, there is no page to pop and canPop is false.
While popUntil will allow you to only pop some pages but not all. The use case is when the user finishes a wizard to set up something, you want to remove all the wizard's pages from the history but still keep the history the user accumulated before starting the wizard.

go wouldn't work in this scenario, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

no I meant it seems like the go can replace popUntil. Since developer creates their own route table, they will be able to use go to achieve functional equivalence of popUntil. For example if they have route table like this

GoRouteA(
  routes: [
    GoRouteB(
      routes: [GoRouteC()]
    )
  ],
)

lets say route match list is [GoRouteA, GoRouteB, GoRouteC].
popUntil((route) => route == GoRouteB) will be the same as go(GoRouteB)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this comes back to flutter/flutter#99112 's discussion.

Here you are assuming that the match list is [GoRouteA, GoRouteB, GoRouteC] and matches perfectly the structure

GoRouteA(
  routes: [
    GoRouteB(
      routes: [GoRouteC()]
    )
  ],
)

And pop from GoRouteC always goes "back" to GoRouteB.

But as I described in flutter/flutter#99112 (comment), this is not what users are usually expecting. They don't always access GoRouteC from GoRouteB. So always "going back" to GoRouteB from GoRouteC doesn't work.
The way I understand it is that you are trying to merge/combine "app location" and "history"? But that's 2 different things that should be independent.

Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

no, you are right, the previous comment is under the assuming that the match list matches the structure. I am assuming history means the internal route stack, not the browser history. In that case, my proposal user can still modify the history through imperative API, it just that they won't be able to change app location with imperative API. Let's continue the discussion in the issue.

bool hasPopped = false;
while (!predicate(_matchList.last)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

things has changed a bit, its probably the best to call pop directly since it will remove any pageless route that are on top such as dialog.

Copy link

@AceChen1 AceChen1 Dec 9, 2022 •
edited

Choose a reason for hiding this comment

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

@chunhtai It's hard to use go router.... Us project previous use more ‘pop with value on pages’, now have the requirement need do the web compatible, but go router no this feature to provide, then use 'Go_Router_Flow' lib, but looks like Pop function have been changed....

Copy link
Contributor

Choose a reason for hiding this comment

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

No I mean the implementation of popUntil should use pop method. I am on board with implementing popUntil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chunhtai I am calling pop in ♻️ Call pop repeatedly

hasPopped = true;
_matchList.pop();
}
if (hasPopped) {
notifyListeners();
}
}

/// Replaces the top-most page of the page stack with the given one.
///
/// See also:
Expand Down
5 changes: 5 additions & 0 deletions packages/go_router/lib/src/misc/extensions.dart
View file
Open in desktop
Expand Up @@ -4,6 +4,7 @@

import 'package:flutter/widgets.dart';

import '../match.dart';
import '../router.dart';

/// Dart extension to add navigation function to a BuildContext object, e.g.
Expand Down Expand Up @@ -61,6 +62,10 @@ extension GoRouterHelper on BuildContext {
/// [Navigator.pop].
void pop() => GoRouter.of(this).pop();

/// Calls [pop] repeatedly until the predicate returns true.
void popUntil(bool Function(RouteMatch) predicate) =>
GoRouter.of(this).popUntil(predicate);

/// Replaces the top-most page of the page stack with the given URL location
/// w/ optional query parameters, e.g. `/family/f2/person/p1?color=blue`.
///
Expand Down
6 changes: 6 additions & 0 deletions packages/go_router/lib/src/router.dart
View file
Open in desktop
Expand Up @@ -8,6 +8,7 @@ import 'configuration.dart';
import 'delegate.dart';
import 'information_provider.dart';
import 'logging.dart';
import 'match.dart';
import 'matching.dart';
import 'misc/inherited_router.dart';
import 'parser.dart';
Expand Down Expand Up @@ -271,6 +272,11 @@ class GoRouter extends ChangeNotifier implements RouterConfig<RouteMatchList> {
_routerDelegate.pop();
}

/// Calls [pop] repeatedly until the predicate returns true.
void popUntil(bool Function(RouteMatch) predicate) {
_routerDelegate.popUntil(predicate);
}

/// Refresh the route.
void refresh() {
assert(() {
Expand Down
2 changes: 1 addition & 1 deletion packages/go_router/pubspec.yaml
View file
Open in desktop
@@ -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: 5.1.10
version: 5.2.0
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
21 changes: 21 additions & 0 deletions packages/go_router/test/delegate_test.dart
View file
Open in desktop
Expand Up @@ -56,6 +56,27 @@ void main() {
});
});

group('popUntil', () {
testWidgets('It should pop 2 pages', (WidgetTester tester) async {
final GoRouter goRouter = await createGoRouter(tester)
..push('/a')
..push('/a');

await tester.pumpAndSettle();
expect(goRouter.routerDelegate.matches.matches.length, 3);
goRouter.routerDelegate.addListener(expectAsync0(() {}));
final RouteMatch second = goRouter.routerDelegate.matches.matches[1];
final RouteMatch last = goRouter.routerDelegate.matches.matches.last;

int count = 0;
goRouter.popUntil((RouteMatch routeMatch) => count++ >= 2);

expect(goRouter.routerDelegate.matches.matches.length, 1);
expect(goRouter.routerDelegate.matches.matches.contains(second), false);
expect(goRouter.routerDelegate.matches.matches.contains(last), false);
});
});

group('push', () {
testWidgets(
'It should return different pageKey when push is called',
Expand Down