csells / go_router

The purpose of the go_router for Flutter is to use declarative routes to reduce complexity, regardless of the platform you're targeting (mobile, web, desktop), handling deep linking from Android, iOS and the web while still allowing an easy-to-use developer experience.
https://gorouter.dev
441 stars 97 forks source link

[Proposal] GoRoute.pageBuilder should be optional if `.routes` has a non-zero length #172

Closed esDotDev closed 2 years ago

esDotDev commented 2 years ago

I find I often need to add placeholder builders, simply to satisfy GoRouter, and then add redirects to avoid those empty routes:

Notice the /app and settings routes, they are only there for organization, and is not actually valid destinations:

late GoRouter goRouter = GoRouter(
    routes: [
      GoRoute(path: '/', pageBuilder: (_, __) => const MaterialPage(child: LoginPage())),
      GoRoute(
        path: '/app',
        pageBuilder: (_, __) => const MaterialPage(child: SizedBox.shrink()), // empty page, 
        routes: [
          GoRoute(path: 'inbox', pageBuilder: (_, __) => const MaterialPage(child: InboxPage())),
          GoRoute(
            path: 'settings',
            pageBuilder: (_, __) => const MaterialPage(child: SizedBox.shrink()), // empty page, 
            routes: [
              GoRoute(path: 'general', pageBuilder: (_, __) => const MaterialPage(child: GeneralPage())),
              GoRoute(path: 'accounts', pageBuilder: (_, __) => const MaterialPage(child: AccountsPage())),
              GoRoute(path: 'filters', pageBuilder: (_, __) => const MaterialPage(child: FiltersPage())),
            ],
          ),
        ],
      ),
    ],
  );

Then I have to add redirects, like:

redirect: (state) {
    // redirect away from empty /app route
    if (state.location == '/app' || state.location == '/app/') return '/app/inbox';

    // redirect away from empty /app/settings route
    if (state.location == '/app/settings' || state.location == '/app/settings/') return '/app/settings/general';
  },

It seems like it would be nice if the GoRoute would just use the first pageBuilder found in .routes if it has no builder of its own.

csells commented 2 years ago

You can write that like this, which I think is more clear:

late GoRouter goRouter = GoRouter(
    routes: [
      GoRoute(path: '/', pageBuilder: (_, __) => const MaterialPage(child: LoginPage())),
      GoRoute(
        path: '/app',
        redirect: (s) => '/app/inbox', 
        routes: [
          GoRoute(path: 'inbox', pageBuilder: (_, __) => const MaterialPage(child: InboxPage())),
          GoRoute(
            path: 'settings',
            redirect: (s) => '/app/settings/general', 
            routes: [
              GoRoute(path: 'general', pageBuilder: (_, __) => const MaterialPage(child: GeneralPage())),
              GoRoute(path: 'accounts', pageBuilder: (_, __) => const MaterialPage(child: AccountsPage())),
              GoRoute(path: 'filters', pageBuilder: (_, __) => const MaterialPage(child: FiltersPage())),
            ],
          ),
        ],
      ),
    ],
  );

This is one of the benefits of route-level redirect and there's no need for a top-level redirect.

esDotDev commented 2 years ago

Sorry for the delayed response, this route-level redirect is nice for many use cases, and is preferable to the top level redirect for this use case, but not ideal as we need to repeat a string that is already declared, so it makes another maintenance pt. If any of app, settings or general changes, then this might break, vs the automatic fall-through would just work.