[go_router] Refactored RouteMatchList and imperative APIs by chunhtai · Pull Request #5497 · flutter/packages

chunhtai · 2023-11-27T22:55:39Z

This pr refactor RouteMatchList to be a tree structure.

Added a common base class RouteMatchBase. It is extended by both RouteMatch and ShellRouteMatch.

The RouteMatch is for GoRoute, and is always a leaf node

The ShellRouteMatch is for ShellRouteBase, and is always and intermediate node with a list of child RouteMatchBase[s].

This pr also redo how push is processed. Will add a doc explain this shortly.

This is a breaking change, will write a migration guide soon.

fixes flutter/flutter#134524
fixes flutter/flutter#130406
fixes flutter/flutter#126365
fixes flutter/flutter#125752
fixes flutter/flutter#120791
fixes flutter/flutter#120665
fixes flutter/flutter#113001
fixes flutter/flutter#110512

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2023-11-27T22:55:42Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

chunhtai · 2023-12-07T19:09:24Z

packages/go_router/lib/src/builder.dart

most of the recursive build pages logic is moved to _CustomNavigatorState

chunhtai · 2023-12-07T19:12:01Z

packages/go_router/lib/src/delegate.dart

previously we need to scan the list and use parent navigator key to jump around the list. Therefore i created this iterator to abstract away the logic.

After the refactor we just need to look at the last node in the RouteMatchList and walk up the tree branch.

chunhtai · 2023-12-07T19:13:51Z

packages/go_router/lib/src/match.dart

This is the most complex part in this pr, previously match just produce a RouteMatch for the input route.

After this change, this match will recursively match the subroute in route and return the entire route matches if it finds the full match.

hannah-hyj · 2023-12-11T19:22:10Z

packages/go_router/lib/src/builder.dart

nit : ShellRouteMatch

hannah-hyj · 2023-12-11T19:24:53Z

packages/go_router/lib/src/builder.dart

hannah-hyj · 2023-12-11T21:28:11Z

packages/go_router/lib/src/match.dart

nit: 'Generates a list of'

hannah-hyj · 2023-12-11T22:46:43Z

packages/go_router/lib/src/match.dart

hannah-hyj · 2023-12-11T23:41:58Z

packages/go_router/lib/src/match.dart

nit: recursively

hannah-hyj · 2023-12-12T00:22:17Z

packages/go_router/lib/src/match.dart

Remove is usually caused by pop

by usually do you mean there are other cases?

this function looks like it assumes all removal is pop. it not only removes the target, but also remove all matches after the target.

updated wording

hannah-hyj · 2023-12-12T00:25:18Z

packages/go_router/lib/src/match.dart

I found the name leafMatch a bit confusing cause a tree can have many leaves. Maybe lastLeaf is more clear?

hannah-hyj · 2023-12-12T00:26:50Z

packages/go_router/lib/src/builder.dart

nit: returns

gramma is correct here since I am describing a fact, but the return may not be a good word here. will update

hannah-hyj · 2023-12-12T00:28:25Z

packages/go_router/lib/src/match.dart

nit: considered

hannah-hyj · 2023-12-14T22:31:27Z

packages/go_router/lib/src/builder.dart

+          configuration: configuration,
+          errorBuilder: errorBuilder,
+          errorPageBuilder: errorPageBuilder,
+        ));


hannah-hyj · 2023-12-14T22:33:32Z

packages/go_router/lib/src/builder.dart

+    required this.navigatorRestorationId,
+    required this.onPopPageWithRouteMatch,
+    required this.matchList,
+    required this.matches,


why pass both matchList and matches? Can you add some comments?

packages/go_router/lib/src/match.dart

hannah-hyj · 2023-12-14T22:59:44Z

packages/go_router/lib/src/builder.dart

+      registry: _registry,
+      child: HeroControllerScope(
+        controller: _controller!,
+        child: Builder(


Builder() here seems like unnecessary. context is not used.

hannah-hyj · 2023-12-14T23:22:26Z

packages/go_router/lib/src/match.dart

+    // Grabs the route matches for the scope navigator key and put it into the
+    // matches for `null`.
+    if (result.containsKey(scopedNavigatorKey)) {
+      final List<RouteMatchBase> matchForScopedNavigator =


nit matchesForScopedNavigator

hannah-hyj · 2023-12-14T23:26:41Z

packages/go_router/lib/src/match.dart

+      );
+    } else {
+      assert(false, 'Unexpected route type: $route');
+      return const <GlobalKey<NavigatorState>?, List<RouteMatchBase>>{};


Suggested change

return const <GlobalKey<NavigatorState>?, List<RouteMatchBase>>{};

return const _empty;

packages/go_router/lib/src/match.dart

hannah-hyj · 2023-12-14T23:45:14Z

packages/go_router/lib/src/match.dart

+    required Uri uri,
+  }) {
+    final GlobalKey<NavigatorState>? parentKey =
+        route.parentNavigatorKey == null ||


hannah-hyj · 2023-12-15T00:40:08Z

packages/go_router/CHANGELOG.md

@@ -1,3 +1,10 @@
+## 13.0.0
+
+- Refactored `RouteMatchList` and imperative APIs.


nit: Refactors

Migration guide for flutter/packages#5497

chunhtai · 2023-12-19T00:02:26Z

cc @ValentinVignal

hannah-hyj

LGTM

…utter/packages#5497)

flutter/packages@4c3bc49...23d2d9c 2023-12-22 49699333+dependabot[bot]@users.noreply.github.com Bump lewagon/wait-on-check-action from 1.3.1 to 1.3.3 (flutter/packages#5737) 2023-12-21 [email protected] Disable `presubmit: false` targets for recipes CQ (flutter/packages#5735) 2023-12-21 [email protected] [go_router] Refactored RouteMatchList and imperative APIs (flutter/packages#5497) 2023-12-21 [email protected] Add env_variables in ci.yaml (flutter/packages#5730) 2023-12-21 [email protected] Roll Flutter (stable) from 2e9cb0a to 78666c8 (1 revision) (flutter/packages#5734) 2023-12-21 [email protected] Roll Flutter from da0cd69 to 11def8e (16 revisions) (flutter/packages#5732) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

chunhtai marked this pull request as draft November 27, 2023 22:55

github-actions bot added the p: go_router label Nov 27, 2023

chunhtai force-pushed the route-match-tree branch 2 times, most recently from 77e95ca to 109bdfc Compare December 6, 2023 23:02

chunhtai changed the title ~~WIP route match tree~~ [go_router] Refactored RouteMatchList and imperative APIs Dec 7, 2023

chunhtai commented Dec 7, 2023

View reviewed changes

chunhtai marked this pull request as ready for review December 7, 2023 19:36

chunhtai requested a review from hannah-hyj December 7, 2023 19:36

This was referenced Dec 7, 2023

[go-router] child page transition animation doesn't play when placed inside ShellRoute flutter/flutter#139471

Open

[go_router] Path and query parameters are not preserved in StatefulShellRoute flutter/flutter#129878

Closed

hannah-hyj reviewed Dec 12, 2023

View reviewed changes

chunhtai added 2 commits December 14, 2023 10:00

WIP route match tree

2bc4d5c

addressing comments

0e058a5

chunhtai force-pushed the route-match-tree branch from 73d0cd2 to 0e058a5 Compare December 14, 2023 22:01

update

5d4578d

chunhtai requested a review from hannah-hyj December 14, 2023 22:12

hannah-hyj reviewed Dec 14, 2023

View reviewed changes

hannah-hyj reviewed Dec 15, 2023

View reviewed changes

chunhtai mentioned this pull request Dec 15, 2023

Add /go/go-router-v13-breaking-changes redirect flutter/website#9934

Merged

3 tasks

parlough pushed a commit to flutter/website that referenced this pull request Dec 15, 2023

Add /go/go-router-v13-breaking-changes redirect (#9934)

5a8dcb0

Migration guide for flutter/packages#5497

chunhtai added 4 commits December 15, 2023 12:42

update

9884c28

fix analyze

6cb708c

add more doc and example

2a78266

update

7743c27

chunhtai requested a review from hannah-hyj December 18, 2023 23:53

update

f341ff0

hannah-hyj approved these changes Dec 20, 2023

View reviewed changes

update

87b8bfc

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 21, 2023

auto-submit bot merged commit 6660212 into flutter:main Dec 21, 2023

engine-flutter-autoroll mentioned this pull request Dec 22, 2023

Roll Packages from 4c3bc496bed2 to 23d2d9c01366 (6 revisions) flutter/flutter#140546

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 22, 2023

6660212 [go_router] Refactored RouteMatchList and imperative APIs (fl…

919add9

…utter/packages#5497)

engine-flutter-autoroll mentioned this pull request Dec 22, 2023

Roll Packages from 4c3bc496bed2 to 23d2d9c01366 (6 revisions) flutter/flutter#140550

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 22, 2023

6660212 [go_router] Refactored RouteMatchList and imperative APIs (fl…

0b10e40

…utter/packages#5497)

reidbaker mentioned this pull request Jan 12, 2024

Dialog Debug #5881

Closed

reidbaker mentioned this pull request Jan 19, 2024

[in_app_purchase] Add play country code api #5941

Merged

12 tasks

reidbaker mentioned this pull request Feb 19, 2024

[in_app_purchase_android] Add UserChoiceBilling mode. #6162

Merged

16 tasks

hannah-hyj mentioned this pull request Mar 15, 2024

[go_router]: fix GoRouter.optionURLReflectsImperativeAPIs flag works with new imperative APIs #6236

Merged

11 tasks

	return const <GlobalKey<NavigatorState>?, List<RouteMatchBase>>{};
	return const _empty;

		@@ -1,3 +1,10 @@
		## 13.0.0

		- Refactored `RouteMatchList` and imperative APIs.

Conversation

chunhtai commented Nov 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Nov 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hannah-hyj Dec 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Dec 19, 2023

Uh oh!

hannah-hyj left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chunhtai commented Nov 27, 2023 •

edited

Loading

hannah-hyj Dec 14, 2023 •

edited

Loading