The original code does not support mounting routes with non simple parameters that include regexps. In order to support those I had to adapt the RouterEntry code to hold where each extracted parameter starts and ends.

Example:

test('can mount dynamic routes with regexp', () async {
  var app = Router();

  app.mount(r'/before/<bookId|\d+>/after', (Request request, String bookId) {
    var router = Router();
    router.get('/', (r) => Response.ok('book ${int.parse(bookId)}'));
    return router(request);
  });

  app.all('/<_|[^]*>', (Request request) {
    return Response.ok('catch-all-handler');
  });

  server.mount(app);

  expect(await get('/before/123/after'), 'book 123');
  expect(await get('/before/abc/after'), 'catch-all-handler');
});

@jonasfj – thoughts on this?

I don't think I particularly dislike this specific feature. I just sort of generally would prefer to avoid adding any features.

Routing is a critical feature, introducing unnecessary complexity is not desirable.

(A) what are the use cases for this?

(B) how will it play with code generation? It'll kind of interfere with any future attempts at generating REST API clients and documentation.

(C) Can't it already be done, by using request.change(path: path) in combination with router.all?
I'm not 100% sure, but if possible this would be a great extension method to have in a community package.

Part of me would rather see feature a heavy shelf router be proposed and maintained by the community. Since many of these features are a matter of opinion and many people won't need them.

Part of me thinks shelf_router is better served by staying simple (I'm not entirely sure nested routers was needed at all). This is also partly me admitting that we the Dart team have limited resources.

Curious, how does parameters in mount work, if you have patterns which match beyond /?

Also, won't this pretty much require dynamic creation of a Router which seems like a bad idea. Because every route has a pattern that is converted to regular expression in the constructor.

See:
https://github.com/dart-lang/shelf/blob/master/pkgs/shelf_router/lib/src/router_entry.dart

(This isn't a lightweight operation you should do for every route every time you handle a request).

If we wanted to have patterns in mount, then we should instead add their values to Request.context and require that every route in the nested router takes such parameter as first argument. Or something like that (using properties on the nested router to get the parameters from a zone specific to each request would be nice).

But this is all getting quite complicated.

Moreover it's nice that all routes are evaluated early on, so that issues in patterns throw at the server startup time.

When it comes to structuring a server I would even suggest that nested routers are rarely a good idea. Sure maybe separate /api/ from the rest (but even that might be overkill).

Even if you want multiple routers, why not write the full path pattern in each route, and just concatenate the routers with Cascade or Router.mount('/', ...), to keep things simple.

If you really need more advanced than that, why not build a framework with custom code generation?

(Sorry, if most of my comments question if we need more features 🤣, do note I'd love to see more community maintained shelf packages)

@jonasfj Thanks for the feedback!

(A) what are the use cases for this?

My main use case is organizing related routes, say different routes for a specific user in a composable way or via appropriate isolated classes/functions.

(B) how will it play with code generation? It'll kind of interfere with any future attempts at generating REST API clients and documentation.

That's a good point. I haven't played much with the code generation in shelf_router to answer that question, but the current implementation in the PR holds the extracted parameters information, so an API client generator could access it, if I'm not mistaken.

(C) Can't it already be done, by using request.change(path: path) in combination with router.all?
I'm not 100% sure, but if possible this would be a great extension method to have in a community package.

I would love to see an example of that, maybe it's just me and I'm not seeing the full potential with the current state of shelf_router.

I agree that it would be better to leave things as simple as possible, specially if it's a package maintained by the Dart team and there is not enough bandwith to develop new more complex features.

Maybe @felangel could share more input related to this feature in the context of dart_frog. As far as I know it's not possible to define these kind of routes VeryGoodOpenSource/dart_frog#136. The package is a framework based on shelf and code generation with routes defined by file names (routes/info.dart, routes/users/[id].dart...), similar to NextJS. I like how the routes can end up in single files and it's very easy to navigate in a large code base. If this separation of routes and dynamic routes is possible to do with vanilla shelf and router (no codegen) I would love to see it in action.

I would love to see an example of that, maybe it's just me and I'm not seeing the full potential with the current state of shelf_router.

Look at the implementation of Router.mount can be more or less implemented by Router.all + Request.change.

@jonasfj I managed to simplify the PR code. Now the router entry and param parsing code is untouched.

What I believe is not possible to do with Router.all + Request.change is accessing the parameters list. The request provides the Map of values, but not the list of parameter names in order. This PR essentially handles that for you internally.

Request.change cannot receive a path with unresolved path parameters, otherwise the shelf package code will fail. It needs to be a plain and simple URL path. That is why in the PR code the RouterEntry is included as an extra parameter for all when we are handling a mounted route.

Maybe there is another way to do it? I thought about including the list of parameters to the context, but given that the map of parameters is already available through the context I thought it would be better to keep the list itself invisible to the outside, same as now.

Can't you do something like:

final app = Router();

app.all('/user/<user>/<rest|.*>', (request, user, rest) {
  final subrouter = createSubRouter();
  return subrouter(request.change(path: 'user/$user/'));
});

I'm not saying it's pretty, but it can be done in an extension method (maybe there is a few corner cases missing here), like adding additional patterns to handle trailing /. But overall something like this might work.

That said, it's still horribly slow to do this.

@jonasfj Ok I see what you mean now, thanks for the snippet!
Yes, I can see how creating a subrouter every time a request is handled would be very wasteful.
Do you see any way of resolving that so that a composable router is created at server start time instead?
Maybe it could look like this:

var app = Router();

final Handler Function(String) usersHandler = createUsersHandler();

app.mount('/users/<user>', (Request request, String user) {
  final handler = usersHandler(user);
  return handler(request);
});

But I'm not sure how createUsersHandler would be implemented in order to return that function definition.

Two ideas:

final userRouter = Router();
userRouter.get('/posts/<postId>', (request, userId, postId) {
  ...
});

final app = Router();

app.mount('/user/<userId>/', userRouter);

This is not crazy, but perhaps super easy to do, and not easy to understand or follow.

This would fairly easy to support, but also involve a lot of typying:

final userRouter = Router();
userRouter.get('/posts/<postId>', (request, postId) {
  final userId = param(request, 'userId');
  ...
});

final app = Router();

app.mount('/user/<userId>/', userRouter);

This would require use of zones and be rather magical:

part 'userservice.g.dart'; // generated with 'pub run build_runner build'


class UserService {
  String get userId => Router.param('userId'); // this can only work if Router start using zones

  @Route.get('/posts/<postId>')
  Response listUsers(Request request, String postId) {
    // can access userId from within the context of a request.
    ...
  }

  Router get router => _$UserServiceRouter(this);
}

final app = Router();

app.mount('/user/<userId>/', userRouter);

I think these are fun ideas for making better logic, but I think it should live outside of shelf_router, maybe a community package called shelf_super_router or something.

@jonasfj
Thanks for the ideas!
Unfortunately all of those depend on mount supporting parameters, which it doesn't at the moment.

This throws an internal error in shelf because the URL path shelf receives contains unexpected tokens (<, >...)
app.mount('/user/<userId>/', ...);
Supporting those parameters is essentially what this PR is trying to solve.

@jonasfj I've modified the PR in 1a7d4dc so that Routers are not created inside the handlers. For that to work, the parameters extracted from the mount prefix need to be accessed from the context of the request.
I opted to save them in a different context variable so that request.params only returns the parameters from the route it is being defined.
If merging all the parameters in a single Map is preferred, I could change that.

These changes allow to create the routes similar to the second code snippet in your previous comment.

void main() {
  Router createUsersRouter() {
    var router = Router();

    String getUser(Request r) => r.mountedParams['user']!;

    router.get('/self', (Request request) {
      return Response.ok("I'm ${getUser(request)}");
    });

    router.get('/', (Request request) {
      return Response.ok('${getUser(request)} root');
    });
    return router;
  }

  var app = Router();

  final usersRouter = createUsersRouter();
  app.mount('/users/<user>', (Request r, String user) => usersRouter(r));
}

There is another question and that is if the function passed to mount should include or not the parameters.
I prefer them to be passed because that way it works the same as the other methods (get, post, all...)

There is another question and that is if the function passed to mount should include or not the parameters.
I prefer them to be passed because that way it works the same as the other methods (get, post, all...)

What would be the alternative? It would be nice imo to have both the mount handler as well as nested route handlers (from sub-routers) receive the params.

What would be the alternative? It would be nice imo to have both the mount handler as well as nested route handlers (from sub-routers) receive the params.

Yes, having them both ways would be my preferred way as well.
The alternative would be just having them through the context, which is what the previous snippets showed.

@jonasfj Is there anything else that you see it could be improved in the PR in order to get merged?
Unfortunately at the moment is not possible to build additional functionality around routing/mounting via extensions/packages without making some small changes into shelf_router

@jonasfj ?

@jonasfj is it possible to take another look at this PR?