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

tolo · 2023-06-19T22:18:45Z

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).

This fixes issue flutter/flutter#127804.

Pre-launch Checklist

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

JohnnyRainbow81 · 2023-06-21T11:32:09Z

Hi,
Thank you so much for your work on making routing a better dev experience in Flutter :)

But I have a question (If this is the wrong place for discussions / general questions, please give me a hint on where I can contact you alternatively):

As I am having trouble implementing deep linking into our current app structure, I'd like to ask you if it's possible to get StatefulShellRoute working with PageViews?

Right now and after searching the web on GoRouter / StatefulShellRoute tutorials, all examples mostly rely on having a single StatefulNavigationShell where the content is swapped with different child - StatefulShellBranch widgets.

I might oversee things here, but I guess it's not possible to have a PageView with fluent navigation, relying on user-swiping and/or a PageController, in combination with StatefulShellRoute & deep linking?

So, instead of having this:

...
Widget build(BuildContext context) {
    return Scaffold(
      body: navigationShell,
      bottomNavigationBar: BottomNavigationBar()  
 items: const <BottomNavigationBarItem>[
          BottomNavigationBarItem(icon: Icon(Icons.home), label: 'Child 1'),
          BottomNavigationBarItem(icon: Icon(Icons.work), label: 'Child 2'),
          BottomNavigationBarItem(icon: Icon(Icons.tab), label: 'Child 3'),
        ],
        currentIndex: navigationShell.currentIndex,
        onTap: (int index) => _onTap(context, index),
      ),
...

...would something like this also be possible?

Widget build(BuildContext context) {
    return Scaffold(
      body: PageView(children: [ ChildWithOwnRoute1(), ChildWithOwnRoute2(), ChildWithOwnRoute3()]),
      )}

This might be obvious for people with more experience on routing, but right now I'm unable to see the forest for the trees...Thank you for your help!

tolo · 2023-06-25T07:17:08Z

Hi, Thank you so much for your work on making routing a better dev experience in Flutter :)

But I have a question (If this is the wrong place for discussions / general questions, please give me a hint on where I can contact you alternatively):

Hi @JohnnyRainbow81, and thank you! 😊
Hopefully these kinds of questions can be answered by the implementation of flutter/flutter#127209. In the meantime, the best place for this is probably Discord, StackOverflow, Twitter etc.

I did have an example with a PageView at one point, I'll try to find that. I'm also planning on putting together a suite of examples and covering them in a blog post, and they can possibly also be added to flutter/flutter#127209.

…preload # Conflicts: # packages/go_router/lib/src/builder.dart

tolo · 2023-06-25T21:37:49Z

This PR is now ready for the first review. As stated in the PR description, there are both similarities and differences compared to the original implementation, most notably that the functionality is concentrated to fewer files (more is handled in ShellRouteBase and StatefulShellRoute). Also, the preload parameter has been renamed to lazy, which was discussed in the final comments around preload in #2650. I welcome feedback on a suitable name for this parameter though.

JohnnyRainbow81 · 2023-06-25T21:39:52Z

Hi @tolo ! Thanks for answering here :)
It would be great if you could showme / post your PageView example here. Otherwise I'm thinking about doing my own (hack'ish) deep linking solution for using mostly-PageView-based routing with GoRouter...

chunhtai · 2023-07-13T17:23:42Z

packages/go_router/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 9.1.0
+
+- Adds preload support to StatefulShellRoute, configurable via `lazy` parameter on StatefulShellBranch.


Why picking lazy over preload? It seems to me latter is more descriptive. I am also a bit concern that lazy may have some negative impression, which we should avoid using in public API

The only reason I changed it to lazy was due to suggestions/feedback in #2650, but I agree that perhaps preload is a more fitting name after all.

I think preload is a more suited name.

chunhtai · 2023-07-13T18:38:26Z

packages/go_router/lib/src/builder.dart

+        // Build pages for preloadable navigators
+        final Map<GlobalKey<NavigatorState>, RouteMatchList>
+            preloadableNavigators =
+            route.preloadableNavigatorLocations(configuration);


can all these logic to be done in part of of StatefulNavigationShell's initState or didUpdateWidget or build? it seems weird that builder would need to know about preload....

We probably need to refactor some API for RouteBuilder though.

It might, but I'd I have to look in to it, but we would have to expose new API on RouteBuilder.

Refactored this logic out of RouteBuilder, and at the moment into StatefulShellRoute. The reason I didn't move it into StatefulNavigationShell and initState/didUpdateWidget is that I wanted the call back into RouteBuilder (buildShellNavigator) to be in the same build cycle, and thus ensuring the GoRouterStateRegistry, _PagePopContext and _goHeroCache are updated correctly.

chunhtai · 2023-07-27T21:57:09Z

(triage) Hi @tolo, are you still planning on coming back to this pr?

tolo · 2023-07-28T06:24:01Z

(triage) Hi @tolo, are you still planning on coming back to this pr?

Hi @chunhtai, sorry for the lack of responses, yes I am planning on coming back to this, just been away on vacation.

lvsecoto · 2023-08-11T17:43:37Z

Anything update? This is a good feature for TabBarView + ShellRouteBranch

chunhtai · 2023-08-17T21:50:38Z

(triage) @tolo Is it ok to close this pr for now until you have time to come back to this PR?

tolo · 2023-08-22T07:13:31Z

(triage) @tolo Is it ok to close this pr for now until you have time to come back to this PR?

Sorry for the late reply and low activity here recently - been away on a longish summer vacation. I'm planning on finding some time this week to work on this PR, so if we could keep it open for just a while longer, that would be great.

…preload # Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/lib/src/builder.dart # packages/go_router/lib/src/route.dart # packages/go_router/lib/src/typedefs.dart # packages/go_router/pubspec.yaml

lvsecoto · 2023-09-07T01:21:25Z

@chunhtai. Code has been updated. Can you review it?

chunhtai · 2023-09-07T21:44:52Z

Hi @tolo is this ready for another look?

tolo · 2023-09-08T15:57:11Z

Hi @tolo is this ready for another look?

Yes, I believe it is.

alaatm · 2023-10-01T21:12:10Z

Is there something we can help to push this feature?

chunhtai · 2023-10-02T16:20:42Z

I have been quite busy lately, I will make sure I review this pr today or tomorrow

chunhtai

I am still a bit skeptical about the use case of preload. It seems you can still preload data that is required for rendering ahead of time. The building of the subtree should be fast enough. It may also be cleaner to separate out data model from ui representation anyway. An alternative is to expose api for people to build their own preload solution while keep the statefulShellRoute unopinionated

chunhtai · 2023-10-09T18:46:07Z

packages/go_router/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 9.1.0
+
+- Adds preload support to StatefulShellRoute, configurable via `lazy` parameter on StatefulShellBranch.


I think preload is a more suited name.

chunhtai · 2023-10-09T18:47:44Z

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

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


why turning this to index stack? this example is for custom stateful_shell_route?

chunhtai · 2023-10-09T21:07:50Z

packages/go_router/lib/src/route.dart

@@ -1157,6 +1241,9 @@ class StatefulNavigationShellState extends State<StatefulNavigationShell>
  RouteMatchList? _matchListForBranch(int index) =>
      _branchLocations[route.branches[index]]?.value;


Can you define a class for both branch and Navigator and use one list to store them, so that you don't need to keep two lists in sync?

Also I think the current API is slightly weird. The navigator will become SizedBox.srink() when they are not loaded but still passed into containerBuilder, it seems counter intuitive if the containerBuilder decide to put some child that are never visited before or preloaded before onto the screen. We should probably have a better api.

Another thing is that we rely on both the _preloadNavigator to be build and the containerBuilder to put it as an offstage child to build the widget subtree. What if they use custom container builder and only builds the child at the index but throw the rest away?

I think the containerBuilder should have some kind of API that asks child for a given index, and if they do ask for it, we build the complete widget subtree regardless it is preloaded or not.

If branch is set to preload or has been built before but is not built by the containerBuilder, we can build a stack internally by StatefulNavigationShell and put them offstage

but is not built by the containerBuilder

This will be the hardest part of the implementation, because containerBuilder can ask for widget build still throw it away. I think you may be able to build some special widget that wraps the navigator and only build subtree when it has not been built before. Not sure if that is a good idea though. Alternative, We could also document that if you ask for widget in containerBuilder you must also build it in the subtree.

I am curious if there is a better way.

skullchap · 2023-10-10T04:12:18Z

I am still a bit skeptical about the use case of preload.

@chunhtai In defense of this PR, I can say that I have been actively using this branch for a month in active project, and it served me well without any problems. My use case is app has two tabs (2 shells of home page), that do network requests to fetch related data.

My very first annoyance with the go_router was that the tabs were constantly updated and refetched data as soon as the active tab changed, this was fixed with the help of StatefulShellBranch. The second inconvenience was that when the app was initially launched, the active tab was already making a network request, while the hidden tab was waiting for it to be visible so that network request could be made. This branch's 'preload' thingie fixed that for me.

alaatm · 2023-10-10T07:36:25Z

@chunhtai Another use case is when a tab registers multiple listeners that are triggered from various locations in the app or in response to remote notification and must react based on those notifications. Listeners are not triggered unless the tab has been loaded. It is not just a matter of pre-fetching data.

All what we need is a way for us to be able to pre-load certain tabs. Whether the implementation stays as the current proposal or changed in a way that we can control the loading logic is fine in my opinion.

chunhtai · 2023-11-02T21:45:23Z

Hi @tolo Are you still planning on coming back to this pr?

tolo · 2023-11-07T18:31:08Z

Hi @tolo Are you still planning on coming back to this pr?

Hey @chunhtai, I apologise for the bad attention on this on my part - it's been beyond crazy lately. But I am still planning on coming back to this soon.

chunhtai · 2023-11-09T22:38:40Z

Hi @tolo , I am going to close this pr for now, and feel free to reopen if you have time to come back to this pr.

tolo · 2023-11-10T06:51:32Z

Hi @tolo , I am going to close this pr for now, and feel free to reopen if you have time to come back to this pr.

Sounds good @chunhtai

alaatm · 2023-11-19T17:08:19Z

Is there anyway that we can manually force preloading all branches until this is finalized? Currently we're doing some nasty hacks to visit all branches on app load which is ugly and causes unpleasent UI flickering.

iakdis · 2024-02-28T10:24:48Z

Hi there, is there any update on this PR? We are also in need of a way to preload all branches.

cedvdb · 2024-03-09T22:19:20Z

I am still a bit skeptical about the use case of preload. It seems you can still preload data that is required for rendering ahead of time. [...] The building of the subtree should be fast enough.

@chunhtai The delay is still non negligible compared to a preload even if you load your data before rendering the page, (which we do). Widget tree async builders, images, .. add a delay during the build phase until the page is really ready. The delay is small but noticeable nonetheless

tolo · 2024-03-12T12:13:43Z

Sorry for the delay in responding, I'm taking a fresh look at this now. It might be possible to implement this a bit better now, with the refactoring done in RouteBuilder (and in go_router in general). But I will have to take a closer look at the code to be sure.

However, I see three possible solutions to this:

Create an updated solution based on the original solution (i.e. full preload support, including loading a complete navigation stack / RouteMatchList).
A somewhat simpler approach that just makes it possible to specify a preloaded widget, possibly via a preloadBuilder field on StatefulShellBranch or similar.
Figuring out a good enough pattern (a bit similar to option 2), that doesn't require any changes to go_router.

This is just a brain dump at this stage, but I'm going to start looking at these and see what's possible and makes most sense.

tolo · 2024-03-12T18:35:38Z

Surprisingly enough, I believe I was able to re-implement the preload behaviour of this PR, but with substantially less code and complexity. @chunhtai, I guess a new PR would be the best way forward instead of reopening this old one?

Anyway, in the meantime, the changes can be found here for anyone interested:
https://github.com/tolo/flutter_packages/tree/statefulshellroute-preload-v2

iakdis · 2024-03-14T12:14:02Z

Thank you so much for your effort, @tolo. I'm not sure if @chunhtai still works at Flutter, as there aren't any commits since January 1st on their profile. Perhaps we need to tag someone else.

cedvdb · 2024-03-14T12:45:39Z

@johnpryan works on the router I believe

cedvdb · 2024-03-26T13:52:22Z

@tolo In the absence of response, I'd say the best bet would be to open one in the meantime.

tolo · 2024-03-26T21:08:01Z

@tolo In the absence of response, I'd say the best bet would be to open one in the meantime.

Yes @cedvdb, I think I'll do that 👍

cedvdb · 2024-04-02T10:32:53Z

Hey @tolo just a friendly reminder that a new pull request could be posted. Since the review process takes time, might as well post it as early as possible.

I can post the PR myself if you do not have the time

tolo · 2024-04-05T15:46:13Z

Hey @tolo just a friendly reminder that a new pull request could be posted. Since the review process takes time, might as well post it as early as possible.

I can post the PR myself if you do not have the time

Thanks @cedvdb, and sorry for the delay, I'll take care of it right now.

Done: #6467

… (revised solution) (#6467) 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.

First attempt at re-implementing support for branch preload

0f8f433

github-actions bot added the p: go_router label Jun 19, 2023

tolo added 3 commits June 25, 2023 09:19

Added tests and docs.

6b441e9

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

39ed0f2

…preload # Conflicts: # packages/go_router/lib/src/builder.dart

Bumped version and updated changelog.

ca61845

tolo marked this pull request as ready for review June 25, 2023 21:27

tolo requested a review from chunhtai as a code owner June 25, 2023 21:27

chunhtai reviewed Jul 13, 2023

View reviewed changes

tolo added 2 commits September 4, 2023 15:04

Refactoring

d55ab83

tolo requested a review from chunhtai September 8, 2023 15:56

chunhtai reviewed Oct 9, 2023

View reviewed changes

chunhtai closed this Nov 9, 2023

tolo mentioned this pull request Apr 5, 2024

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

Merged

11 tasks

		@@ -1,3 +1,7 @@
		## 9.1.0

		- Adds preload support to StatefulShellRoute, configurable via `lazy` parameter on StatefulShellBranch.

		@@ -1157,6 +1241,9 @@ class StatefulNavigationShellState extends State<StatefulNavigationShell>
		RouteMatchList? _matchListForBranch(int index) =>
		_branchLocations[route.branches[index]]?.value;

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

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

Conversation

tolo commented Jun 19, 2023 • edited Loading

Pre-launch Checklist

JohnnyRainbow81 commented Jun 21, 2023 • edited Loading

tolo commented Jun 25, 2023 • edited Loading

tolo commented Jun 25, 2023

JohnnyRainbow81 commented Jun 25, 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

chunhtai commented Jul 27, 2023

tolo commented Jul 28, 2023

lvsecoto commented Aug 11, 2023

chunhtai commented Aug 17, 2023 • edited Loading

tolo commented Aug 22, 2023

lvsecoto commented Sep 7, 2023

chunhtai commented Sep 7, 2023

tolo commented Sep 8, 2023

alaatm commented Oct 1, 2023

chunhtai commented Oct 2, 2023

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

skullchap commented Oct 10, 2023 • edited Loading

alaatm commented Oct 10, 2023 • edited Loading

chunhtai commented Nov 2, 2023

tolo commented Nov 7, 2023

chunhtai commented Nov 9, 2023

tolo commented Nov 10, 2023

alaatm commented Nov 19, 2023

iakdis commented Feb 28, 2024

cedvdb commented Mar 9, 2024 • edited Loading

tolo commented Mar 12, 2024

tolo commented Mar 12, 2024

iakdis commented Mar 14, 2024

cedvdb commented Mar 14, 2024

cedvdb commented Mar 26, 2024

tolo commented Mar 26, 2024

cedvdb commented Apr 2, 2024 • edited Loading

tolo commented Apr 5, 2024 • edited Loading

tolo commented Jun 19, 2023 •

edited

Loading

JohnnyRainbow81 commented Jun 21, 2023 •

edited

Loading

tolo commented Jun 25, 2023 •

edited

Loading

chunhtai commented Aug 17, 2023 •

edited

Loading

skullchap commented Oct 10, 2023 •

edited

Loading

alaatm commented Oct 10, 2023 •

edited

Loading

cedvdb commented Mar 9, 2024 •

edited

Loading

cedvdb commented Apr 2, 2024 •

edited

Loading

tolo commented Apr 5, 2024 •

edited

Loading