[go_router] Refactored RouteMatchList and imperative APIs #5497

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


-  /// Returns the top-level pages instead of the root navigator. Used for
-  /// testing.
-  List<Page<Object?>> _buildPages(


most of the recursive build pages logic is moved to _CustomNavigatorState

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

packages/go_router/lib/src/delegate.dart

@@ -73,25 +79,36 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>

  /// Returns `true` if the active Navigator can pop.
  bool canPop() {
-    final _NavigatorStateIterator iterator = _createNavigatorStateIterator();


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


  /// Generate a [RouteMatch] object by matching the `route` with
  /// `remainingLocation`.
  ///
  /// The extracted path parameters, as the result of the matching, are stored
  /// into `pathParameters`.
-  static RouteMatch? match({
+  static List<RouteMatchBase> match({


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


    if (builder == null) {
      return null;
    }
-    return buildPage(context, state, Builder(builder: (BuildContext context) {
+    return _buildPlatformAdapterPage(context, state,
+        Builder(builder: (BuildContext context) {
      return builder(context, state);
    }));
  }

  /// Builds a [Page] for [ShellRouteBase]


nit : ShellRouteMatch

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

packages/go_router/lib/src/builder.dart

+    return _buildPlatformAdapterPage(context, state,
+        Builder(builder: (BuildContext context) {
+      return match.route.buildWidget(context, state, shellRouteContext)!;
+    }));


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

packages/go_router/lib/src/match.dart

+  GoRouterState buildState(
+      RouteConfiguration configuration, RouteMatchList matches);
+
+  /// Generate a list [RouteMatchBase] objects by matching the `route` and its


nit: 'Generates a list of'

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

packages/go_router/lib/src/match.dart

+    }
+    return copyWith(
+        matches: _createNewMatchUntilIncompatible(
+            matches, match.matches.matches, match));


packages/go_router/lib/src/match.dart

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

packages/go_router/lib/src/match.dart

+  /// The last leaf route.
+  ///
+  /// If the last RouteMatchBase from [matches] is a ShellRouteMatch, it
+  /// cursively goes into its [ShellRouteMatch.matches] until it reach the leaf


nit: recursively

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

packages/go_router/lib/src/match.dart

+  /// order is removed.
+  static List<RouteMatchBase> _removeRouteMatchFromList(
+      List<RouteMatchBase> matches, RouteMatchBase target) {
+    // Remove is usually caused by pop; therefore, start searching from the end.


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

+  @override
+  final ShellRouteBase route;
+
+  RouteMatch get _leafMatch {


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

+  @override
+  void didChangeDependencies() {
+    super.didChangeDependencies();
+    // Return a HeroController based on the app type.


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

+      }
+    }
+    if (subRouteMatches?.isEmpty ?? true) {
+      // If not finding a sub route match, it is consider not matched for this


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 54558023+keyonghan@users.noreply.github.com Disable `presubmit: false` targets for recipes CQ (flutter/packages#5735) 2023-12-21 47866232+chunhtai@users.noreply.github.com [go_router] Refactored RouteMatchList and imperative APIs (flutter/packages#5497) 2023-12-21 54558023+keyonghan@users.noreply.github.com Add env_variables in ci.yaml (flutter/packages#5730) 2023-12-21 engine-flutter-autoroll@skia.org Roll Flutter (stable) from 2e9cb0a to 78666c8 (1 revision) (flutter/packages#5734) 2023-12-21 engine-flutter-autoroll@skia.org 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 flutter-ecosystem@google.com,rmistry@google.com 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

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

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 54558023+keyonghan@users.noreply.github.com Disable `presubmit: false` targets for recipes CQ (flutter/packages#5735) 2023-12-21 47866232+chunhtai@users.noreply.github.com [go_router] Refactored RouteMatchList and imperative APIs (flutter/packages#5497) 2023-12-21 54558023+keyonghan@users.noreply.github.com Add env_variables in ci.yaml (flutter/packages#5730) 2023-12-21 engine-flutter-autoroll@skia.org Roll Flutter (stable) from 2e9cb0a to 78666c8 (1 revision) (flutter/packages#5734) 2023-12-21 engine-flutter-autoroll@skia.org 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 flutter-ecosystem@google.com,rmistry@google.com 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

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

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
80 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[go_router] Refactored RouteMatchList and imperative APIs #5497

[go_router] Refactored RouteMatchList and imperative APIs #5497

chunhtai commented Nov 27, 2023 •

edited

Loading

flutter-dashboard bot commented Nov 27, 2023

chunhtai Dec 7, 2023

chunhtai Dec 7, 2023

chunhtai Dec 7, 2023

hannah-hyj Dec 11, 2023

hannah-hyj Dec 11, 2023

hannah-hyj Dec 11, 2023

hannah-hyj Dec 11, 2023

hannah-hyj Dec 11, 2023

hannah-hyj Dec 12, 2023

chunhtai Dec 14, 2023

hannah-hyj Dec 12, 2023

hannah-hyj Dec 12, 2023

chunhtai Dec 14, 2023

hannah-hyj Dec 12, 2023

hannah-hyj Dec 14, 2023

hannah-hyj Dec 14, 2023 •

edited

Loading

hannah-hyj Dec 14, 2023

chunhtai Dec 15, 2023

hannah-hyj Dec 14, 2023

hannah-hyj Dec 14, 2023

hannah-hyj Dec 14, 2023

hannah-hyj Dec 15, 2023

chunhtai commented Dec 19, 2023

hannah-hyj left a comment

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

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

		- Refactored `RouteMatchList` and imperative APIs.

[go_router] Refactored RouteMatchList and imperative APIs #5497

[go_router] Refactored RouteMatchList and imperative APIs #5497

Conversation

chunhtai commented Nov 27, 2023 • edited Loading

Pre-launch Checklist

flutter-dashboard bot commented Nov 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannah-hyj Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunhtai commented Dec 19, 2023

hannah-hyj left a comment

Choose a reason for hiding this comment

chunhtai commented Nov 27, 2023 •

edited

Loading

hannah-hyj Dec 14, 2023 •

edited

Loading