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

Add support for named routes #36

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
85 changes: 82 additions & 3 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ var mixin = require('utils-merge')
var parseUrl = require('parseurl')
var Route = require('./lib/route')
var setPrototypeOf = require('setprototypeof')
var pathToRegexp = require('path-to-regexp')

/**
* Module variables.
Expand Down Expand Up @@ -72,6 +73,7 @@ function Router(options) {
router.params = {}
router.strict = opts.strict
router.stack = []
router.routes = {}

return router
}
Expand Down Expand Up @@ -429,7 +431,8 @@ Router.prototype.process_params = function process_params(layer, called, req, re
}

/**
* Use the given middleware function, with optional path, defaulting to "/".
* Use the given middleware function, with optional path,
* defaulting to "/" and optional name.
*
* Use (like `.all`) will run for any http METHOD, but it will not add
* handlers for those methods so OPTIONS requests will not consider `.use`
Expand All @@ -441,6 +444,10 @@ Router.prototype.process_params = function process_params(layer, called, req, re
* pathname.
*
* @public
* @param {string} path (optional)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than making the argument's description "(optional)", can we use the JSDoc features to make it as optional? Probably a better IDE experience.

* @param {function} handler
* @param {string} name (optional)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than making the argument's description "(optional)", can we use the JSDoc features to make it as optional? Probably a better IDE experience.

*
*/

Router.prototype.use = function use(handler) {
Expand Down Expand Up @@ -469,6 +476,28 @@ Router.prototype.use = function use(handler) {
throw new TypeError('argument handler is required')
}

var name

// If a name is used, the last argument will be a string, not a function
if (callbacks.length > 1 && typeof callbacks[callbacks.length - 1] !== 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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; 

name = callbacks.pop()
}

if(name && !((path instanceof String) || typeof path === 'string')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we ever want to be accepting boxed primitives.

throw new Error('only paths that are strings can be named')
}

if(name && this.routes[name]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we are accepting an empty string as a valid name argument, but then silently not acting on it. SHould either just use that as a valid name, or reject it with an error.

throw new Error('a route or handler with that name already exists')
}

if (name && callbacks.length > 1) {
throw new Error('only one handler can be used if Router.use is called with a name parameter')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a huge limitation, and I have almost no production app that could ever use the feature like this, because routes need many handlers, like auth handlers, permission handlers, parsing handlers, data loading handlers, etc.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably the error message isn't great here, it doesn't prevent multiple handlers in total, just that you can't have more than one handler for one name.

}
if (name && !(callbacks[0] instanceof Router)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The instanceof check is unlikely to work, no? For example, people are allowed to mix multiple versions of this module together, as well as with versions of the router that are copied into Express itself.

throw new Error('handler should be a Router if Router.use is called with a name parameter')
}

for (var i = 0; i < callbacks.length; i++) {
var fn = callbacks[i]

Expand All @@ -488,6 +517,10 @@ Router.prototype.use = function use(handler) {
layer.route = undefined

this.stack.push(layer)

if(name) {
this.routes[name] = {'path':path, 'handler':fn};
}
}

return this
Expand All @@ -502,12 +535,20 @@ Router.prototype.use = function use(handler) {
* and middleware to routes.
*
* @param {string} path
* @param {string} name (optional)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than making the argument's description "(optional)", can we use the JSDoc features to make it as optional? Probably a better IDE experience.

* @return {Route}
* @public
*/

Router.prototype.route = function route(path) {
var route = new Route(path)
Router.prototype.route = function route(path, name) {
if(name && this.routes[name]) {
throw new Error('a route or handler with that name already exists')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"with that name" will probably not be too helpful, especially since people are likely to calculate the name. May want to include it in the message?

}
var route = new Route(path, name)

if(name) {
this.routes[name] = route
}

var layer = new Layer(path, {
sensitive: this.caseSensitive,
Expand All @@ -534,6 +575,44 @@ methods.concat('all').forEach(function(method){
}
})

/**
* Find a path for the previously created named route. The name
* supplied should be separated by '.' if nested routing is
* used. Parameters should be supplied if the route includes any
* (e.g. {userid: 'user1'}).
*
* @param {string} route name or '.' separated path
* @param {Object} params
* @return {string}
*/

Router.prototype.findPath = function findPath(routePath, params) {
if (!((routePath instanceof String) || typeof routePath === 'string')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to be accepting boxed primitives.

throw new Error('route path should be a string')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be a TypeError, probably.

}
var firstDot = routePath.indexOf('.')
var routeToFind;
if (firstDot === -1) {
routeToFind = routePath
} else {
routeToFind = routePath.substring(0, firstDot)
}
var thisRoute = this.routes[routeToFind]
if (!thisRoute) {
throw new Error('route path \"'+ routeToFind + '\" does not match any named routes')
}
var toPath = pathToRegexp.compile(thisRoute.path)
var path = toPath(params)
if (firstDot === -1) { // only one segment or this is the last segment
return path
}
var subPath = routePath.substring(firstDot + 1)
if(thisRoute.handler === undefined || thisRoute.handler.findPath === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to make findPath a fully-documented public expectation for other router implementations. The idea behind the router currently is that anyone can implement various different ones and mix and match as needed. The docs, in addition to describing this new method, should describe how we would expect other router implementations to implement it as well.

We may need to consider what's going to happen if we just call findPath on someone's existing handler function that's out there as well, since we've never done that before, so don't know what we'd be breaking. Let's get some research going on this :)

throw new Error('part of route path \"' + subPath + '\" does not match any named nested routes')
}
return path + thisRoute.handler.findPath(subPath, params)
}

/**
* Generate a callback that will make an OPTIONS response.
*
Expand Down
6 changes: 4 additions & 2 deletions lib/route.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,17 @@ var slice = Array.prototype.slice
module.exports = Route

/**
* Initialize `Route` with the given `path`,
* Initialize `Route` with the given `path` and `name`,
*
* @param {String} path
* @param {String} name (optional)
* @api private
*/

function Route(path) {
function Route(path, name) {
debug('new %s', path)
this.path = path
this.name = name
this.stack = []

// route handlers for various http methods
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
"debug": "~2.2.0",
"methods": "~1.1.2",
"parseurl": "~1.3.1",
"path-to-regexp": "0.1.7",
"path-to-regexp": "1.2.1",
"setprototypeof": "1.0.0",
"utils-merge": "1.0.0"
},
Expand Down
24 changes: 22 additions & 2 deletions test/route.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,26 @@ describe('Router', function () {
assert.equal(route.path, '/foo')
})

it('should set the route name iff provided', function () {
var router = new Router()
var route = router.route('/abc', 'abcRoute')
assert.equal(route.path, '/abc')
assert.equal(route.name, 'abcRoute')
assert.equal(router.routes['abcRoute'], route)
var route2 = router.route('/def')
assert.equal(router.routes['abcRoute'], route)
assert.equal(null, router.routes[undefined])
})

it('should not allow duplicate route or handler names', function () {
var router = new Router()
var route = router.route('/abc', 'abcRoute')
assert.throws(router.route.bind(router, '/xyz', 'abcRoute'), /a route or handler with that name already exists/)
var nestedRouter = new Router()
router.use(nestedRouter, 'nestedRoute')
assert.throws(router.route.bind(router, '/xyz', 'nestedRoute'), /a route or handler with that name already exists/)
})

it('should respond to multiple methods', function (done) {
var cb = after(3, done)
var router = new Router()
Expand Down Expand Up @@ -480,7 +500,7 @@ describe('Router', function () {
.expect(200, cb)
})

it('should work in a named parameter', function (done) {
/* it('should work in a named parameter', function (done) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just marking down the concern that there is a commented-out test in here still :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it does depend on path-to-regexp 1.x so it couldn't be landed until that dependency is updated and I guess this test would be deleted or rewritten as part of that.

var cb = after(2, done)
var router = new Router()
var route = router.route('/:foo(*)')
Expand All @@ -495,7 +515,7 @@ describe('Router', function () {
request(server)
.get('/fizz/buzz')
.expect(200, {'0': 'fizz/buzz', 'foo': 'fizz/buzz'}, cb)
})
})*/

it('should work before a named parameter', function (done) {
var router = new Router()
Expand Down
87 changes: 87 additions & 0 deletions test/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,50 @@ describe('Router', function () {
.expect(404, done)
})

it('should support named handlers', function () {
var router = new Router()

var router2 = new Router()
router.use(router2, 'router2')
assert.notEqual(router.routes['router2'], undefined)
assert.equal(router.routes['router2'].handler, router2)
assert.equal(router.routes['router2'].path, '/')

var router3 = new Router()
router.use('/mypath', router3, 'router3')
assert.notEqual(router.routes['router3'], undefined)
assert.equal(router.routes['router3'].handler, router3)
assert.equal(router.routes['router2'].handler, router2)
assert.equal(router.routes['router3'].path, '/mypath')

var router4 = new Router()
router.use(router4, 'router4')
assert.equal(router.routes['router4'].handler, router4)
assert.equal(router.routes['router4'].path, '/')
})

it('should not allow duplicate names', function () {
var router = new Router()
router.use('/', new Router(), 'router1')
assert.throws(router.use.bind(router, new Router(), 'router1'), /a route or handler with that name already exists/)
router.route('/users', 'users')
assert.throws(router.use.bind(router, new Router(), 'users'), /a route or handler with that name already exists/)
})

it('should not support named handlers if handler is a function', function () {
var router = new Router()
assert.throws(router.use.bind(router, '/', new Router(), new Router(), 'hello'), /only one handler can be used if Router.use is called with a name parameter/)
assert.throws(router.use.bind(router, '/', function() {}, function () {}, 'hello'), /only one handler can be used if Router.use is called with a name parameter/)
assert.throws(router.use.bind(router, '/', function(){}, 'hello'), /handler should be a Router if Router.use is called with a name parameter/)
assert.throws(router.use.bind(router, function(){}, 'hello'), /handler should be a Router if Router.use is called with a name parameter/)
})

it('should not support named handlers unless path is a string', function () {
var router = new Router()
assert.throws(router.use.bind(router, ['/123', '/abc'], new Router(), '123abc'), /only paths that are strings can be named/)
assert.throws(router.use.bind(router, /\/abc|\/xyz/, new Router(), '123abc'), /only paths that are strings can be named/)
})

describe('error handling', function () {
it('should invoke error function after next(err)', function (done) {
var router = new Router()
Expand Down Expand Up @@ -983,6 +1027,49 @@ describe('Router', function () {
.expect(200, 'saw GET /bar', done)
})
})

describe('.findPath(path)', function () {
it('should only allow string paths', function() {
var router = new Router()
router.route('/users/:userid', 'users')
assert.throws(router.findPath.bind(router, function(){}), /route path should be a string/)
assert.throws(router.findPath.bind(router, new Router()), /route path should be a string/)
assert.throws(router.findPath.bind(router, {}), /route path should be a string/)
})

it('should return a path to a route', function () {
var router = new Router()
router.route('/users/:userid', 'users')
var path = router.findPath('users', {userid: 'user1'});
assert.equal(path, '/users/user1')
var path2 = router.findPath(new String('users'), {userid: 'user2'});
assert.equal(path2, '/users/user2')
})

it('should throw error if route cannot be matched', function () {
var router = new Router()
router.route('/users/:userid', 'users')
assert.throws(router.findPath.bind(router, 'users.hello', {userid: 'user1'}), /part of route path "hello" does not match any named nested routes/)
assert.throws(router.findPath.bind(router, 'hello', {userid: 'user1'}), /route path "hello" does not match any named routes/)
assert.throws(router.findPath.bind(router, 'users', {abc: 'user1'}), /Expected "userid" to be defined/)
assert.throws(router.findPath.bind(router, 'users', {}), /Expected "userid" to be defined/)
})

it('should support nested routers', function () {
var routerA = new Router()
var routerB = new Router()
routerA.use('/base/:path', routerB, 'routerB')
var r = routerB.route('/some/:thing', 'thing')
var path = routerA.findPath('routerB.thing', {path: 'foo', thing: 'bar'})
assert.equal(path, '/base/foo/some/bar')
path = routerA.findPath('routerB', {path: 'foo'})
assert.throws(routerA.findPath.bind(routerA, 'route', {path: 'foo'}), /route path "route" does not match any named routes/)
assert.equal(path, '/base/foo')
path = routerB.findPath('thing', {thing: 'bar'})
assert.equal(path, '/some/bar')
assert.throws(routerA.findPath.bind(routerA, 'routerB.thing.hello', {path: 'foo', thing: 'bar'}), /part of route path "hello" does not match any named nested routes/)
})
})
})

function helloWorld(req, res) {
Expand Down