-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[go_router] Path based branch routing for StatefulShellRoute - deprecating goBranch #7622
base: main
Are you sure you want to change the base?
Conversation
…emoving the need of using goBranch.
@chunhtai, would appreciate if you could have a look at this and see what you think, before I contuine fully implementing/documenting/testing this. The current implementation works (at least as first attempt) and the |
It feels a bit weird that we force the statefulbranch to use numeric path, is there a reason behind this change? |
Well, a numeric index is what we're using already, via the programmatic API. So there really is no change in what information you use to switch branch. But I was actually thinking about re-introducing the concept of giving each branch a name (which did exist at some point in the original StatefulShellRoute PR), and now that I think of it, this may certainly make the navigation feel less weird. I'll go ahead and add that change. |
…named branch redirects (i.e. instead of index).
I am more concern about url will be force to use numeric path like Another concern is that we should figure out when two url may point to same page. for example if /mypath/subpath also switch branch to 0, then both /mypath/subpath and /mypath/0 points to same page. In this case should the url for /mypath/0 redirect to /mypath/subpath? we need to update the reportRouteToEngine code to make sure we update the history correctly. |
Updated the description with more information about using names instead of indices. This change uses the standard redirect handling, so I would assume circular redirects etc would be handled already (i.e. redirect limits and supporting reporting back to the engine)? |
…fulNavigationShell. Introduced the GoRouterState subclass ShellRouteState, as replacement for ShellRouteContext and StatefulNavigationShell in public APIs.
Ok, so I took it one step further, to more thoroughly adress the issues of flutter/flutter#128262. It may be too much to do in a single step / PR, but I mainly did it to showcase a potential refactoring of the API around StatefulShellRoute, to improve the DX and make the API more in line with the rest of go router (i.e. path based). Would be great if you could take a deeper look at this @chunhtai. (Edit: updated the original PR description/comment with information about these changes). |
@tolo I'm not sure I understand the point of this PR. This is a response to your comment on the other PR where you link this one. Maybe it's to make routing work nicely with the usual navigation widgets which work with indexes currently, in which case, what's wrong with:
|
Basically, the initial intent of this PR was to add support for a path-based way of navigating to the current state of a stateful shell route or a branch thereof, which is not possible today. The more I worked on this PR though, other related opportunities for improvements also presented themselves. I would now summarise the motivations of this PR like this (will add it to the description):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of using index, have you thought about using name instead? i.e a new name parameter to StatefulShellRouteBranch. and then you can use reuse goName API
@@ -42,19 +42,35 @@ typedef ShellRoutePageBuilder = Page<dynamic> Function( | |||
); | |||
|
|||
/// The widget builder for [StatefulShellRoute]. | |||
typedef StatefulShellRouteBuilder = Widget Function( | |||
@Deprecated('Use updated builder method') | |||
typedef DeprecatedStatefulShellRouteBuilder = Widget Function( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not really help with deprecation since you changed the method name.
Also since this is a package, I would just write a breaking change doc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I know, this was mainly temporary, to showcase the changes made more clearly. And the deprecation was just a way to "ease" into these changes, to make them less dramatic. But perhaps it's better to just rip off the band-aid and make it a breaking change.
/// The page builder for [StatefulShellRoute]. | ||
typedef StatefulShellRoutePageBuilder = Page<dynamic> Function( | ||
@Deprecated('Use updated page builder method') | ||
typedef DeprecatedStatefulShellRoutePageBuilder = Page<dynamic> Function( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
Removed deprecation.
Good suggestion. |
Support for path based routing (i.e. using
GoRouter.go
) when switching branches in aStatefulShellRoute
, making routing with StatefulShellRoute less of a special case. This also means that thegoBranch
method becomes obsolete.This change introduces a new field on StatefulShellRoute to configure a
path
for branch navigation on the shell route, which under the hood generates a number of redirection routes. These routes allow for path based navigation for switching branch and restoring branch/route state. Effectively, this negates the need to usegoBranch
, and even the need to expose the classStatefulNavigationShell
.To summarise, the basic motivations of this PR are:
TL;DR - see the stateful_shell_route.dart example for a runnable demo of this change.
Short demonstration of this change
You start by configuring your shell route like this:
This enables you to do this:
Instead of this:
There are also additional redirects available:
Update
The latest version of this PR also included some refactoring around the
StatefulShellRoute
API, including the introduction of theGoRouterState
subclassShellRouteState
. The goal with this is to hide internal implementation details (primarily the classStatefulNavigationShell
) and improve the DX of the API. Example (fromstateful_shell_route.dart
):This PR addresses:
StatefulShellRoute.indexedStack
should have aroutes
field for allowing navigations that go on top of theStatefulShellRoute
flutter#140273Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.