Adds routePrefix feature and allRoutes getter generator for router and routes classes

[nateshmbhat]

What it does :

• Adds route prefix option that can be specified for Router class so that each route in the generated Route class is prefixed by this route.
• Adds a way to access all the generated routes in the Router class by generating a getter method for the Router.
• Fixes some issues regarding Function parameters in the Route Class.

Fixes Issues :

#88
#91
#90

Results based on this code :

This is my $Router class :

$Router class

@MaterialAutoRouter(routesClassName: 'BusRoute', generateRouteList: true , routePrefix: '/bus/')
class $MyBusRouter{
  BusFilterScreen busFilterScreen ;
  BaseLoadingScreen baseLoadingScreen ; 
}

Generated Class :

abstract class BusRoute {
  static const busFilterScreen = '/bus/bus-filter-screen';
  static const baseLoadingScreen = '/bus/base-loading-screen';
  static const all = [
    busFilterScreen,
    baseLoadingScreen,
  ];
}

class MyBusRouter extends RouterBase {
  @override
  List<String> get allRoutes => const [
        BusRoute.busFilterScreen,
        BusRoute.baseLoadingScreen,
      ];

  //This will probably be removed in future versions
  //you should call ExtendedNavigator.ofRouter<Router>() directly
  static ExtendedNavigatorState get navigator =>
      ExtendedNavigator.ofRouter<MyBusRouter>();

  @override
  Route<dynamic> onGenerateRoute(RouteSettings settings) {
    final args = settings.arguments;
    switch (settings.name) {
      case BusRoute.busFilterScreen:
        if (hasInvalidArgs<BusFilterScreenArguments>(args, isRequired: true)) {
          return misTypedArgsRoute<BusFilterScreenArguments>(args);
        }
        final typedArgs = args as BusFilterScreenArguments;
        return MaterialPageRoute<dynamic>(
          builder: (context) => BusFilterScreen(
              key: typedArgs.key,
              tripId: typedArgs.tripId,
              filterScreenManager: typedArgs.filterScreenManager),
          settings: settings,
        );
      case BusRoute.baseLoadingScreen:
        if (hasInvalidArgs<BaseLoadingScreenArguments>(args)) {
          return misTypedArgsRoute<BaseLoadingScreenArguments>(args);
        }
        final typedArgs =
            args as BaseLoadingScreenArguments ?? BaseLoadingScreenArguments();
        return MaterialPageRoute<dynamic>(
          builder: (context) => BaseLoadingScreen(
              key: typedArgs.key,
              showAppBar: typedArgs.showAppBar,
              appBarTitle: typedArgs.appBarTitle,
              showThreeDotMenu: typedArgs.showThreeDotMenu),
          settings: settings,
        );
      default:
        return unknownRoutePage(settings.name);
    }
  }
}

Regarding issue #90

I will raise a separate PR to convert the List getter into a Set getter for optimisation purpose.

Thank you for this awesome library 😄
will try to add a PR for fixing issue #91 , but I have no idea how I might fix it . May be you can help me here :)

[Milad-Akarie]

Hey, Nate! good job man. I was thinking some people might wanna chip in at this point so I was busy trying to add tests lately.

Anyways everything looks fine except for one thing
Initial routes should be excluded and always be named "/" otherwise weird things will happen. what do you think?

[nateshmbhat]

I knew there might be something that I am missing when i saw that initial route condition 😅
anyway , we can try either of 2 things :

• just add a final check to the route after filling the routeConfig and if its an initial route , change the path back to '/' . I suppose this would fix it ?
• make routePrefix to default to "/" and if its initial route , we won't add the field.name . But i don't think this is a good way since it has some more ambiguity.

[Milad-Akarie]

I'd default the prefix to '/' so we would keep the expected behavior
I rearranged my code and added your line

 _routes.forEach((r) {
      final routeName = r.name;
      if (r.initial == true) {
        _writeln("static const $routeName = '/';");
      } else {
        final preFix = _routerConfig.routePrefix;
        final pathName = r.pathName ?? "$preFix${toKababCase(routeName)}";
        _writeln("static const $routeName = '$pathName';");
      }
    });

I don't think we should add the prefix to custom path names, what do you think?

[nateshmbhat]

I don't think we should add the prefix to custom path names, what do you think?

ohh right. i agree . i absolutely forgot about custom path names there 😬
that code looks perfect 👍 ✅

[nateshmbhat]

also could we give an option to user to turn off that kabab case conversion ? 🤔
there may be use cases where user doesn't want kabab case for all his routes and he doesn't wanna specify custom names for every single route in the router.

you think its good to provide this flexibility to the user 🤔

[Milad-Akarie]

We added the kabab case conversation so the route names are web friendly I don't think it would make any difference for native users.

[Milad-Akarie]

about this bit

@override
  Set<String> get allRoutes => const {
        BusRoute.busFilterScreen,
        BusRoute.baseLoadingScreen,
      };

can't it be

@override
  Set<String> get allRoutes => BusRoute.all;

[nateshmbhat]

We added the kabab case conversation so the route names are web friendly I don't think it would make any difference for native users.

Yes sure , makes sense 👍

about this bit

@override
  List<String> get allRoutes => const [
        BusRoute.busFilterScreen,
        BusRoute.baseLoadingScreen,
      ];

can't it be

@override
  List<String> get allRoutes => BusRoute.all;

I didn't do that because user may not have specified that generateRouteList flag in the router config :)

[nateshmbhat]

so any attempt to access that all getter would error out.

[Milad-Akarie]

so any attempt to access that all getter would error out.

ya I thought we'd default the getter to return an empty Set/List or throw an informative exception like you need to set generateRouteList option to true

[nateshmbhat]

hmm 🤔
I still think its good to add the allRoutes getter to the Router class regardless of the generateRouteList flag value.

The reason is when user is Pushing the Route , he will be accessing the Routes class and having a seperate unrelated route member called ("all") may not be what he wants.

So there's a slight inconvenience here.

On other hand adding the getRoutes to Router will not affect the user in any way. Even in terms of usage , he won't be using the Router class instance directly.

Even if he uses it , it would just have 2 members to deal with.

When we are using multiple modules in flutter , it's crucial to have this getter in Router. But sometimes we may not want it in the Routes class (since it would just get in the way of accessing other routes)

[Milad-Akarie]

So you're saying move all routes getter from Routes to RouterBase?

[nateshmbhat]

So you're saying move all routes getter from Routes to RouterBase?

its still good to keep it in Routes class because some users may want to access the static method.

It would act as an interface method when we have a list of Routers or any other use case which would require us to store all these routers under a common base class :

List<RouterBase> myRouters ; 
...

myRouters.forEach( (router) => print(router.allRoutes) );

[nateshmbhat]

Do you agree with the points mentioned ? :)

[nateshmbhat]

We can't make that method static anyway inside the router class because it won't serve the purpose anyway.

I was kinda confused there when I said we need a static method in router.

I see no other way than just having an instance method getter allRoutes and allow the user specify if he wants "all" method in Routes class which is the current behavior.

So in essense what I am saying is , we can keep the changes that are in this PR.

[Milad-Akarie]

@nateshmbhat I understand what you're trying to say but it's still confusing to have the generateRoutesList flag effect the routes class only and generate the list inside of the router anyways.. don't you think?

[nateshmbhat]

hmm , you definitely have a valid point ✔️
i'm fine with making that generateRoutesList flag add that getter method to both Router and Routes class or just the Router class.
It's your call 😄

[Milad-Akarie]

I guess that would do. Don't forget to throw an informative exception if users call get allRoutes without setting the generateRouteList flag to true

also we're going for this right?

@override
 Set<String> get allRoutes => Routes.all; 

[nateshmbhat]

Yes definitely 👌👌💯

[nateshmbhat]

Will you be making the changes to this PR itself ? :)

[Milad-Akarie]

ya it would be great if you updated it!
again thanks for your efforts

[nateshmbhat]

OK here are the things I would be updating :

Check if generateRouteList is true in the getRoutes method. And only return the list if it's true in both the router and routes classes.

Change the list to Set.
So would change the name to generateRoutesSet

Do that prefix change that we agreed to.

[nateshmbhat]

This is it right ?

Regarding the getter for routes. Tbh I still am not that convinced that its good to show error when accessing the routes getter from router class.

[nateshmbhat]

Imagine a user accessing the router object and getting code completion for accessing the getter method only to end up with an error that he needs to set that flag.

All I'm saying is, there's no harm in adding this method to router. But there's a little harm in adding this getter to the routes class ( harm is that user may not need to see that "all" route) when he uses his routes class or may he already has a route that has the name "all".

There's also a harm in fully removing that getter from Routes class because user may not want to create and store router object just to access all methods.

So basically there's no harm in giving the getter method to router.

But in Routes class, there's harm whether we give getter there or not. So depending on what user wants, he can set the generateRouteList flag.

[nateshmbhat]

As for as your concern that it may raise ambiguity to the user, simply saying "this flag will generate an additional routes list member in the Routes class" with an example in the documentation should be enough.

Again, your call though 😄 😄

Let me know about it , so that i can push the final changes.

[Milad-Akarie]

hmmm I'm not sure, a safer approach would be if we always generate the routes lists. The flag was added back when route names were generated in the router class but now that they have their own I guess we could get rid of the flag?

[nateshmbhat]

Yes, that seems like a reasonable solution. I agree that it's better to get rid of the flag itself.

[Milad-Akarie]

@nateshmbhat let me know when you're done.

[nateshmbhat]

Sure, I'm also looking into that function parameter issue. Let's see if I can add a fix for that along with the discussed changes.

I have also thought of making a video regarding this library on my youtube channel.

[Milad-Akarie]

Sweet! I'm working on adding tests so I can proceed with improvements with confidence.

[nateshmbhat]

@Milad-Akarie hey is there some way to set break point and debug things when the build runner is running ? I'm trying to look into that Function parameter issue.

[Milad-Akarie]

@nateshmbhat unfortunately, I failed to do so but I happen to know the fix to this issue. it's pretty simple actually, it seems that analyzer doesn't create an element for typedefs or typed functions so adding a nullability check on element in the below function will solve the problem

file: lib/src/route_paramter_config.dart

 Future<String> _resolveLibImport(Element element) async {
    if (element?.source == null || isCoreDartType(element.source)) {
      return null;
    }

...

[nateshmbhat]

Yes I got that null check issue. But what I badly need is a support for Function parameters too.

Do you have any hint or idea that can help us solve this ?

I was trying to look into various parameters of the parameterElement. But so far, no luck about getting the element object to pass to your import method.

[nateshmbhat]

Let's say the function has some params of some types. These types need to be imported too.

Right now the library isn't doing it.

[Milad-Akarie]

I totally missed that bit. hmmmmm if can get it as an ExcutableElement than we can simply access the returnType and parameters

[nateshmbhat]

Please review the changes.

[nateshmbhat]

Here's a sample test of the changes :

@MaterialAutoRouter(
    routesClassName: 'BusRoute', routePrefix: '/bus/')
class $MyBusRouter {
  BusFilterScreen busFilterScreen;

  @initial
  BusFilterScreen busFilterScreenNew;

  BusBoardingDropScreen busBoardingDropScreen;
}

Generated :

abstract class BusRoute {
  static const busFilterScreen = '/bus/bus-filter-screen';
  static const busFilterScreenNew = '/';
  static const busBoardingDropScreen = '/bus/bus-boarding-drop-screen';
  static const allRoutes = {
    busFilterScreen,
    busFilterScreenNew,
    busBoardingDropScreen,
  };
}

class MyBusRouter extends RouterBase {
  @override
  Set<String> get allRoutes => const {
        BusRoute.busFilterScreen,
        BusRoute.busFilterScreenNew,
        BusRoute.busBoardingDropScreen,
      };
...
}


class BusFilterScreenArguments {
  final Key key;
  final Int64 tripId;
  final FilterScreenManager<BusSortFilterResponse> filterScreenManager;
  BusFilterScreenArguments(
      {this.key, @required this.tripId, @required this.filterScreenManager});
}

//BusBoardingDropScreen arguments holder class
class BusBoardingDropScreenArguments {
  final Key key;
  final Int64 tripId;
  final PaginationRequest paginationRequest;
  final void Function(LocationItem) onSelected;
  BusBoardingDropScreenArguments(
      {this.key,
      @required this.tripId,
      this.paginationRequest,
      @required this.onSelected});
}

In the generated item final void Function(LocationItem) onSelected the LocationItem's import is not happening as you already know.