tomgilder / routemaster

Easy-to-use Navigator 2.0 router for web, mobile and desktop. URL-based routing, simple navigation of tabs and nested routes.
https://pub.dev/packages/routemaster
MIT License
327 stars 61 forks source link

☂️ API naming #19

Closed tomgilder closed 3 years ago

tomgilder commented 3 years ago

StackNavigator

Guards

Delegate

RouteData

Current plan for 0.8.0

theweiweiway commented 3 years ago

I like url I like canNavigate I like builder or routeMapBuilder I like RouteData I like StackNavigator Just my 2 cents...

chimon2000 commented 3 years ago

Need to make decisions on some names, such as:

  • path could be route or url

I like route, href, or url.

href is synonymous with the entire URL: https://developer.mozilla.org/en-US/docs/Web/API/Location/href. Route also has a pretty synonymous meaning in web development.

  • validate could be canNavigate

I can go with canNavigate or canActivate.

  • routesBuilder could be mapBuilder, builder or routeMapBuilder

Just builder unless there's a need to be explicit here, like multiple builders, the IntelliSense (I assume is more than enough).

RouteData

Alternatives:

  • PathInfo

For me, RouteData conveys enough info - maybe RouteInfo makes the most sense. PathInfo makes sense if you stick with path.

StackNavigator

Alternatives:

  • PagesNavigator
  • PageStackNavigator

This one is the least obvious to me. I actually had to look at StackNavigator in code to figure out what it was doing. When I think of a stack I automatically think of a Flutter Stack or IndexStack widget rather than the route history. I am assuming that StackNavigator would be used in tandem with PageView/TabView/IndexedStack/Stack - anywhere that you would want to reflect back to the URL using some type of controller. Looking at an example implementation:

Maybe PageStackNavigator or RouteStackNavigator makes the most sense here. I saw a similar pattern in react-router with switches. I could see RouteSwitch since that this would convey that the widget switches the route.

tomgilder commented 3 years ago

Thank you for your feedback!

I like route, href, or url.

href is synonymous with the entire URL: https://developer.mozilla.org/en-US/docs/Web/API/Location/href. Route also has a pretty synonymous meaning in web development.

I feel like I don't want to go too close to web dev, but I'm not quite sure why 😁 I guess it's because when running not as a web app, there's no URL as such... urgh... naming is hard!

Mostly I've tried to find examples in Flutter to copy. Possibly route is a better option than path, though.

  • validate could be canNavigate

I can go with canNavigate or canActivate.

I like canNavigate, but what should the onValidationFailed property be called? Is there anything similar to this in Flutter currently? 🤔

For me, RouteData conveys enough info - maybe RouteInfo makes the most sense. PathInfo makes sense if you stick with path.

I originally had RouteInfo, but Flutter already has RouteInformation, so I felt it was confusing - I'm not sure I'd even remember which was which. PathInfo might be a good call, though.

StackNavigator Alternatives:

  • PagesNavigator
  • PageStackNavigator

This one is the least obvious to me. I actually had to look at StackNavigator in code to figure out what it was doing. When I think of a stack I automatically think of a Flutter Stack or IndexStack widget rather than the route history. I am assuming that StackNavigator would be used in tandem with PageView/TabView/IndexedStack/Stack - anywhere that you would want to reflect back to the URL using some type of controller. Looking at an example implementation:

Maybe PageStackNavigator or RouteStackNavigator makes the most sense here. I saw a similar pattern in react-router with switches. I could see RouteSwitch since that this would convey that the widget switches the route.

I strongly agree; I really don't like the naming of this. I guess another option would be NestedNavigator to highlight that it's used for nested routes.

chimon2000 commented 3 years ago

I like canNavigate, but what should the onValidationFailed property be called? Is there anything similar to this in Flutter currently? 🤔

onNavigateFailed maybe makes sense. The solution here differs wildly from each library I have seen.
Flutter Modular and Beamer allow you to define a fallback that will be routed to if the guard(s) fail.

/// flutter modular
@override
final List<ModularRoute> routes = [
    ChildRoute(
      '/home',
      child: (context, args) => HomePage(),
      guards: [AuthGuard()],
      /// Route here if guard fails.
      guardedRoute: '/login',
    ),
];

/// beamer
@override
List<BeamGuard> get guards => [
  BeamGuard(
    pathBlueprints: ['/books/*'],
    check: (context, location) => location.pathParameters['bookId'] != '2',
     /// Route here if guard fails.
    showPage: forbiddenPage,
  ),
];

I don't necessarily agree with this pattern because it's a bit inflexible. There are many reasons why a guard might fail and this pattern locks you into a many-to-one error to page redirect pattern. This is also a departure from Angular's canActivate (that flutter modular takes inspiration from) which handles the redirect in the guard function.

Maybe canNavigateFallback or just fallback work here if following this pattern. A similar pattern played out in routemaster might be something like the following:

'/protected-route': (route) => Guard(
    canNavigate: (route, context) => canUserAccessPage() ? null : '/no-access',
    canNavigateFallback:  '/no-access',
    builder: (route, context) => MaterialPage(child: ProtectedPage()),
)

qlevar_router and vrouter have different implementations but the end result is the same: they allow the guard function to return a url to redirect or nothing.

/// qlevar
class AuthMiddleware extends QMiddleware{
  final dataStorage = // Get you Data storage
  @override
  bool canPop() => dataStorage.canLeave;

   /// Route here if guard fails.
  @override
  Future<String?> redirectGuard(String path) async => dataStorage.isLoggedIn ? null: '/parent/child-2';
}

/// vrouter
class Example extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return VRouter(
      debugShowCheckedModeBanner: false,
      routes: [
        VWidget(path: '/login', widget: LoginScreen(login)),
        VGuard(
          /// Route here if guard fails.
          beforeEnter: (vRedirector) async =>
              isLoggedIn ? null : vRedirector.push('/login'),
          stackedRoutes: [VWidget(path: '/home', widget: HomeScreen(logout))],
        ),
      ],
    );
  }
}

The idea of the redirector seems a bit unnecessary compared to just returning a url to redirect. A similar pattern played out here might be something like:

'/protected-route': (route) => Guard(
    canNavigate: (route, context) => canUserAccessPage() ? null : '/no-access',
    builder: (context) => MaterialPage(child: ProtectedPage()),
)

I must admit I'm a bigger fan of the flexibility of this pattern in comparison to the first set of examples, especially when complex routing scenarios come into play. Maybe the best of both words is to change the relationship between Guard and MaterialPage.

var map = {
  '/protected-route': (route) => MaterialPage(
        child: ProtectedPage(),
        guards: [
          Guard(
            canNavigate: (route, context) => isLoggedIn(),
            canNavigateFallback: '/login',
          ),
          Guard(
            canNavigate: (route, context) => canUserAccessPage(),
            canNavigateFallback: '/no-access',
          )
        ],
      )
};

This allows each route to have as many guards as it wants, and each guard to be 1-1.

tomgilder commented 3 years ago

Well, I'm now wondering if we need Guard at all, at least for most scenarios 😁

I'm going to add a NotFound "page" return type (or possibly UnknownRoute), so instead of:

'/book/:id': (route) => Guard(
      validate: (info, context) => booksDatabase.books.any(
        (book) => book.id == info.pathParameters['id'],
      ),
      builder: () => MaterialPage(
        child: BookPage(id: route.pathParameters['id']!),
      ),
    )

You can just do:

'/book/:id': (route) =>
    booksDatabase.books.any((book) => book.id == route.pathParameters['id'])
        ? MaterialPage(child: BookPage(id: route.pathParameters['id']!))
        : NotFound()

Your example above could be written as:

'/protected-route': (route) {
    if (!isLoggedIn()) return Redirect('/login');
    if (!canUserAccessPage) return Redirect('/no-access');
    return ProtectedPage();
},

I think it's worth keeping Guard as an alternative, but I think I'll recommend this as the main way to protect routes.

tomgilder commented 3 years ago

The other option is allowing a null return which defaults to 404:

'/book/:id': (route) {
  if (_canShowPage())
    return MaterialPage(child: BookPage(id: route.pathParameters['id']!));
},

...but I'm not really a fan of this.

chimon2000 commented 3 years ago

Well, I'm now wondering if we need Guard at all, at least for most scenarios 😁

I'm going to add a NotFound "page" return type (or possibly UnknownRoute), so instead of:

'/book/:id': (route) => Guard(
      validate: (info, context) => booksDatabase.books.any(
        (book) => book.id == info.pathParameters['id'],
      ),
      builder: () => MaterialPage(
        child: BookPage(id: route.pathParameters['id']!),
      ),
    )

You can just do:

'/book/:id': (route) =>
    booksDatabase.books.any((book) => book.id == route.pathParameters['id'])
        ? MaterialPage(child: BookPage(id: route.pathParameters['id']!))
        : NotFound()

Your example above could be written as:

'/protected-route': (route) {
    if (!isLoggedIn()) return Redirect('/login');
    if (!canUserAccessPage) return Redirect('/no-access');
    return ProtectedPage();
},

I think it's worth keeping Guard as an alternative, but I think I'll recommend this as the main way to protect routes.

I agree, that given the declarative, React-like nature of Flutter, you can probably get by w/o Guards. Looking at how React Router works

          <Switch>
            <Route path="/public">
              <PublicPage />
            </Route>
            <Route path="/login">
              <LoginPage />
            </Route>
            <PrivateRoute path="/protected">
              <ProtectedPage />
            </PrivateRoute>
          </Switch>

PrivateRoute is just a wrapper that uses authentication to determine whether to return ProtectedPage or a Redirect to login, similar to the example you provide. I am definitely liking your proposed change.