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

☂️ API naming #19

Closed
8 tasks done
tomgilder opened this issue Apr 6, 2021 · 7 comments · Fixed by #89
Closed
8 tasks done

☂️ API naming #19

tomgilder opened this issue Apr 6, 2021 · 7 comments · Fixed by #89

Comments

@tomgilder
Copy link
Owner

tomgilder commented Apr 6, 2021

StackNavigator

  • Class name - alternatives PagesNavigator or PageStackNavigator

Guards

  • validate could be canNavigate
  • onValidationFailed would need to change to

Delegate

  • routesBuilder could be mapBuilder, builder or routeMapBuilder

RouteData

  • Class could be called PathInfo
  • path - could be route or url

Current plan for 0.8.0

  • Rename StackNavigator to PageStackNavigator
  • Deprecate guards, but rename validate to canNavigate and onValidationFailed to onNavigationFailed for anyone who really wants to use them
@tomgilder tomgilder changed the title API naming ☂️ API naming Apr 6, 2021
@tomgilder tomgilder added this to To Do in Project board Apr 10, 2021
@tomgilder tomgilder removed this from To Do in Project board Apr 24, 2021
@theweiweiway
Copy link

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

@chimon2000
Copy link

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 tomgilder added this to the 0.7.0 milestone May 2, 2021
@tomgilder tomgilder added this to In Progress in Project board May 2, 2021
@tomgilder
Copy link
Owner Author

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
Copy link

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
Copy link
Owner Author

tomgilder commented May 3, 2021

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
Copy link
Owner Author

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.

@tomgilder tomgilder modified the milestones: 0.7.0, 0.8.0 May 3, 2021
@chimon2000
Copy link

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.

@tomgilder tomgilder linked a pull request May 9, 2021 that will close this issue
@tomgilder tomgilder moved this from In Progress to In Review in Project board May 9, 2021
@tomgilder tomgilder moved this from In Review to Done in Project board May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
API Design
Awaiting triage
Development

Successfully merging a pull request may close this issue.

3 participants