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

Store matched routes in request #86

Open
wants to merge 11 commits into
base: 2.0
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .npmrc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
package-lock=false
13 changes: 2 additions & 11 deletions .travis.yml
Original file line number Diff line number Diff line change
@@ -1,17 +1,8 @@
language: node_js
node_js:
- "0.10"
- "0.12"
- "1.8"
- "2.5"
- "3.3"
- "4.9"
- "5.12"
- "6.14"
- "7.10"
- "8.11"
- "9.11"
- "10.6"
- "12.14"
- "13.5"
sudo: false
cache:
directories:
Expand Down
12 changes: 12 additions & 0 deletions HISTORY.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,15 @@
unreleased
==========

* deps: [email protected]
* deps: [email protected]
* deps: [email protected]
* deps: [email protected]
* deps: [email protected]
* deps: [email protected]
* deps: [email protected]
* Store matched routes in request

2.0.0-alpha.1 / 2018-07-27
==========================

Expand Down
74 changes: 66 additions & 8 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -175,14 +175,14 @@ rather than responding.

```js
router.route('/')
.all(function (req, res, next) {
next()
})
.all(check_something)
.get(function (req, res) {
res.setHeader('Content-Type', 'text/plain; charset=utf-8')
res.end('Hello World!')
})
.all(function (req, res, next) {
next()
})
.all(check_something)
.get(function (req, res) {
res.setHeader('Content-Type', 'text/plain; charset=utf-8')
res.end('Hello World!')
})
```

## Middleware
Expand Down Expand Up @@ -382,6 +382,64 @@ router.route('/pet/:id')
server.listen(8080)
```

## Migrating to 2.x from 1.x

The main change is the update to `[email protected]`, which has a few breaking changes:

#### No longer a direct conversion to a RegExp with sugar on top.

It's a path matcher with named and unnamed matching groups. It's unlikely you previously abused this feature,
it's rare and you could always use a RegExp instead. An example of this would be:

```javascript
// Used to work
router.get('/\\d+')

// Now requires matching group
router.get('/(\\d+)')
```

#### All matching RegExp special characters can be used in a matching group.

Other RegExp features are not supported - no nested matching groups, non-capturing groups or look aheads
There is really only one common change is needing replacing any routes with `*` to `(.*)`. Some examples:

- `/:user(*)` becomes `/:user(.*)`
- `/:user/*` becomes `/:user/(.*)`
- `/foo/*/bar` becomes `/foo/(.*)/bar`

#### Parameters have suffixes that augment meaning - `*`, `+` and `?`. E.g. `/:user*`

Needs more info.

#### Named params with regex no longer define positionally.

One other small change (hopefully low impact), is that named parameters with regular expressions no longer result in positional
values in the `params` object. An example is:

```javascript
router.get('/:foo(.*)')

// old GET /bar
console.log(req.params) // {0: 'bar', 'foo': 'bar'}

// new GET /bar
console.log(req.params) // {'foo': 'bar'}
```

#### Partial matching, prefer escaping delimiter

The update to `path-to-regexp@3` includes a change to how partial matches are handled,
now you should escape the delimiter before a partial match segment. For example:

```javascript
// old
router.get('/user(s)?/:user/:op')

// new
router.get('\\/user(s)?/:user/:op')
```

## License

[MIT](LICENSE)
Expand Down
11 changes: 9 additions & 2 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,8 @@ Router.prototype.handle = function handle(req, res, callback) {
// manage inter-router variables
var parentParams = req.params
var parentUrl = req.baseUrl || ''
var done = restore(callback, req, 'baseUrl', 'next', 'params')
var parentMatchedRoutes = req.matchedRoutes
var done = restore(callback, req, 'baseUrl', 'next', 'params', 'matchedRoutes')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like adding matchedRoutes is necessary here, but no tests fail. So we should either try to add a test for it, or remove it if it's not necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be apparent once more tests are added with sub routers. This controls the behavior of what happens on a router exit.


// setup next layer
req.next = next
Expand Down Expand Up @@ -199,6 +200,7 @@ Router.prototype.handle = function handle(req, res, callback) {
req.baseUrl = parentUrl
req.url = protohost + removed + req.url.substr(protohost.length)
removed = ''
req.matchedRoutes = parentMatchedRoutes
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this is trying to restore the req.matchedRoutes back to what it used to be, but since the router is just directly manipulating the array, it still ends up containing the new additions that were made. I'm not 100% sure what the purpose of this restoration of the property is meant to do after every route call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dougwilson This whole approach needs to be tweaked anyway to to deal with the "sibling" routes example you mentioned in #86 (comment).

I'm open to suggestions for the best way to deal with the matchedRoutes "stack". I don't know enough yet about the router internals to know the best place to push and pop from it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, due to the lack of documentation here or a full written explanation for how this all should work, I really have no idea what to suggest for the changes, because I'm still unclear how this feature is actually supposed to operate :) Maybe we should circle back to explaining exactly how this feature should work in text form, so then we can all help on the implementation by knowing what we're trying to achieve. Right now, I'm still unclear on what exactly the plain is, so don't have a way to suggest what changes to make to the code to get the outcome to be whatever it is it should be, if that makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dougwilson Apologies. This PR has moved a number of times between expressjs and pillarjs, so much of the original context may have been lost.

AFAIK, this all got started with expressjs/express#2501 requesting req.originalRoute to complement req.originalUrl. The use case provided there is for logging route names instead of fully resolved paths, which is the same as what I need. Another use case I've seen is for recording metrics. E.g. when recording how many times an endpoint like GET /users/:id is called, you may want to use the full name of the endpoint without path parameters substituted.

As for the technical decisions in this PR, I can't speak to most of them. The original author (@ajfranzoia) seems to have abandoned his efforts, and I'm merely picking what he wrote back up. Perhaps @wesleytodd can provide more insight?

I assume req.originalRoute morphed into an array of matchedRoutes when it was realized that route names aren't always strings and that it wasn't as simple as joining path segments together.

}

// signal to exit router
Expand Down Expand Up @@ -287,6 +289,11 @@ Router.prototype.handle = function handle(req, res, callback) {
return next(layerError || err)
}

if (layer.path) {
req.matchedRoutes = req.matchedRoutes || []
req.matchedRoutes.push(layer.matchedPath.path)
}

if (route) {
return layer.handle_request(req, res, next)
}
Expand Down Expand Up @@ -342,7 +349,7 @@ Router.prototype.process_params = function process_params(layer, called, req, re
var params = this.params

// captured parameters from the layer, keys and values
var keys = layer.keys
var keys = layer.matchedPath && layer.matchedPath.keys

// fast track
if (!keys || keys.length === 0) {
Expand Down
56 changes: 46 additions & 10 deletions lib/layer.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

var pathRegexp = require('path-to-regexp')
var debug = require('debug')('router:layer')
var flatten = require('array-flatten')

/**
* Module variables.
Expand All @@ -28,23 +29,48 @@ var hasOwnProperty = Object.prototype.hasOwnProperty

module.exports = Layer

function Layer(path, options, fn) {
function Layer(p, options, fn) {
if (!(this instanceof Layer)) {
return new Layer(path, options, fn)
return new Layer(p, options, fn)
}

debug('new %o', path)
debug('new %o', paths)
var opts = options || {}

// If not in strict allow both with or without trailing slash
var paths = p
if (!opts.strict) {
if (!Array.isArray(paths) && paths !== '/' && paths[paths.length - 1] === '/') {
paths = paths.substr(0, paths.length - 1)
} else {
for (var i = 0; i < paths.length; i++) {
if (paths[i] !== '/' && paths[i][paths[i].length - 1] === '/') {
paths[i] = paths[i].substr(0, paths[i].length - 1)
}
}
}
}

this.handle = fn
this.name = fn.name || '<anonymous>'
this.params = undefined
this.path = undefined
this.regexp = pathRegexp(path, this.keys = [], opts)
dougwilson marked this conversation as resolved.
Show resolved Hide resolved
this.matchedPath = undefined

// set fast path flags
this.regexp.fast_star = path === '*'
this.regexp.fast_slash = path === '/' && opts.end === false
this.fastStar = paths === '*'
this.fastSlash = paths === '/' && opts.end === false

this.paths = (!Array.isArray(paths) ? [paths] : flatten(paths)).map(function (path) {
var keys = []
var pathObj = {
path: path,
keys: keys,
regexp: pathRegexp(path, keys, opts)
}

return pathObj
})
}

/**
Expand Down Expand Up @@ -123,29 +149,39 @@ Layer.prototype.handle_request = function handle(req, res, next) {

Layer.prototype.match = function match(path) {
var match
var checkPath

if (path != null) {
// fast path non-ending match for / (any path matches)
if (this.regexp.fast_slash) {
if (this.fastSlash) {
this.params = {}
this.path = ''
this.matchedPath = this.paths[0]
return true
}

// fast path for * (everything matched in a param)
if (this.regexp.fast_star) {
if (this.fastStar) {
this.params = {'0': decode_param(path)}
this.path = path
this.matchedPath = this.paths[0]
return true
}

// match the path
match = this.regexp.exec(path)
for (var i = 0; i < this.paths.length; i++) {
checkPath = this.paths[i]
if (match = checkPath.regexp.exec(path)) {
this.matchedPath = checkPath
break
}
}
}

if (!match) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this.matchedPath should be reset inside this conditional as well, otherwise it will look like there is a matched path when none of the paths matched, but they did in a previous attempt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Is there a way to test this?

Copy link
Contributor

Choose a reason for hiding this comment

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

There should be by creating a specific order of routes and running though them. I can help make one if you need, perhaps this weekend

this.params = undefined
this.path = undefined
this.matchedPath = undefined
return false
}

Expand All @@ -158,7 +194,7 @@ Layer.prototype.match = function match(path) {
var params = this.params

for (var i = 1; i < match.length; i++) {
var key = keys[i - 1]
var key = this.matchedPath.keys[i - 1]
var prop = key.name
var val = decode_param(match[i])

Expand Down
15 changes: 8 additions & 7 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,18 @@
"debug": "3.1.0",
"methods": "~1.1.2",
"parseurl": "~1.3.2",
"path-to-regexp": "0.1.7",
"setprototypeof": "1.1.0",
"path-to-regexp": "^3.0.0",
"setprototypeof": "1.2.0",
"utils-merge": "1.0.1"
},
"devDependencies": {
"after": "0.8.2",
"eslint": "3.19.0",
"eslint-plugin-markdown": "1.0.0-beta.6",
"eslint": "6.8.0",
"eslint-plugin-markdown": "1.0.1",
"finalhandler": "1.1.1",
"mocha": "3.5.3",
"nyc": "10.3.2",
"supertest": "1.2.0"
"mocha": "7.0.0",
"nyc": "15.0.0",
"supertest": "4.0.2"
},
"files": [
"lib/",
Expand All @@ -38,6 +38,7 @@
},
"scripts": {
"lint": "eslint --plugin markdown --ext js,md .",
"lint-fix": "eslint --plugin markdown --ext js,md . --fix",
"test": "mocha --reporter spec --bail --check-leaks test/",
"test-cov": "nyc --reporter=text npm test",
"test-travis": "nyc --reporter=html --reporter=text npm test"
Expand Down
16 changes: 8 additions & 8 deletions test/auto-head.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@ describe('HEAD', function () {
router.get('/users', sethit(1), saw)

request(server)
.head('/users')
.expect('Content-Type', 'text/plain')
.expect('x-fn-1', 'hit')
.expect(200, done)
.head('/users')
.expect('Content-Type', 'text/plain')
.expect('x-fn-1', 'hit')
.expect(200, done)
})

it('should invoke head if prior to get', function (done) {
Expand All @@ -27,10 +27,10 @@ describe('HEAD', function () {
router.get('/users', sethit(2), saw)

request(server)
.head('/users')
.expect('Content-Type', 'text/plain')
.expect('x-fn-1', 'hit')
.expect(200, done)
.head('/users')
.expect('Content-Type', 'text/plain')
.expect('x-fn-1', 'hit')
.expect(200, done)
})
})

Expand Down
Loading