Skip to content
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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

tolo
Copy link
Contributor

@tolo tolo commented Sep 11, 2024

Support for path based routing (i.e. using GoRouter.go) when switching branches in a StatefulShellRoute, making routing with StatefulShellRoute less of a special case. This also means that the goBranch 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 use goBranch, and even the need to expose the class StatefulNavigationShell.

To summarise, the basic motivations of this PR are:

  • Introduce path based navigation for StatefulShellRoute, to make more in line with routing in GoRouter in general, and less of a special case.
  • Related to the above point: remove the method goBranch, primarily from the Widget StatefulNavigationShell, where it is somewhat misplaced.
  • Hide internal implementation details, like StatefulNavigationShell, to make the surface area of the StatefulShellRoute API smaller, and again; more in line with GoRouter in general, and less of a special case.

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:

StatefulShellRoute.indexedStack(
  path: '/myshell', 
  initialBranchIndex: 1, // Optionally set which branch should be default
  ...
  branches: <StatefulShellBranch>[
    StatefulShellBranch(
    name: 'branchA', // Optionally associate a name/alias to a branch
    ...

This enables you to do this:

GoRouter.of(context).go('/myshell/0');
// or this:
GoRouter.of(context).go('/myshell/branchA');

Instead of this:

StatefulNavigationShell.of(context).goBranch(0);

There are also additional redirects available:

// Redirect to the initial state of the first branch: 
GoRouter.of(context).go('/myshell/0/initial'); 
// or:
GoRouter.of(context).go('/myshell/branchA/initial'); 

// Redirect to the current state/branch of the stateful shell route. If no state 
// exists, the redirect will go to the initial location of the branch at 'initialBranchIndex'
GoRouter.of(context).go('/myshell'); 

Update
The latest version of this PR also included some refactoring around the StatefulShellRoute API, including the introduction of the GoRouterState subclass ShellRouteState. The goal with this is to hide internal implementation details (primarily the class StatefulNavigationShell) and improve the DX of the API. Example (from stateful_shell_route.dart):

StatefulShellRoute.indexedStackContainer(
  path: '/shell',
  builder: (BuildContext context, ShellRouteState state, Widget child) {
    return ScaffoldWithNavBar(shellState: state, child: child);
   }
   ...,
}

...

class ScaffoldWithNavBar extends StatelessWidget {
  const ScaffoldWithNavBar({
    required this.shellState,
    required this.child,
    Key? key,
  }) : super(key: key ?? const ValueKey<String>('ScaffoldWithNavBar'));

  final ShellRouteState shellState;
  final Widget child;

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: child,
      bottomNavigationBar: BottomNavigationBar(
          items: const <BottomNavigationBarItem>[
            BottomNavigationBarItem(icon: Icon(Icons.home), label: 'Section A'),
            BottomNavigationBarItem(icon: Icon(Icons.work), label: 'Section B'),
            BottomNavigationBarItem(icon: Icon(Icons.tab), label: 'Section C'),
          ],
          currentIndex: shellState.navigatorIndex,
          onTap: (int index) {
            return switch (index) {
              1 => GoRouter.of(context).go('/shell/branchB'),
              2 => GoRouter.of(context).go('/shell/branchC'),
              _ => GoRouter.of(context).go('/shell/branchA'),
            };
          }),
    );
  }
}


This PR addresses:

Pre-launch Checklist

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

@tolo
Copy link
Contributor Author

tolo commented Sep 11, 2024

@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 stateful_shell_route.dart sample is updated to use it.

@chunhtai
Copy link
Contributor

It feels a bit weird that we force the statefulbranch to use numeric path, is there a reason behind this change?

@tolo
Copy link
Contributor Author

tolo commented Sep 13, 2024

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).
@chunhtai
Copy link
Contributor

I am more concern about url will be force to use numeric path like
/mypath/0, /mypath/1.

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.

@tolo
Copy link
Contributor Author

tolo commented Sep 16, 2024

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.
@tolo
Copy link
Contributor Author

tolo commented Sep 17, 2024

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

@cedvdb
Copy link
Contributor

cedvdb commented Sep 17, 2024

@tolo I'm not sure I understand the point of this PR. router.go('/branchA'); already works. Why would a number in the URL be desirable ( when you consider the web platform ) ?

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:

StatefulNavigationShell.of(context).goBranch(index);

@tolo
Copy link
Contributor Author

tolo commented Sep 18, 2024

@tolo I'm not sure I understand the point of this PR. router.go('/branchA'); already works. Why would a number in the URL be desirable ( when you consider the web platform ) ?

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

  • Introduce path based navigation for StatefulShellRoute, to make more in line with routing in GoRouter in general, and less of a special case.
  • Related to the above point: remove the method goBranch, primarily from the Widget StatefulNavigationShell, where it is somewhat misplaced.
  • Hide internal implementation details, like StatefulNavigationShell, to make the surface area of the StatefulShellRoute API smaller, and again; more in line with GoRouter in general, and less of a special case.

Copy link
Contributor

@chunhtai chunhtai left a 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(
Copy link
Contributor

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.

/// The page builder for [StatefulShellRoute].
typedef StatefulShellRoutePageBuilder = Page<dynamic> Function(
@Deprecated('Use updated page builder method')
typedef DeprecatedStatefulShellRoutePageBuilder = Page<dynamic> Function(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants