[Proposal] Route helper for view

[RomainLanz]

Dear everyone,

Initial issue: #3151

It looks like Sails doesn't provide a way to generate URLs for routes. It means, that developer needs to provide URLs in several places of the application (in route configuration, in links, in scripts, etc). This dramatically breaks the DRY principle. I do believe that URL generation is a very important aspect of a router component for any solid web framework.

Example

// File: config/routes.js

module.exports.routes = {

    // ...

    'GET /':        { name: 'home',     controller: 'PageController.home'  },
    'GET /about':   { name: 'about',    controller: 'PageController.about' },

    // ...

}

// File: Any kinds of view file

<a href="<%= route('home') %>">Home page</a>
<a href="<%= route('about') %>">About page</a>

<!-- Outputed HTML -->

<a href="www.example.com">Home page</a>
<a href="www.example.com/about">About page</a>

Additional Information

• Laravel Route Helper - Laravel Named Route
• Symfony Routing

cc @slavafomin @tjwebb

[dsazup]

I was gonna submit a question here if this is already implemented as I could not find anything on google. I've been using laravel so I know how helpful this route function is. I would love to see this on sails.

[mikermcneil]

@Mirago @RomainLanz I've taken a pass at implementing this a couple of times in the past, and I always run up against the issue of explicit routes vs implicit routes. It would be quite simple to make this work against explicit routes (i.e. in your routes.js file), but more of a design problem to make it work for implicit routes (i.e. factoring in blueprint routing). That said, I recognize the usefulness of this, and if there's across-the-board interest here (for explicit routes), I'd like to take care of this in 0.12.

Here's a proposal:

Technical Summary

This would be a method on the app object (sails) and exposed by the controllers hook rather than specifically reserved for use as a view helper; i.e. so you can also use it programmatically in your controllers & services. We can always add a specific view helper later that gives you just the URL pattern part of the route address (no HTTP method), but I'd rather start lean. Also note that below I omitted the base URL-- in my experience, it's best to manage the base URL for your application through configuration (there are just too many different setups across hosting providers, and attempting to infer the domain/protocol/port automatically in the past has just led to lots of confusion amongst Sails users).

Usage

Common:

e.g.

sails.getRouteAddress({ target: 'PageController.about' });
// =>
// {
//   method: 'get',
//   url: '/about'
// }

---

For non-GET routes:

e.g.

sails.getRouteAddress({ target: 'WolfController.howl' });
// =>
// {
//   method: 'post',
//   url: '/wolves'
// }

---

For route addresses with no method specified:

Method is returned as empty string; e.g.

sails.getRouteAddress({ target: 'MeController.logout' });
// =>
// {
//   method: '', 
//   url: '/logout'
// }

---

URL pattern variables:

e.g.

sails.getRouteAddress({ target: 'PageController.projectDashboard' });
// =>
// {
//   method: 'get',
//   url: '/:organizationSlug/:username/projects/:projectSlug'
// }

---

URL wildcard suffix:

e.g.

sails.getRouteAddress({ target: 'INodeController.upload' });
// =>
// {
//   method: 'put',
//   url: '/:username/tree/*'
// }

---

Would that do the trick?

[niallobrien]

What's the status on this?

[mikermcneil]

@niallobrien stewing in the old noggin at the moment-- was hoping to get some feedback on the proposal there. Whatcha think?

[niallobrien]

@mikermcneil I'm liking it, though the view helpers would need to follow pretty quickly imo. @RomainLanz What's your opinion?

[RomainLanz]

Seems good to me.

[mikermcneil]

@RomainLanz @niallobrien K added here: https://github.com/balderdashy/sails/blob/master/ROADMAP.md#backlog

It occurred to me that we still haven't clarified a few exit conditions:

1. invalid usage - e.g. if no target key is provided, or it is provided as a non-string (the obvious thing to do is to throw here, but it's important to call that out specifically-- we do not want that to fail silently)
2. ambiguous usage - if a target key is provided which does not contain a ., and therefore corresponds to a controller (the easiest thing to do here is to treat it as meaning the index action of whatever controller was specified, without checking that the controller exists. However, the right answer is probably to throw a usage error if there is no .)
3. unrecognized target - if a target key refers to an unknown controller or action... (in this scenario, we need to either throw an error if the controller or action is unrecognized, or opt to converge/fail silently and return null. In general, I tend to prefer the former approach, and particularly since this will be most commonly used as a view helper, that's probably the right choice here.)
4. ambiguous target - it is possible that a particular target might be pointed at by more than one route address. I have learned the hard way across a number of different Sails apps that this is generally a bad idea (in my experience, you're always better off redirecting /user/login to /login rather than having both route addresses point at the same target-- both from the perspective of SEO and minimizing technical debt). That said, it's still a thing. (My intuition is to use the first match from the app's explicit routes in this scenario. I don't believe that changing the API of this function to return an array sometimes, or returning an array all the time, is worth it for an edge case that isn't particularly recommended usage anyways)

Can you guys think of anything else we missed?

btw @niallobrien re a view helper, the reason why I'm suggesting it isn't quite as big of a deal is that, in the current proposal, from your view, you'd be able to do:

<a href="<%= sails.getRouteAddress({ target: 'PageController.about' }).url %>">About</a>

Point being that, after looking at it, I'm not sure we really gain a whole lot from adding something like:

<a href="<%= getRouteAddressUrl({ target: 'PageController.about' }) %>">About</a>

(I feel like personally, I'd just forget which one was which all the time)

[mikermcneil]

By the way, just for the record, the new contribution guide I'm going to post this week would have us carrying out this discussion in a PR. Rather than disrupting everything in this issue and moving the discussion, I think it's best we just finish up in here. But just wanted to clarify that for anyone who runs across this and wonders to themselves "Hey, wait a minute!" -- and also to avoid making y'all feel awkward or anything. Anyways- we're good 👍

If you guys wouldn't mind reviewing the proposal and having a think about any other exit conditions or other stuff we might have missed and giving me a thumbs up, then I will take care of implementing this asap.

[sgress454]

Implementation-wise, it would be good to coordinate this with #2659 if we still want to include that as well.

[mikermcneil]

@RomainLanz @Mirago @niallobrien ok so after getting a start on implementing this, it seems to me that we'd be best off with two methods:

• sails.getRouteFor() (specced exactly as above-- the lower-level API for accessing method + url. Could also be extended in the future to include other information about the route, but for now, just returns a dictionary with those two properties.)
• sails.getUrlFor() (exactly as above, except it returns only the URL-- useful primarily for getting hrefs or form actions in views and redirect urls in actions. This method name is also a bit more semantically interoperable w/ the Rails convention)

Does that check out with y'all?

[mikermcneil]

@RomainLanz @Mirago @niallobrien I'd also suggest that both methods support simplified usage as a shortcut, e.g.:

As a shortcut for:

var url = sails.getUrlFor({ target: 'DuckController.quack' });

you could write:

var url = sails.getUrlFor('DuckController.quack');

[mikermcneil]

Stubbed implementation and added some basic tests here: 9992c37

Still needs:

• the implementation (trivial-- I'll add that in a bit)
• tests added for the other exit conditions mentioned above (ensuring that a usage error is thrown with bad usage, ensuring that an unrecognized target error is thrown if no matches are found, and ensuring that a usage error is thrown if the provided target contains no .)
• two reference documentation pages (getUrlFor.md and getRouteFor.md) added to https://github.com/balderdashy/sails-docs/tree/master/reference/application

If anyone would like to take a pass at submitting PRs for the last two bits, that'd be awesome, and I can review them tonight.

[mikermcneil]

Just ran across one more edge case:

• If route address has no HTTP method (i.e. it routes requests from the conventional HTTP methods), then set method property of the result from getRouteFor() to empty string (''). Otherwise, just include the HTTP method as configured in the route config.

[niallobrien]

Great work @mikermcneil - looking good!

[mikermcneil]

@niallobrien @RomainLanz just pushed up a version that should do the trick (as well as docs on the 0.12 branch). There are a few more tests I'd like to write, but this is usable now as specced here on master, afaik. One minor change is that, rather than returning null, if no matching route is found, we throw an error with code E_NOT_FOUND. I should also point out that I did not document the options usage for now (since target is the only possible option), and that I extended this to support matching routes with targets that have separate controller and action properties, routes that use the explicit target key, and of course routes w/ string targets.

Feel free to pull down master and try it now if you're curious; otherwise we're going to push out a new rc with this feature and a few other improvements some time before Tuesday of this week.

[mikermcneil]

I'm going to go ahead and close this issue --- tests finished in c39cce2. Thanks for your help guys!(and please open a new issue if you notice any problems after you've had a chance to take this for a spin).

[luislobo]

Just a simple addition, that might be not that hard to add: what about supporting controllers without the "Controller" word, just like when you define 'Me.login' instead of 'MeController.login'

[mikermcneil]

@luislobo definitely- occurred to me to do that and I just hadn't gotten around to it yet. Would be glad to merge a PR on this if anyone has time to get to it.

[mikermcneil]

Hey @luislobo, I went ahead and added TODO stubs for this-- but it made me realize that the right way to do this is probably to make an addition to the hook spec. Not sure on the details yet, but it could be something like this:

• hooks may choose to define a checkAreRouteTargetsEquivalent() function which receives two arguments, each of which is a route target. If the hook recognizes both route targets and knows they are equivalent, it should return true; otherwise false.
• when running getRouteFrom(), in order to compare the specified target with each potential target in the configured explicit routes, Sails core calls out to the compareRouteTargets() functions of all installed hooks which expose them. If any return true for one of the explicit route targets, then we have a match.

The reason this is necessary (eventually at least) is that technically, the routing syntax for controllers/actions is not actually even part of the Sails router-- hooks just listen for the route:typeUnknown event, and if they know how to handle that syntax, they bind a route accordingly. This works really well as far as separating concerns to individual hooks, and using the approach above for reverse routing, we'd be able to keep all logic related to controller/action comparisons in the controllers hook where it currently lives.

In short, I can't personally focus on enhancing this feature to support this for 0.12, but it'd be a great next step (and would allow for complete control over reverse routing in a future-proof way).

[luislobo]

@mikermcneil OK, thanks! I was already developing a solution for this, I'll check your additions and adjust accordingly

[mikermcneil]

@luislobo (sry just saw this)

cool-- most of the time, you'll probably want http://sailsjs.org/documentation/reference/application/sails-get-url-for (but there's also .getRouteFor() if you need it)