-
Notifications
You must be signed in to change notification settings - Fork 405
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
Params are parsed for all matching routes, not just the first route. #292
Comments
👍 I just ran into this myself when trying to implement some contextual middle-ware. |
This is actually caused by #231 here's a failing test case for 7.x it('only runs param functions for matched route', function (done) {
var app = new Koa();
var router = new Router();
router
.get('/new', function (ctx, next) {
ctx.body = {ran: ''};
ctx.body.ran += '/new';
return next();
})
.get('/:id', function (ctx, next) {
ctx.body.ran += '/:id';
return next();
});
app.use(router.routes());
app.use(function(ctx, next) {
ctx.body.params = ctx.params;
// console.log(ctx._matchedRoute) // == /:id
return next();
});
request(http.createServer(app.callback()))
.get('/new')
.expect(200)
.end(function (err, res) {
expect(res.body.ran).to.eql('/new/:id');
expect(res.body.params).not.to.have.property('id');
done(err);
});
}); |
I am seeing this same problem while working to add koa2 support to express-restify-mongoose. For that project I am testing with both koa-router and koa-better-router. The later of these is fully working at this point. With REST, we have However, I am wondering whether there is background (reasons) for the way it is now, and therefore a reason my PR wouldn't be accepted? I'd include the work along with my existing koa-router PR 334, which makes debugging these routes and spotting poorly assembled middleware chains a lot easier. |
Enhanced debug statements
Considering the following program:
when I go to
http://localhost:8080/new
the app matches the/new
route and the/:id
route. But it runs param setters far all the matching routes, instead of just the first one.The result is, that although the path is
/new
(so it should match the/new
route, since it is the first one), thecheckIdParam
middleware gets'new'
as anid
param, which it shouldn't.The text was updated successfully, but these errors were encountered: