Add support for named routes

[sjanuary]

There have been a couple of requests in the Express project for supporting named routes, expressjs/express#2746 and expressjs/express#2739. This PR enables a name to be used when a route is created via Router.route. Routes with names can be looked up via a new method - Router.findRoute. The name argument is optional so no existing functionality should be broken. A test is also included.

[fourpixels]

I can't believe my eyes! Great job! 👏

Here are few thoughts to consider though:

• Routes are unique per Router. Which means it's okay to have two Routes with the same name withing different Routers. I know it's because the whole logic is within the Router, and I never got to decide if that's good or not. It's independent, yes, but still hides huge risks if you're not using just the default Router.

• There is no way to reverse the url, which is why all this is needed. Currently the regex is inside the Layer, but there should be some functionality to easily do something like:

  router.route('/users/:param1', 'nameToRoute');
  var url = router.reverse('nameToRoute', { param1: 'value1' }); // -> '/users/value1'

  This functionality is much needed in views - I don't understand how it's missing. And that's the core issue with having anonymous Routes.

• Names are the first step, but are still not enough IMO. Even if you get the router by name and have the reverse functionality I described above, it still not enough because:

  app.use('/some/path', new Router());

  Which means that everything the Router knows about itself is not enough to build a proper path, without messing around with Express. That's why I sometimes think fixing just Router is not enough - Express might need some centralized storage of all routes.

So thanks a lot for trying to fix things up, I'm pretty glad someone is doing the effort. And I hope this is the beginning of some discussion about how to really fix it!

Thanks!

[sjanuary]

Routes are unique per Router. Which means it's okay to have two Routes with the same name withing different Routers

Yes, it would be difficult to prevent this in Router as far as I can see as Routers don't know about each other.

There is no way to reverse the url, which is why all this is needed.

I understood from expressjs/express#2739 (comment) that it would be problematic to add a reverse function to the API due to the fact that middleware can be added that can change the URL, so the result of reverse would actually be incorrect. The idea of this PR was that named routes should make it easier for developers to add reverse functionality themselves 'at their own risk'. As you said, adding anything else to the Router or Express API requires further discussion and thought.

[wesleytodd]

simpler to just check if (name && .... IMO that is also more explicit about the intention of this code.

[blakeembrey]

Ok, some brief thoughts on functionality like this (I do like it, just considering how it can be improved). Firstly, there's a big issue with nested routers here - I think a lot of people do use nested router functionality. If we're going to expose this level of functionality, I'd love to support as many people as possible. Also, it's possible to implement reverse functionality since I have support for it in path-to-regexp. That said, it can't reverse non-string paths.

So, thoughts: What if finding routes by name was just a map instead of an iteration of the stack? Now, what if we also restricted the key to a subset of characters (same as path-to-regexp keys)? Once we have that, is it possible that we can expose functionality like reverse using a nested key string? Something like Router#getPath('users.userId', { userId: 123 }). That's what most people actually need, and if they just want to get a route by name they can just do router.routes.users.

Edit: I believe this would resolve what @fourpixels is asking for. Of course, the path can only be built from the ground up (if you build it from the nested route, it won't know the path to child routes). On top of this, I think Router#use will also need to be name-able.

[fourpixels]

I was just about to say that use is also part of the deal :)

One questions - where do you think this map should be in? Just inside the Router? If so, you would use the default router on which everything is mounted, in order to build a path to the route you want? Or am I missing something? :)

@sjanuary don't get me wrong - I just think there should be a little bit more things changed so that this starts actually working for everyone. Adding just a name and leaving people still implementing their own version of reverse routing won't solve the problem, but it's the beginning of the journey.

[sjanuary]

I like the idea of using a map, being able to do router.routes.users is nice, although router.use doesn't create a route so how those two things work together needs some thought. Maybe the map isn't called routes and you get something else back if use was called. Or maybe you just get nothing.

I'm hearing that the reverse functionality is important so I'll give that some thought, probably next week sometime.

[fourpixels]

I can't think of any other good reason doing all this, and having named routes, except backtracking and referring to them.
Maybe @dougwilson could give us an inside help how to make this work out the easiest way possible? :)

[dougwilson]

There was actually a bit of discussion about the pull request in the Express.js community hangout last night :)

Here are some of my main high-level thoughts on the pull request, as it is today:

1. For a new feature, there are definitely not enough tests, not even enough to get all lines covered in the code, which is a pretty low bar (since line coverage is not a good indication of code coverage). Let's work on getting a full suite of tests for all the different aspects of this feature and various edge cases.
2. There is no documentation added to the README.
3. As touched on already, the changes at this point ay be a bit "cart before the horse", unless I'm missing something. For example, given the code in this pull request, what is the problem set that will be solved with these changes? Can you provide a code example of how a real-world user may be using this feature to implement something?

Besides those three things, I would re-iterate the thoughts already made, adding that I think a lot more throught needs to be put into the design, especially how the feature will interact with the other current features of the router and how to a possible implementation will meet some kind of goal for users to use.

All that said, nice work!

[wesleytodd]

To speak a bit to the "use cases" that this feature is good for:

When writing api's it is a common practice to return a set of links, in our apps we do this:

{
  "_links": {
    "self": "https://www.stream.me/api-live/v1/streams?limit=20&offset=0",
    "next": "https://www.stream.me/api-live/v1/streams?limit=20&offset=20"
  }
}

It would be nice to be able to do something like this:

res.json({
  _links: {
    next: url.format({
      // ...
      path: router.routes.streams.reverse({/* ... */})
    })
  }
});

That is the main use case I can think of for this feature that I would use. Obviously the actual semantics would be up for debate, if it is router.routes.<route name>.reverse or something else, but that is the general idea. In my example it is the current route I want the link for, but there are many other cases where I might want to link to another api resource, for example if a stream had a user who owned it I might do:

res.json({
  _links: {
    owner: url.format({
      path: router.routes.owner.reverse({/* ... */})
    })
  }
});

I think it would also be helpful for this to expose something more like:

req.route.reverse();

This probably has a ton of other issues because things can modify the route and what not, but it would be nice.

Is that helpful and/or what you all where thinking of this for?

[blakeembrey]

@wesleytodd The reason I suggest a method called #getPath(routePath: string, params: Object) is because using reverse on the router itself has zero context. A router can be deeply nested inside other routers, so reversing only a single layer is of limited use here too.

[wesleytodd]

I like the sound of that api, my previous comment was more focused on the use case than the api specifically. But...

The issue I see with the api you mentioned is that if you change where a router is mounted, then my examples above also have to change everywhere I use the routePath from your api. Which doesn't give me a win over what we have already by just using pathToRegexp.compile. Not that this is a huge problem, but the feature as it is outlined here is a bit cleaner user experience than having to share the routePath around the app.

To outline the problem more clearly, here is using the api in the PR method:

var routerA = new Router();
var routerB = new Router();

var r = routerB.route('/some/:thing', 'foobar')
r.get(function (req, res) {
  // The issue here is that routerB needs to know that it was mounted
  // on routerA in order to jam the paths together, which a route does not know.
  res.json({
    link: routerB.routes.foobar.reverse({path: 'foo', thing: 'bar'}) // /some/bar ??
  });
});
routerA.use('/base/:path', routerB);

With the api you are proposing, correct me if I am wrong, we would need to hard code the full paths in all the response places:

var routerA = new Router();
var routerB = new Router();

var r = routerB.route('/some/:thing');
r.get(function (req, res) {
  // For sure knows the right path, but if we were to mount somewhere else we would
  // have to change all these, and also is unfriendly to use because the whole point
  // of using sub routers is to encapsulate functionality without knowledge of how it 
  // all might come together.
  res.json({
    link: getPath('/base/:path/some/:thing', {path: 'foo', thing: 'bar'}) // /base/foo/some/bar
  });
});
routerA.use('/base/:path', routerB);

If I understand that issue correctly from above, then it seems like we could come up with a hybrid of the two apis with a few simple things to give the correct context. The main thing would be telling the router where it was mounted in a parent app/router. We could do something similar to what express does for mounted apps.

[blakeembrey]

@wesleytodd Please re-read my initial comment. It did not suggest just replicating the routes, but instead using a traversal of the names you've used to generate the path. E.g. users.userId would traverse through app.routers.users.routers.userId building the complete path. This is the main issue with your proposal to use reverse directly on routers - it neglects the nested router use-case. The only way to solve this using your proposal is to keep an array that you push and pop as your traverse the routers to keep context, but you'd then need to use a backtracking method like ../userId to get to paths on other routers.

[blakeembrey]

To go further, yes, you've got the right use-case. I'm don't have any other use-cases in mind. I'm only commenting on the application of the use-case.

var routerA = new Router();
var routerB = new Router();

var r = routerB.route('/some/:thing', 'thing');
r.get(function (req, res) {
  res.json({
    link: routerA.getPath('routerB.thing', {path: 'foo', thing: 'bar'}) // Also possible: `routerB.getPath('thing')` but the path is now shorter and not complete. You'd need a reference to the "root" of where you're generating the path from.
  });
});
routerA.use('/base/:path', 'routerB', routerB); // Use the name we're proposing to add here already.

[fourpixels]

I understand your point and you're totally right about all the things. But the one I'm most afraid of is

You'd need a reference to the "root" of where you're generating the path from

If we think of an Express kind of app (just for a second), and the most common use case - app.route(), then we have to really get any path straight from the beginning. You miss the start - you miss the end :)

To me it seems more like a workaround, with a bit nota bene sign: keep in mind to reverse the route from the main router, or it won't work. Unfortunately I can't think of any other possible solution, as I understand the Router should not care where it's mounted.

[wesleytodd]

@blakeembrey I miss-understand your original comment, also I made my last comment on github mobile, which makes it very hard to read the context from the rest of the conversation. Sorry :)

That last code example made way more sense to me and now I get it. I still think it is not optimal to have to have access to routerA inside the route in routerB. Could we do something like what the express basePath thing does? When a router is mounted it keeps its parent and concats the paths together when you request a path. I can write code examples for this if you want and it would make it more clear.

[blakeembrey]

@fourpixels Yeah, it's absolutely a workaround. The current model of the router doesn't allow for something else. Of course, we're allowed to imagine a world where the router is a different beast, but that's for another module 😄

@wesleytodd All good, I was on mobile too and get lazy typing, so that's why the response was super short. Should really wait to reply properly. Yes, your proposal is possible (it's actually what I mentioned in my last comment) by using an array and pushing and popping as you move along. The only issue is that you then need to use relative links to other routers to get the full path of another router.

[sjanuary]

This is still very much a work in progress (loads more tests needed), but I did check in some changes I made today partly because I wanted to discuss the possibility of updating path-to-regexp. I needed to update to use compile, but it did break one of the existing tests unfortunately so I guess that's a problem going forward...

[dougwilson]

I wanted to discuss the possibility of updating path-to-regexp. I needed to update to use compile, but it did break one of the existing tests unfortunately so I guess that's a problem going forward...

Yea, there is various discussion around this, including in the Express.js repo itself. The dependency is already at the max version before breaking changes, so it can (and will) be upgraded, but can't until Express 5.0 (version 2.0 of this module). That is definitely OK to do, though it's really a task in itself. I recently shared more information about what we need to do around getting it updated in expressjs/express#2057 (comment)

[sjanuary]

Cool, I didn't know there was a work in progress label! The failing test when I updated path-to-regexp was this one, which I just commented out for now in this PR.

[blakeembrey]

@sjanuary Yes, that test has been noted before: pillarjs/path-to-regexp#57. This functionality would need to be 2.0 - which has a few other PRs waiting too 😄

[dougwilson]

This is a little too liberal, due to the flatten above. For example, this would allow for router.use([[[[function(){}]],'name']]) and pick up the name, when it probably needs to be restricted to being a top-level argument.

[dougwilson]

Also keep in mind we should be able to describe the arguments in TypeScript, since that will be coming to these modules soon.

[blakeembrey]

I'd go for the route name as the second parameter, for the same reasons @dougwilson has mentioned above. The middleware arity is infinite so it's not possible to type this interface (easily).

[fourpixels]

Can't we just shift them? I'm interested how this is going to be described in TypeScript :)

[blakeembrey]

Shift which? For TypeScript, it'll look like:

type Middleware = (req: any, res: any, next?: (err: Error | string) => any) => any;

function use (...middleware: Middleware[]): this;
function use (path: string, ...middleware: Middleware[]): this;
function use (path: string, name: string, ...middleware: Middleware[]): this;

However, with the name at the end, it looks a little inconsistent. I did realise that since it only accepts a single middleware function, it's less of an issue, but seeing the signatures like this does make it more obvious.

function use (path: string, middleware: Middleware, name: string): this;

[sjanuary]

I'd go for the route name as the second parameter, for the same reasons @dougwilson has mentioned above. The middleware arity is infinite so it's not possible to type this interface (easily).

If only one string parameter is passed how do you tell if it's the path or the name? The TypeScript above looks like you insist on a path being used if you want to use a name, which I could implement if that seems like a better choice. The current implementation was designed to allow:

function use (middleware: Middleware, name: string): this; 

[dougwilson]

Hi @sjanuary, I took a look for a bit and gave some initial comments, but I have to move on to something else for the time being, so will finish looking again probably tomorrow. Please add documentation for your new functionality to the README. I know it may not be finished yet, but documentation in the README and really helps me understand what it is supposed to do by being able to an English description of what to do and looking at an example of it's use in the example section.

[sjanuary]

Thanks for all the comments @dougwilson. I've replied to a couple inline and for the rest I will make the changes you suggested.

[blakeembrey]

Another note about the format, but it effectively breaks the work I've done over the last year with #29 and previously #13 by allowing router extensions. Should this be considered here?

[sjanuary]

@blakeembrey - Is #29 a definite for 2.0? If so, I could rebase if/when it is merged although I can see it's a really big change so it might not be straightforward. I would expect that if this PR is accepted then anyone extending the router would also have to implement findPath so that would need to be clearly documented.

I'm just working on an example for this, but I think I've implemented all the other comments so far.

[blakeembrey]

It's up to @dougwilson, but I really hope so because I've had the PR open for over a year now and there's two other router implementations waiting for this.

The issue is if the router subclassing gets merged, then it makes your API changes hard to do because subclassed routers can have more than one "path" argument. That's why I suggested thinking about the API a little more first, a simple solution would be var named = router.name('x'), but it changes the implementation a lot.

[sjanuary]

@blakeembrey I will look into it if @dougwilson can confirm that #29 is definitely going in. Maybe it could be implemented as a subtype instead of changing the API for the base Router, which would allow it to be stricter on number of path arguments etc without compromising other use cases.

[kokujin]

Is there a status on this? I really would like to use named routes in my application

[sjanuary]

@kokujin - this does work if you would like to try it out, but there's a conflict with another pull request #29 in terms of design, so I don't think it's likely to be accepted in the current state unless the other pull request is rejected.

[edevil]

Is there any news regarding this? It seems neither this nor #29 got much traction. Is there something that can be done to help?