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.

Sign up for GitHub

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

Jump to bottom

populate ctx.params before running any matched middleware #490

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

populate ctx.params before running any matched middleware #490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
89680b2
populate `ctx.params` before running any matched middleware
Dec 6, 2018
da8ff95
remove unnecessary tests
Dec 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 2 additions & 5 deletions lib/layer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,15 +64,13 @@ Layer.prototype.match = function (path) {
/**
* Returns map of URL parameters for given `path` and `paramNames`.
*
* @param {String} path
* @param {Array.<String>} captures
* @param {Object=} existingParams
* @returns {Object}
* @private
*/

Layer.prototype.params = function (path, captures, existingParams) {
var params = existingParams || {};
Layer.prototype.params = function (captures) {
var params = {};

for (var len = captures.length, i=0; i<len; i++) {
if (this.paramNames[i]) {
Expand All @@ -93,7 +91,6 @@ Layer.prototype.params = function (path, captures, existingParams) {
*/

Layer.prototype.captures = function (path) {
if (this.opts.ignoreCaptures) return [];
return path.match(this.regexp).slice(1);
};

Expand Down
6 changes: 2 additions & 4 deletions lib/router.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ Router.prototype.use = function () {
});
}
} else {
router.register(path || '(.*)', [], m, { end: false, ignoreCaptures: !hasPath });
router.register(path || '(.*)', [], m, { end: false });
}
});

Expand Down Expand Up @@ -334,14 +334,13 @@ Router.prototype.routes = Router.prototype.middleware = function () {
var matchedLayers = matched.pathAndMethod
var mostSpecificLayer = matchedLayers[matchedLayers.length - 1]
ctx._matchedRoute = mostSpecificLayer.path;
ctx.params = mostSpecificLayer.params( mostSpecificLayer.captures(path) )
if (mostSpecificLayer.name) {
ctx._matchedRouteName = mostSpecificLayer.name;
}

layerChain = matchedLayers.reduce(function(memo, layer) {
memo.push(function(ctx, next) {
ctx.captures = layer.captures(path, ctx.captures);
ctx.params = layer.params(path, ctx.captures, ctx.params);
Copy link

@divmgl divmgl Jun 15, 2019 •
edited

Choose a reason for hiding this comment

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

This line is needed in order to pass in params to the Router.prototype.param function in nested routers.

ctx.params = layer.params(layer.captures(path))

Copy link

Choose a reason for hiding this comment

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

Addressed in #508

ctx.routerName = layer.name;
return next();
});
Expand Down Expand Up @@ -556,7 +555,6 @@ Router.prototype.register = function (path, methods, middleware, opts) {
sensitive: opts.sensitive || this.opts.sensitive || false,
strict: opts.strict || this.opts.strict || false,
prefix: opts.prefix || this.opts.prefix || "",
ignoreCaptures: opts.ignoreCaptures
});

if (this.opts.prefix) {
Expand Down
72 changes: 0 additions & 72 deletions test/lib/layer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,78 +72,6 @@ describe('Layer', function() {
.end(done);
});

it('populates ctx.captures with regexp captures', function(done) {
var app = new Koa();
var router = new Router();
app.use(router.routes());
router.get(/^\/api\/([^\/]+)\/?/i, function (ctx, next) {
ctx.should.have.property('captures');
ctx.captures.should.be.instanceOf(Array);
ctx.captures.should.have.property(0, '1');
return next();
}, function (ctx) {
ctx.should.have.property('captures');
ctx.captures.should.be.instanceOf(Array);
ctx.captures.should.have.property(0, '1');
ctx.status = 204;
});
request(http.createServer(app.callback()))
.get('/api/1')
.expect(204)
.end(function(err) {
if (err) return done(err);
done();
});
});

it('return orginal ctx.captures when decodeURIComponent throw error', function(done) {
var app = new Koa();
var router = new Router();
app.use(router.routes());
router.get(/^\/api\/([^\/]+)\/?/i, function (ctx, next) {
ctx.should.have.property('captures');
ctx.captures.should.be.type('object');
ctx.captures.should.have.property(0, '101%');
return next();
}, function (ctx, next) {
ctx.should.have.property('captures');
ctx.captures.should.be.type('object');
ctx.captures.should.have.property(0, '101%');
ctx.status = 204;
});
request(http.createServer(app.callback()))
.get('/api/101%')
.expect(204)
.end(function(err) {
if (err) return done(err);
done();
});
});

it('populates ctx.captures with regexp captures include undefiend', function(done) {
var app = new Koa();
var router = new Router();
app.use(router.routes());
router.get(/^\/api(\/.+)?/i, function (ctx, next) {
ctx.should.have.property('captures');
ctx.captures.should.be.type('object');
ctx.captures.should.have.property(0, undefined);
return next();
}, function (ctx) {
ctx.should.have.property('captures');
ctx.captures.should.be.type('object');
ctx.captures.should.have.property(0, undefined);
ctx.status = 204;
});
request(http.createServer(app.callback()))
.get('/api')
.expect(204)
.end(function(err) {
if (err) return done(err);
done();
});
});

it('should throw friendly error message when handle not exists', function() {
var app = new Koa();
var router = new Router();
Expand Down
29 changes: 0 additions & 29 deletions test/lib/router.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -849,35 +849,6 @@ describe('Router', function () {
expect(router.stack[0]).to.have.property('path', '/one');
expect(router.stack[1]).to.have.property('path', '/two');
});

it('resolves non-parameterized routes without attached parameters', function(done) {
var app = new Koa();
var router = new Router();

router.get('/notparameter', function (ctx, next) {
ctx.body = {
param: ctx.params.parameter,
};
});

router.get('/:parameter', function (ctx, next) {
ctx.body = {
param: ctx.params.parameter,
};
});

app.use(router.routes());
request(http.createServer(app.callback()))
.get('/notparameter')
.expect(200)
.end(function (err, res) {
if (err) return done(err);

expect(res.body.param).to.equal(undefined);
done();
});
});
Copy link

Choose a reason for hiding this comment

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

@smkoyan this test is important and removing it will not allow us to check use cases where users want a specific route to be hit followed by a catch-all route.

router.get('/specific-route') // Hit when specific-route is provided
router.get('/:catchall') // Hit in any other case with a capture

Copy link

Choose a reason for hiding this comment

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

Addressed in #508


});

describe('Router#use()', function (done) {
Expand Down