Skip to content
This repository has been archived by the owner on Feb 25, 2022. It is now read-only.

Add navigatorBuilder params to GoRoute #133

Closed
Tracked by #174
esDotDev opened this issue Nov 4, 2021 · 34 comments
Closed
Tracked by #174

Add navigatorBuilder params to GoRoute #133

esDotDev opened this issue Nov 4, 2021 · 34 comments
Labels
enhancement New feature or request

Comments

@esDotDev
Copy link

esDotDev commented Nov 4, 2021

While navigatorBuilder is useful for providing a single top-level scaffold or menu around your app, it forces you to use imperative logic in cases where you do not simply want to have a single wrapping scaffold.

Consider a use-case, where I have a single login page with no scaffold, once logging in, I want to wrap my main menu around the inner routes.

I could write it imperitively, with something like:

navigatorBuilder: (nav){
  if(route == '/login') return nav;
  return AppMenu(child: nav);
}

Or, I could write it declaratively like this:

routes: [
   GoRoute(path: '/login', ...);
   GoRoute(path: '/', 
       builder: (route) => AppMenu(route),
       routes: [ 
          ....
       ]
   )
]

Both approaches are valid, but currently only the imperative is possible. Imperative works nice for simple use cases like the above, but has some problems expressing complex trees.

Declarative allows much more flexibility for various navigation schemes. For example, it easily allows you to wrap secondary or tertiary menus around your routes by just declaring the builders around the routes that need them:

routes: [
   GoRoute(path: '/', ... ); // no scaffold here
   GoRoute(path: '/app', 
       builder: (route) => AppMenu(route), // wrap an outer scaffold for all sub-routes of /app
       routes: [ 
            GoRoute(path: '/messages', 
                 builder: (route) => MessagesMenu(route), // wrap an inner scaffold for all sub-routes of /messages
                 routes: [ 
                      GoRoute(path: '/inbox', ...),
                      GoRoute(path: '/outbox', ...), 
                 ]
             )
       ]
   )
]
@csells
Copy link
Owner

csells commented Nov 4, 2021

I considered that, but I don't believe it's worth the complexity. You can achieve what you're after today by simply wrapping the widget in the sub-route and not in the parent route (super route?).

@esDotDev
Copy link
Author

esDotDev commented Nov 4, 2021

So like:

GoRoute(path: '/app', 
   routes: [ 
      GoRoute(path: '/messages', 
         routes: [ 
            GoRoute(path: '/inbox', pageBuilder: (_, _) => AppMenu(child: MessagesMenu(child: Inbox()))),
            GoRoute(path: '/outbox', pageBuilder: (_, _) => AppMenu(child: MessagesMenu(child: Outbox()))),
         ]
     )
   ]
  )

Ya it works, it's kinda cludgy. As my tree grows, moving things around is a lot harder. If I want to change some properties of the Menu, I'm pasting it N number of times, basically it's not DRY.

Regarding, complexity I think it's just a few lines of recursive build calls. I was planning on putting together a PR for this sometime soon, so maybe you can look at the code and see if it's as bad as you think?

@esDotDev
Copy link
Author

esDotDev commented Nov 4, 2021

The other thing to consider is, we can have imperative logic in these sub-builders, just like in the root-level navigatorBuilder, and it lets that logic live next to the routes it is concerned with. Which the inline method doesn't really support well.

@csells
Copy link
Owner

csells commented Nov 4, 2021

Ya it works, it's kinda cludgy. As my tree grows, moving things around is a lot harder. If I want to change some properties of the Menu, I'm pasting it N number of times, basically it's not DRY.

Gosh, if only the Dart language itself had a way to package reusable code that you could leverage for this situation... (he said, sarcastically : )

Regarding, complexity I think it's just a few lines of recursive build calls. I was planning on putting together a PR for this sometime soon, so maybe you can look at the code and see if it's as bad as you think?

It's a slippery slope, man. I don't want to add anything to GR that you can already do in Flutter w/o my help.

@esDotDev
Copy link
Author

esDotDev commented Nov 4, 2021

Sure you could make config widgets purely for the purposes of building specific sets of widgets, like:

class AppMenuWithMessagesMenu { 
    AppMenuWithMessagesMenu(this.child);
    final Widget child;
    ...
} 

class AppMenuWithHomeMenu {} 

class AppMenuWithMessageMenuWithAdminMenu {} 

but yuck :p I think canonical flutter way here is nested builders for sure.

@csells
Copy link
Owner

csells commented Nov 4, 2021

perhaps a good ol' fashioned parameterized function would do the trick?

@csells csells added wontfix This will not be worked on and removed waiting on customer input labels Nov 7, 2021
@csells csells closed this as completed Nov 7, 2021
@esDotDev
Copy link
Author

esDotDev commented Nov 7, 2021

Named functions is better than having to declare a class, but still objectively cludgy compared to a builder.

  • repetitive and adds clutter to the route declerations, raises signal:noise ratio, making things harder to read
  • slower to refactor and change the tree, as you have to unwrap these widgets if the parent changes, rather than just reparenting routes
  • splits logic apart away from the tree, you're forced to put imperative logic inside of these wrapper widget/methods instead of being in the builder, close to the context it's referring too
  • just less flexible and more rigid in general, once you get to the 3rd set of menus, the wrapping method can't scale

Also, thinking about it some more, I'm not sure you actually can get the same behavior from wrapping an inner widget.

  • With the builder proposal, the outer widget(s) are outside of Navigator and do not navigate with the route.
  • With the "nested menu" approach, the menus are part of the route and will be animated.

So there is a fundamental difference here. Pretty sure any arguments made against this, can effectively be made against navigatorBuilder.

@csells
Copy link
Owner

csells commented Nov 7, 2021

The navigatorBuilder function allows you to create a widget above the Navigator. The builder function would just let you factor things differently when you already have three ways to factor:

  1. using existing widgets to wrap the child widgets
  2. creating a new widget to wrap the child widgets
  3. using a function to create the widgets to wrap the child widgets

Regardless, each one of these would be wrapped in a Page<dynamic> and hosted by the Navigator.

@esDotDev
Copy link
Author

esDotDev commented Nov 8, 2021

I think I'm seeing our disconnect. This proposal is for allowing multiple widgets above navigator, in cases where you don't want a "single wrapping scaffold", you instead want multiple wrapping scaffolds or just different scaffolds for different sets of sub-routes.

For the same reason we would want AppMenu above the navigator, we would want MessagesMenu above it (ie, to preserve it's state easily, and avoid being animated away when changing page withn the same sub-section)

The desired widget structure with my example code is something like:

AppMenu(
  child: MessagesMenu(
    child: Navigator(
        pages: [
            ...
         ]
    )
  )
)

Not:

AppMenu(
  child:  Navigator(
        pages: [
            Page(child: MessagesMenu( ... ))
         ]
    )
)

The only way to do this now, is a glob of imperitive code:

navigatorBuilder: (nav){
   if(route.contains('/messages')) return AppMenu(child: MessagesMenu(child: nav)
   return AppMenu(child: nav);
}

Which again, works ok in trivial examples, falls apart and becomes hard to read and debug when things begin to scale.

@csells
Copy link
Owner

csells commented Nov 8, 2021

I see. You're proposing adding a navigatorBuilder to each route. Do you think it'll get that much use? Is it giving you something you can't do in the top level navigatorBuilder?

@esDotDev
Copy link
Author

esDotDev commented Nov 8, 2021

Personally I think it's one of those tiny features that unlocks many use cases. Like I mentioned in the OP, it doesn't give you anything you can't do imperatively, but it lets to express the same thing declaratively. And it's this combination of using declarative and imperative that really provides the nicest workflow and flexibility.

As a concrete example, one very common architectural pattern is one where your initial route does not have a main menu, but then all inner routes do.

final router = GoRouter(
  routes: [
    GoRoute(path: '/', pageBuilder: (c, __) => buildAuthPage(c)),
    GoRoute(
      path: '/tabs',
      navigatorBuilder: (context,  child, state) => TabMenu(child),
      routes: [
        GoRoute(path: '/1', pageBuilder: (_, __) => buildPageOne()),
        GoRoute(path: '/2', pageBuilder: (_, __) => buildPageTwo()),
        GoRoute(path: '/3', pageBuilder: (_, __) => buildPageThree()),
      ],
    ),
  ],
);

A little unrelated, but I keep coming up with this problem in the navigatorBuilder, I can't actually access the current location.

final router = GoRouter(
   navigatorBuilder: (context, child) {
      // I want to make some decisions based on .location here but can't seem to access the router
   }
)

It would be nice if the signature for this builder was (BuildContext, Widget, GoRouterState) so we could easily make decisions based on the current location. This would also be the same signature for the nested builders who may also want to have small chunks of imperative logic inside their builders. This would also solve the misleading Widget? in the builder. Happy to make a ticket if you think it makes sense.

@csells
Copy link
Owner

csells commented Nov 8, 2021

You begin to convince me.

@csells csells reopened this Nov 8, 2021
@esDotDev
Copy link
Author

esDotDev commented Nov 9, 2021

side note - one issue that crops up when using this technique is Dialogs, Overlays and BottomSheets. Usually you want them to appear full-screen, and not be nested in any surrounding nav, which can be an issue when your Navigator is wrapped by scaffolding.

Luckily, I just tested and this appears to work great:

runApp(
      MaterialApp(
          home: MaterialApp.router(
            routerDelegate: router.routerDelegate,
            routeInformationParser: router.routeInformationParser,
          ),
      ),
  );

The top level MaterialApp can handle the full screen stuff, with the inner one handling all routing and wrapping.
I guess we can now truly say that it uses "nested" navigators :D

7Vfgc5vsPb.mp4

@csells
Copy link
Owner

csells commented Nov 9, 2021

wtf?! that's pure craziness. I like it! : )

@csells csells added enhancement New feature or request wontfix This will not be worked on and removed wontfix This will not be worked on labels Nov 9, 2021
@esDotDev
Copy link
Author

esDotDev commented Nov 10, 2021

That makes me wonder... could we just use Router directly? Based on the docs, it almost seems like that should work:

runApp(
      MaterialApp(
          home: Router(
            routerDelegate: goRouter.routerDelegate,
            routeInformationParser: goRouter.routeInformationParser,
          ),
      ),
  );

[Edit] Just tested and this works great! Very cool! Feels a lot less hacky than using multiple MaterialApps.

@noga-dev
Copy link

That makes me wonder... could we just use Router directly? Based on the docs, it almost seems like that should work:

runApp(
      MaterialApp(
          home: Router(
            routerDelegate: goRouter.routerDelegate,
            routeInformationParser: goRouter.routeInformationParser,
          ),
      ),
  );

[Edit] Just tested and this works great! Very cool! Feels a lot less hacky than using multiple MaterialApps.

What if the MaterialApp uses the same delegate as the nested router? Does the nav stack get added to the existing one or create a new one?

@csells csells changed the title Add builder params to GoRoute Add navigatorBuilder params to GoRoute Nov 13, 2021
@csells
Copy link
Owner

csells commented Nov 13, 2021

Is there a real-world example of where this would be useful? If so, I'm inclined to add it.

@csells csells removed the wontfix This will not be worked on label Nov 13, 2021
@lulupointu
Copy link
Contributor

Using navigatorBuilder looks good. I don't think this would be true nested navigation (i.e a navigator inside a navigator) but from my experience nested navigation is never used for more than what navigatorBuilder could do. Great idea for much less headaches !

WARNING about the side note: DON'T use MaterialApp.router or Router nested inside a MaterialApp. Both code samples in #133 (comment) and #133 (comment) will break:

  • Deep-linking
  • Browser navigation (manually types url, backward/forward button)

@csells
Copy link
Owner

csells commented Nov 14, 2021

cc @esDotDev thanks for the tip, @lulupointu !

@esDotDev
Copy link
Author

esDotDev commented Nov 15, 2021

Is there a real-world example of where this would be useful? If so, I'm inclined to add it.

gmail uses this paradigm, all the routes under /settings, are wrapped in this secondary tab menu.
dwbuqWNmoj

In this case,

  • the "outer" scaffold is the side menu, gmail logo and search menu.
  • the nested or inner scaffold, is the SettingsMenu with General | Labels | ... which is only present on the settings sub-routes.
  • If we wanted to animate the tab menu (imagine the blue line tweens to the new menu location), or we wanted to animate in the route itself, we would need the SettingsMenu outside of the navigator so it retains animation state, and does not fadeIn or slideIn with the route itself.

@csells
Copy link
Owner

csells commented Nov 15, 2021

Sorry. I meant if you wanted to produce an example to add to the repo that required route-level navigatorBuilder, then I'm happy to add the feature.

@esDotDev
Copy link
Author

esDotDev commented Nov 19, 2021

Is this enough to run with?

class GoRouteBuildersDemo extends StatefulWidget {
  const GoRouteBuildersDemo({Key? key}) : super(key: key);

  @override
  State<GoRouteBuildersDemo> createState() => _GoRouteBuildersDemoState();
}

class _GoRouteBuildersDemoState extends State<GoRouteBuildersDemo> {
  MaterialPage createPage(Key key, Widget child) => MaterialPage(child: child, key: key);
  late GoRouter goRouter = GoRouter(
    routes: [
      // No scaffold for login page
      GoRoute(path: '/', pageBuilder: (_, __) => const MaterialPage(child: LoginPage())),

      GoRoute(
        path: '/app',
         // All /app pages get the main scaffold
        navigatorBuilder: (_, child) => MainScaffold(child: child!), 
        pageBuilder: (_, __) => const MaterialPage(child: SizedBox.shrink()),
        routes: [
          GoRoute(path: 'inbox', pageBuilder: (_, state) => createPage(state.pageKey, const InboxPage())),
          GoRoute(
            path: 'settings',
            // all /app/settings pages get SettingsMenu
            navigatorBuilder: (_, child) => SettingsMenu(child: child!),            
            pageBuilder: (_, __) => const MaterialPage(child: SizedBox.shrink()),
            routes: [
              GoRoute(path: 'general', pageBuilder: (_, s) => createPage(s.pageKey, const GeneralPage())),
              GoRoute(path: 'accounts', pageBuilder: (_, s) => createPage(s.pageKey, const AccountsPage())),
              GoRoute(path: 'filters', pageBuilder: (_, s) => createPage(s.pageKey, const FiltersPage())),
            ],
          ),
        ],
      ),
    ],
    errorPageBuilder: (BuildContext context, GoRouterState state) => MaterialPage(child: Text('${state.error}'))
  );

  @override
  Widget build(BuildContext context) {
    return MaterialApp.router(
      routeInformationParser: goRouter.routeInformationParser,
      routerDelegate: goRouter.routerDelegate,
    );
  }
}

This gives you:

VGz5MVJDSO.mp4

This is possible now, with the blob of imperative code at the root level, but's not that great to read, and would only get worse over time:

navigatorBuilder: (_, child) {
  // on a page under /app/settings? add a settings menu
  if (goRouter.location.startsWith('/app/settings')) {
    child = SettingsMenu(child: child!);
  }
  // on a page under /app add the main scaffold
  if (goRouter.location.startsWith('/app/')) {
    child = MainScaffold(child: child!);
  }
  return child;
},

Supporting content classes here:

class ContentPage extends StatelessWidget {
  const ContentPage({Key? key, required this.title}) : super(key: key);
  final String title;

  @override
  Widget build(BuildContext context) =>
      Scaffold(body: Center(child: Text(title, style: const TextStyle(fontSize: 48))));
}

class LoginPage extends ContentPage {
  const LoginPage({Key? key}) : super(key: key, title: 'LOGIN');

  @override
  Widget build(BuildContext context) {
    return GestureDetector(onTap: () => context.go('/app/inbox'), child: super.build(context));
  }
}

class InboxPage extends ContentPage {
  const InboxPage({Key? key}) : super(key: key, title: 'INBOX');
}

class GeneralPage extends ContentPage {
  const GeneralPage({Key? key}) : super(key: key, title: 'GeneralPage');
}

class AccountsPage extends ContentPage {
  const AccountsPage({Key? key}) : super(key: key, title: 'AccountsPage');
}

class FiltersPage extends ContentPage {
  const FiltersPage({Key? key}) : super(key: key, title: 'FiltersPage');
}

class MainScaffold extends StatelessWidget {
  const MainScaffold({Key? key, required this.child}) : super(key: key);
  final Widget child;

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      backgroundColor: Colors.grey.shade200,
      body: Column(
        children: [
          const Text('APP TITLE', style: TextStyle(fontSize: 32)),
          Expanded(child: child),
          Row(
            children: [
              TabButton('inbox', onPressed: () => context.go('/app/inbox')),
              TabButton('settings', onPressed: () => context.go('/app/settings/general'))
            ],
          )
        ],
      ),
    );
  }
}

class SettingsMenu extends StatelessWidget {
  const SettingsMenu({Key? key, required this.child}) : super(key: key);
  final Widget child;

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Column(
        children: [
          Text('My Settings', style: TextStyle(fontSize: 32)),
          ColoredBox(
            color: Colors.grey.shade300,
            child: Row(
              children: [
                TabButton('general', onPressed: () => context.go('/app/settings/general')),
                TabButton('accounts', onPressed: () => context.go('/app/settings/accounts')),
                TabButton('filters', onPressed: () => context.go('/app/settings/filters'))
              ],
            ),
          ),
          Expanded(child: child),
        ],
      ),
    );
  }
}

class TabButton extends StatelessWidget {
  const TabButton(this.label, {Key? key, required this.onPressed}) : super(key: key);
  final VoidCallback onPressed;
  final String label;

  @override
  Widget build(BuildContext context) {
    return Expanded(
        child: TextButton(
            child: Padding(
              padding: const EdgeInsets.all(16.0),
              child: Text(
                label.toUpperCase(),
                style: TextStyle(fontSize: 24),
              ),
            ),
            onPressed: onPressed));
  }
}

@lulupointu
Copy link
Contributor

I'm not sure I like this. In my opinion:

DO use navigatorBuilder for dependency injection

DON'T use navigatorBuilder for UI building

The reason is that the animations are not right if you do so. When you navigate from InboxPage to a page in SettingsMenu, the top settings menu should be animated with the rest of the page imo. This is just my opinion though, and anyway once/if this feature is implemented nothing will prevent developers to use it for UI building, but I don't think this should be encouraged either.

@esDotDev
Copy link
Author

esDotDev commented Nov 19, 2021

Well the counter point to that, stateful animations within the UI will not work right if they are inside the navigator.

Imagine a basic TabBar in flutter, that tweens the underline to each new tab. This breaks if the menu is outside the navigator.

There's no hard rules here, the design of each app will dictate what is more desirable. imo, just keep it flexible and unopinionated.

If I had your issue IRL, we would likely switch to Fade transitions, this would give us a nice transition from Inbox to Settings, but also maintain a stateful SettingsMenu so I can do cool stuff in there if I need.

Additionally, if I really wanted to, it is trivial to have SettingsMenu have some sort of "SlideIn" effect when it first shows, since it's stateful and can do that sort of thing :) So if you made your routes fade in, and have your inner menus fade in, you'd get basically get the best of both worlds, transitions would look as expected but the menu maintains state when changing child pages.

@esDotDev
Copy link
Author

esDotDev commented Nov 19, 2021

I guess what you are probably recommending is more so like this micro-routing approach, where at some point, all the routes merge into one page, that has a stack of pages internally.

So like in this case, all the /settings/:subroute routes would end up on MaterialPage(key: ValueKey('/settings'), child: SettingsPage(state.params['subroute'])), and internally, Settings would decide which child Widget to show. This would also get you best of both worlds, but the tradeoff is that you need to bury this nested sub-routing code all over your ui, rather than just declaring routes and builders at the top of the tree.

So to me they both have tradeoffs, which way you go would depend on the specific design of your app, and the priorities of the stakeholders. There are ways to make the outer builder approach look good when transitioning, that might be fairly easy to implement, and give an even cooler effect than slideUp or fadeIn.

Like imagine if my sub-menus quickly folded-open or slid-out as the page content slides in from another direction, now we're talking!

@esDotDev
Copy link
Author

For example, I added a simple FadeIn widget to the top of the SettingsMenu, and now it looks quite nice? Another 30m of work on it and it could look even better.

i4SrWb2sN8.mp4

@esDotDev
Copy link
Author

Worth noting this API is nice for providing pieces of state to the tree below in addition to visual overlays or wrapping scaffolds:

 GoRoute(
      navigatorBuilder: (context, navigator){
         return Provider(create: (_)=> Quiz1(), child: navigator);
      },
      path: '/quiz',
      pageBuilder: (context, state) => MaterialPage<void>(...),
      routes: [ ... ], 
    );

As mentioned here: #185

@jamesblasco
Copy link

jamesblasco commented Dec 22, 2021

I personally think this would be confusing to add it to all the GoRoute as it changes the functionality of the routes

Right the navigations is one-level that allow multiple combinations that are stacked one above the other

  • '/'
  • 'family/100'
  • 'family/100/person/200'
  • 'family/100/person/200'

Screenshot 2021-12-22 at 14 18 05

I think it should be possible to have nested navigators like:

  • '/'
  • 'family/100'
    • New Navigator
      • 'person/200'
      • 'person/200/edit'

We could create a specific a new GoNavigatorRoute class, instead of breaking the behaviour of the current GoRoute ( that is the most used case scenario)

Screenshot 2021-12-22 at 14 29 55

@csells
Copy link
Owner

csells commented Dec 22, 2021

agreed, @jamesblasco. that's something we're exploring in #222

@leoshusar
Copy link

leoshusar commented Jan 10, 2022

Some time ago I was discussing similar thing here #128.
Although I've got some workaround for it, it's still not as great as per-route navigationBuilder would be, especially with more routes.

Then I thought the new v3 top level navigatorBuilder with the state parameter would solve it. It doesn't, because I cannot get URL params there.

navigatorBuilder: (context, state, child) {
  if (/* I'm on /users/:userId/xxx */) {
    return Provider(
      create: (_) => UserBloc(state.params['userId']), // params are not available here
      child: child,
    );
  }
}

Also this would not easily work with nested routes and their nested providers and the top level navigatorBuilder would get really big.

So I tried to make PoC how this issue would solve my problem, it's available here and the example is here.
The GoRouter code is not pretty, but for PoC it does the job :)

Explanation:

Here is my users route definition

GoRoute(
  path: '/users',
  builder: (context, state) => const UsersScreen(),
  routes: [
    GoRoute(
      path: ':userId',
      redirect: (state) => '/users/${state.params['userId']}/roles',
      // UserBloc with the current user will be available
      // for all subsequent routes..
      navigatorBuilder: (context, state, child) => Provider(
        create: (_) => UserBloc(int.parse(state.params['userId']!)),
        child: child,
      ),
      routes: [
        GoRoute(
          path: 'roles',
          pageBuilder: (context, state) => const MaterialPage<void>(
            key: ValueKey('/users/:userId'),
            child: UserScreen(tab: 'roles'),
          ),
          routes: [
            GoRoute(
              path: ':roleId',
              builder: (context, state) {
                final userId = int.parse(state.params['userId']!);
                final roleId = state.params['roleId']!;
                return Provider(
                  create: (_) => RoleBloc(userId, roleId),
                  child: const RoleScreen(),
                );
              },
            ),
          ],
        ),
        GoRoute(
          path: 'info',
          pageBuilder: (context, state) => const MaterialPage<void>(
            key: ValueKey('/users/:userId'),
            child: UserScreen(tab: 'info'),
          ),
        ),
      ],
    ),
  ],
),

When I am on the /users tab, everything is ok.
When I open some user, I create UserBloc and want to have it "recursively" in all children routes.

Current way of doing it is like this:

GoRoute(
  path: '/users',
  builder: (context, state) => const UsersScreen(),
  routes: [
    GoRoute(
      path: ':userId/roles',
      pageBuilder: (context, state) => const MaterialPage<void>(
        key: ValueKey('/users/:userId'),
        child: Provider(
          create: (_) => UserBloc(int.parse(state.params['userId']!)),
          child: const UserScreen(tab: 'roles'),
        ),
      ),
      builder: (context, state) => ,
      routes: [
        GoRoute(
          path: ':roleId',
          builder: (context, state) {
            final userId = int.parse(state.params['userId']!);
            final roleId = state.params['roleId']!;
            return Provider(
              create: (_) => RoleBloc(userId, roleId),
              child: const RoleScreen(),
            );
          },
        ),
      ],
    ),
    GoRoute(
      path: ':userId/info',
      pageBuilder: (context, state) => const MaterialPage<void>(
        key: ValueKey('/users/:userId'),
        child: Provider(
          create: (_) => UserBloc(int.parse(state.params['userId']!)),
          child: const UserScreen(tab: 'info'),
        ),
      ),
    ),
  ],
),

see I have to have

Provider(
  create: (_) => UserBloc(int.parse(state.params['userId']!)),
  child: const UserScreen(tab: $tab),
)

written for each route. This gets really ugly when I have multiple blocs and multiple tabs.

The only problem I have noticed is going back to /users from /users/:id/xxx.
When you press the back button, it goes from /users/:id/xxx to /users/:id - because /users/:id is somehow also considered as a route although it's only purpose is to redirect.
So in this example you need to manually remove the user ID when going back to /users, otherwise it would still keep the old UserBloc.
But I don't believe this would be impossible to solve, right?

So... What do you think?

@esDotDev
Copy link
Author

esDotDev commented Jan 10, 2022

I personally think this would be confusing to add it to all the GoRoute as it changes the functionality of the routes

I'd argue it compliments the functionality of the routes, which are used not only to declare single routes, but also to declare parent/child relationships and nesting.

Often scaffolding, or provided controllers, need to be injected along these same parent/child boundaries, so it's a natural place to do it.

For example, if all pages within /search, need a persistent search bar at the top, the most obvious place to declare it is within the parent GoRoute:

return GoRoute(
  path: '/search',
  navigatorBuilder(_, __, child) => SearchScaffold(body: child),
  routes: [
    GoRoute('inbox', ...),
    GoRoute('outbox', ...),
  ]
)

This is most intuitive, and nothing else is comparable in terms of readability and scalability. It does not take very many decisions before the "do it all in the top builder" turns into a big mess.

Using a dedicated widget is a good idea too, VRouter did this with a VNester, but not sure it's worth the extra API surface and complexity when just adding a navigatorBuilder seems effective and simple.

@jamesblasco
Copy link

But then how it would work inside? If there is a navigatorBuilder then the Route would inject a Navigator but if not builder is provided it still uses its parent one?
We should not be creating a navigator per route.

@leoshusar
Copy link

leoshusar commented Jan 10, 2022

For my use case it would use all navigatorBuilders from the top level one to the final matched GoRoute.

So if you had navigatorBuilder defined for every single GoRoute (just an example) and opened route /users/:id/roles, it would use 4 matched builders:

GoRouter # this one
|_GoRoute `users` # this one
| |_GoRoute `:id` # this one
|   |_GoRoute `roles` # this one
|   | |_GoRoute `:roleId`
|   |_GoRoute `info`
|_GoRoute `settings`

Like this:

root navigation builder
|_users navigation builder
  |_id navigation builder
    |_roles navigation builder

Since all of them are navigator builders, I think it makes sense that all matched routes navigator builders are used.
It would just act like a "top level builder extension".

This also meets lulupointu's opinion about using it for dependency injection.

@jamesblasco if I understand @esDotDev correctly, he doesn't inject new navigators, am I right? I believe his idea is similar to mine, that is just wrapping the main navigator with custom widgets (correct me if I'm wrong).

@esDotDev
Copy link
Author

esDotDev commented Jan 10, 2022

Yes, there is not a new navigator created for each route, it would just be wrapping additional scaffolding or providers around the one Navigator.

In the example above, after all 4 builders have ran, you'd end up with something like this returned from the router delegate:

RootNav(
  child: UsersNav(
     child: IdNav(
        child: RolesNav(
           child: Navigator(pages: [])
        )
      )
    )
 )

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants