[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 😅

packages/go_router/lib/src/route.dart

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.

packages/go_router/lib/src/route.dart

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

Any updates on this?

chunhtai

Overall LGTM, just some comments

packages/go_router/lib/src/route.dart

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?

It is now. 😊 Note though that I disabled two tests that fail due to the issue I mentioned above.

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

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

packages/go_router/lib/src/route.dart

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?

(Note: issues with dynamic route config)

Shunt22 · 2024-08-02T18:30:46Z

Any status update?
Would love to have this feature in my project.

…rrors).

tolo · 2024-08-19T19:17:43Z

@chunhtai, as mentioned above, I've included the fix for cleaning obsolete branches. But I'm thinking it might be better to postpone that fix until the issues with duplicate GlobalKeys is resolved.

cedvdb · 2024-08-20T06:00:27Z

packages/go_router/lib/src/route.dart

+
+        final Widget navigator = widget.shellRouteContext.navigatorBuilder(
+          branch.navigatorKey,
+          match!,


! can be removed on this line

Oh? Maybe I'm missing something, but that gives me an error (since match is ShellRouteMatch?). Would have been nice if the assert would have that effect though.

…abBarView again).

cedvdb · 2024-09-16T21:05:44Z

@chunhtai can this PR get some love ? (feedback)

tolo · 2024-09-17T19:46:18Z

FYI, for those interested in StatefulShellRoute evolution, there is a related (draft) PR in the works: #7622. There are some ideas around quality-of-life improvements around StatefulShellRoute in there, so have a look if you're interested.

chunhtai

LGTM

packages/go_router/lib/src/route.dart

…preload-v2

…ple (#7583) Updated `custom_stateful_shell_route.dart` example to better support swiping in TabView. Also added code to demonstrate use of PageView instead of TabView. Note that to be fully effective from a usability perspective, the PR #6467 (branch preloading) need also be merged. This PR addresses: * flutter/flutter#150837 * flutter/flutter#112267 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] page, which explains my responsibilities. - [x] I read and followed the [relevant style guides] and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use `dart format`.) - [x] I signed the [CLA]. - [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g. `[shared_preferences]` - [x] I [linked to at least one issue that this PR fixes] in the description above. - [x] I updated `pubspec.yaml` with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes]. - [x] I updated `CHANGELOG.md` to add a description of the change, [following repository CHANGELOG style], or this PR is [exempt from CHANGELOG changes]. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/packages/blob/main/CONTRIBUTING.md [Tree Hygiene]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md [relevant style guides]: https://github.com/flutter/packages/blob/main/CONTRIBUTING.md#style [CLA]: https://cla.developers.google.com/ [Discord]: https://github.com/flutter/flutter/blob/master/docs/contributing/Chat.md [linked to at least one issue that this PR fixes]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#overview [pub versioning philosophy]: https://dart.dev/tools/pub/versioning [exempt from version changes]: https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#version [following repository CHANGELOG style]: https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changelog-style [exempt from CHANGELOG changes]: https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changelog [test-exempt]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#tests

hannah-hyj

LGTM!

Shunt22 · 2024-10-24T11:14:27Z

Hi!
Sorry, when will this be live?

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

packages/go_router/lib/src/route.dart Show resolved Hide resolved

cedvdb reviewed May 22, 2024

View reviewed changes

packages/go_router/lib/src/route.dart Outdated Show resolved Hide resolved

cedvdb reviewed May 22, 2024

View reviewed changes

packages/go_router/lib/src/route.dart Outdated Show resolved Hide resolved

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

tolo added 2 commits July 26, 2024 17:21

Improved description of preload property.

c2eb9b1

Cleanup obsolete branches

5d93833

(Note: issues with dynamic route config)

tolo added 2 commits August 15, 2024 16:39

Added branch cleanup code and tests (Note: some are disabled due to e…

523bf75

…rrors).

Merged from upstream/main

ca3e898

cedvdb reviewed Aug 20, 2024

View reviewed changes

chunhtai self-requested a review August 22, 2024 21:39

tolo added 3 commits September 4, 2024 17:50

Merged from upstream/main

6e6c39e

Rolled back some changes in custom_stateful_shell_route.dart (using T…

d27cf3a

…abBarView again).

Small update due to API change in RouteConfiguration.findMatch.

ea48de0

tolo mentioned this pull request Sep 5, 2024

[go_router] Fixed TabView swiping in custom stateful shell route example #7583

Merged

11 tasks

chunhtai approved these changes Sep 20, 2024

View reviewed changes

packages/go_router/lib/src/route.dart Outdated Show resolved Hide resolved

packages/go_router/lib/src/route.dart Outdated Show resolved Hide resolved

tolo added 2 commits September 24, 2024 15:48

Code style cleanup.

9d68693

Merge remote-tracking branch 'upstream/main' into statefulshellroute-…

604744b

…preload-v2

chunhtai requested a review from hannah-hyj October 3, 2024 21:40

hannah-hyj approved these changes Oct 14, 2024

View reviewed changes

tolo added 2 commits October 23, 2024 06:40

Merged from upstream/main

4bed9fd

Bumped version and fixed formatting.

a5f6600

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

unacorbatanegra commented May 30, 2024

chunhtai left a comment

chunhtai Jun 7, 2024

tolo Jun 10, 2024

tolo Jun 18, 2024

chunhtai Jul 16, 2024

chunhtai Jul 16, 2024

tolo Aug 15, 2024

cedvdb commented Jul 3, 2024

tolo commented Jul 10, 2024

chunhtai commented Jul 11, 2024

chunhtai Jul 16, 2024

chunhtai Jul 16, 2024

Shunt22 commented Aug 2, 2024

tolo commented Aug 19, 2024

cedvdb Aug 20, 2024

tolo Aug 30, 2024

cedvdb commented Sep 16, 2024 •

edited

Loading

tolo commented Sep 17, 2024

chunhtai left a comment

hannah-hyj left a comment

Shunt22 commented Oct 24, 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

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

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

Shunt22 commented Aug 2, 2024

tolo commented Aug 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cedvdb commented Sep 16, 2024 • edited Loading

tolo commented Sep 17, 2024

chunhtai left a comment

Choose a reason for hiding this comment

hannah-hyj left a comment

Choose a reason for hiding this comment

Shunt22 commented Oct 24, 2024

cedvdb commented Sep 16, 2024 •

edited

Loading