[go_router] Add support for preloading branches of StatefulShellRoute (revised solution) #6467

tolo · 2024-04-05T16:08:20Z

Adds support for preloading branches in a StatefulShellRoute. This functionality was initially part of an early implementation of #2650, however it was decided to implement this in a separate PR. The current implementation is a rewrite of the original implementation to better fit the final version of StatefulShellRoute (and go_router in general).

NOTE: this is a revised version of the initial solution (see #4251), containing a substantially simpler implementation made possible thanks to recent refactoring in go_router.

This fixes issue flutter/flutter#127804.

Pre-launch Checklist

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

…nto statefulshellroute-preload-v2

chunhtai · 2024-04-29T22:24:03Z

I know I have been slacking off at this pr. I am sorry about this. My schedule is quite tight after I came back from vacation. This is still under my radar. I will try find time review this pr within 2 weeks

tolo · 2024-04-30T12:26:09Z

I know I have been slacking off at this pr. I am sorry about this. My schedule is quite tight after I came back from vacation. This is still under my radar. I will try find time review this pr within 2 weeks

No worries @chunhtai, I completely understand, have had far too much on my plate myself the last year 😅

cedvdb · 2024-05-22T10:22:57Z

packages/go_router/lib/src/route.dart

+ return true;
+ });
+
+ final Widget navigator = widget.shellRouteContext.navigatorBuilder(


might want to null check match here

Good catch, will update this.

cedvdb · 2024-05-22T10:24:03Z

packages/go_router/lib/src/route.dart

+ RouteMatchList matchList,
+ List<NavigatorObserver>? observers,
+ String? restorationScopeId);
+// typedef NavigatorBuilder = Widget Function(


seems like this commented code can be removed

cedvdb · 2024-05-22T10:24:38Z

packages/go_router/example/lib/stateful_shell_route.dart

@@ -60,6 60,9 @@ class NestedTabNavigationExampleApp extends StatelessWidget {
 ],
 ),
 ],
+ // To enable preloading of the initial locations of branches, pass
+ // 'true' for the parameter preload.
+ // preload: true,


I'd find it clearer if the last line was uncommented as preload: false

You may be right, I'll consider this.

cedvdb · 2024-05-22T10:28:57Z

packages/go_router/lib/src/route.dart

+ /// *Note:* The primary purpose of branch preloading is to enhance the user
+ /// experience when switching branches, which might for instance involve
+ /// preparing the UI for animated transitions etc. Care must be taken to
+ /// **keep the preloading to an absolute minimum** to avoid any unnecessary


The position here is overly dramatic imo. I'd change

Care must be taken to **keep the preloading to an absolute minimum** to avoid any unnecessary resource use.

into

Preloading involves resource usage for elements that are not displayed, as such this should be used sparingly.

or something like that.

Personally I'm going to use that for every top level branch, which I have 5 of. That's not the absolute minimum. Anyway, I just find the wording too strong and might scare some people from using an otherwise useful feature. People can benchmark the effect.

Great feedback. This wording was actually due to some rather lengthy discussion in the original PR around the potential negative aspects of preloading, as I recall it. There are always dangers of premature/unnecessary optimisation when exposing these kinds of APIs, so some level of cautionary language is certainly justified. But maybe it can be dialed back a bit.

unacorbatanegra · 2024-05-30T14:44:00Z

Any updates on this?

chunhtai

Overall LGTM, just some comments

chunhtai · 2024-06-07T20:38:09Z

packages/go_router/lib/src/route.dart

@@ -996,6 1011,7 @@ class StatefulShellBranch {
 this.initialLocation,
 this.restorationScopeId,
 this.observers,
+ this.preload = false,


should we check for initiallocation is set if preload is true?

Don't think that should be necessary, as it will fall back on the defaultRoute if initialLocation isn't provided.

chunhtai · 2024-06-07T20:38:14Z

packages/go_router/lib/src/route.dart

@@ -1032,6 1048,21 @@ class StatefulShellBranch {
 /// The observers parameter is used by the [Navigator] built for this branch.
 final List<NavigatorObserver>? observers;

+ /// Whether this route branch should be loaded only when navigating to it for


Can you inverse the statement? from reading it, it seem to not preload when this property is true

Right, forgot to change that. How about this: "Whether this route branch should be eagerly loaded when navigating to it for the first time..."?

No, that's also confusing, it's not preloading when accessing the branch for the first time but when accessing the shell.

"Whether this "StatefulShellBranch" should be preloaded once the "StatefulShellRoute" has been accessed."

Ah, but of course, how about this:
"Whether this route branch should be eagerly loaded when navigating to the associated StatefulShellRoute for the first time."

Yes that's much better

yes sounds good to me, can you reflect that to the doc string?

chunhtai · 2024-06-07T20:42:04Z

packages/go_router/lib/src/route.dart

@@ -1255,8 1291,39 @@ class StatefulNavigationShellState extends State<StatefulNavigationShell>
 final bool locationChanged =
 previousBranchLocation != currentBranchLocation;
 if (locationChanged || !hasExistingNavigator) {
- _branchNavigators[branch.navigatorKey] = shellRouteContext
- .navigatorBuilder(branch.observers, branch.restorationScopeId);
+ _branchNavigators[branch.navigatorKey] =


just realized somewhere in this method should probably remove entries in _branchNavigators the are no longer in the StatefulShellRoute?

You're right, that is definitely necessary to do now. Will have a look at it.

@chuntai, I added a fix for removing old entries, but when attempting to write a test case, I ran into issues with duplicate GlobalKeys (_shellStateKey) when updating the dynamic RoutingConfig. Or - I should be clear - at first the test case worked, but not because of the code I added, but instead because the entire StatefulShellRoute was replaced/reloaded. But when I added the possibility to set the GlobalKey<StatefulNavigationShellState> _shellStateKey to maintain state, I started to get this error.

When trying to troubleshoot it, I started to wonder whether dynamic RoutingConfig full supports ShellRoutes, at least with state. Added some test cases to test this, and it seems simple StatefulWidgets in a simple routing scenario works, but when adding a (regular) ShellRoute with a stateful shell I also get and issue with duplicate GlobalKeys.

Not sure if I just missed something, but I anyway added a gist with the test cases here: https://gist.github.com/tolo/922fd838cdea30b17121533b0012eda4

can you file an issue for it? looks like a bug

is the fix in this pr already?

cedvdb · 2024-07-03T12:10:40Z

What's the status here ? It seems like you said that you have a test locally that's failing but it's unrelated to the changes here ?

tolo · 2024-07-10T06:58:33Z

What's the status here ? It seems like you said that you have a test locally that's failing but it's unrelated to the changes here ?

Yes, @chunhtai, have you had a chance to too look at my comment above (#6467 (comment))?

The issue is however only with testing if obsolete branch navigators are removed (which is done dynamic RoutingConfig), so possibly this could be moved to a separate issue/PR. And another issue should probably be opened for fixing the support for dynamic RoutingConfig for (stateful) shell routes. What do you think @chunhtai?

chunhtai · 2024-07-11T21:37:42Z

will take a look this week

chunhtai · 2024-07-16T17:06:57Z

packages/go_router/example/lib/others/custom_stateful_shell_route.dart

 routes: <RouteBase>[
- StatefulShellRoute(
+ StatefulShellRoute.indexedStack(


why changing this example? In stead of adding more complexity to this example, we should probably create a new example that target specific feature

chunhtai · 2024-07-16T17:22:59Z

packages/go_router/lib/src/route.dart

@@ -1032,6 1048,21 @@ class StatefulShellBranch {
 /// The observers parameter is used by the [Navigator] built for this branch.
 final List<NavigatorObserver>? observers;

+ /// Whether this route branch should be loaded only when navigating to it for


yes sounds good to me, can you reflect that to the doc string?

chunhtai · 2024-07-16T17:25:03Z

packages/go_router/lib/src/route.dart

@@ -1255,8 1291,39 @@ class StatefulNavigationShellState extends State<StatefulNavigationShell>
 final bool locationChanged =
 previousBranchLocation != currentBranchLocation;
 if (locationChanged || !hasExistingNavigator) {
- _branchNavigators[branch.navigatorKey] = shellRouteContext
- .navigatorBuilder(branch.observers, branch.restorationScopeId);
+ _branchNavigators[branch.navigatorKey] =


can you file an issue for it? looks like a bug

chunhtai · 2024-07-16T17:26:12Z

packages/go_router/lib/src/route.dart

@@ -1255,8 1291,39 @@ class StatefulNavigationShellState extends State<StatefulNavigationShell>
 final bool locationChanged =
 previousBranchLocation != currentBranchLocation;
 if (locationChanged || !hasExistingNavigator) {
- _branchNavigators[branch.navigatorKey] = shellRouteContext
- .navigatorBuilder(branch.observers, branch.restorationScopeId);
+ _branchNavigators[branch.navigatorKey] =


is the fix in this pr already?

tolo added 4 commits March 12, 2024 18:14

Added support for branch preloading

d4f2d0d

Added support for branch preloading

fc00590

Updated version and CHANGELOG

62b9894

Merge remote-tracking branch 'origin/statefulshellroute-preload-v2' i…

126787f

…nto statefulshellroute-preload-v2

tolo requested a review from chunhtai as a code owner April 5, 2024 16:08

github-actions bot added the p: go_router label Apr 5, 2024

tolo mentioned this pull request Apr 5, 2024

[go_router] Add support for preloading branches of StatefulShellRoute #4251

Closed

11 tasks

cedvdb reviewed May 22, 2024

View reviewed changes

tolo added 2 commits June 3, 2024 20:48

Some refinements and some updates from PR feedback.

b0a92bf

Merged from upstream/main

eb3db3b

chunhtai reviewed Jun 7, 2024

View reviewed changes

chunhtai self-requested a review June 20, 2024 21:41

riscait mentioned this pull request Jun 26, 2024

[go_router] In TabBarView with StatefulShellBranch, content remains Blank when swiping to switch between tabs flutter/flutter#150837

Open

chunhtai reviewed Jul 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[go_router] Add support for preloading branches of StatefulShellRoute (revised solution) #6467

[go_router] Add support for preloading branches of StatefulShellRoute (revised solution) #6467

tolo commented Apr 5, 2024

chunhtai commented Apr 29, 2024

tolo commented Apr 30, 2024

cedvdb May 22, 2024

tolo May 27, 2024

cedvdb May 22, 2024 •

edited

Loading

cedvdb May 22, 2024

tolo May 27, 2024

cedvdb May 22, 2024 •

edited

Loading

tolo May 27, 2024

unacorbatanegra commented May 30, 2024

chunhtai left a comment

chunhtai Jun 7, 2024

tolo Jun 10, 2024

chunhtai Jun 7, 2024

tolo Jun 10, 2024

cedvdb Jun 11, 2024

tolo Jun 15, 2024

cedvdb Jun 16, 2024

chunhtai Jul 16, 2024

chunhtai Jun 7, 2024

tolo Jun 10, 2024

tolo Jun 18, 2024

chunhtai Jul 16, 2024

chunhtai Jul 16, 2024

cedvdb commented Jul 3, 2024

tolo commented Jul 10, 2024

chunhtai commented Jul 11, 2024

chunhtai Jul 16, 2024

chunhtai Jul 16, 2024

chunhtai Jul 16, 2024

chunhtai Jul 16, 2024

[go_router] Add support for preloading branches of StatefulShellRoute (revised solution) #6467

Are you sure you want to change the base?

[go_router] Add support for preloading branches of StatefulShellRoute (revised solution) #6467

Conversation

tolo commented Apr 5, 2024

Pre-launch Checklist

chunhtai commented Apr 29, 2024

tolo commented Apr 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cedvdb May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cedvdb May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unacorbatanegra commented May 30, 2024

chunhtai left a 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

cedvdb commented Jul 3, 2024

tolo commented Jul 10, 2024

chunhtai commented Jul 11, 2024

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

cedvdb May 22, 2024 •

edited

Loading

cedvdb May 22, 2024 •

edited

Loading