ZijianHe / koa-router

Router middleware for koa.
MIT License
4.85k stars 408 forks source link

Weird behaviors when one router that has called #param use .routes() from another router that has called #all/#use #383

Open Mensu opened 6 years ago

Mensu commented 6 years ago

behavior 1

When I make a request of GET /top/1/sub to a server written in the following codes, I get a confusing response.

const Koa = require('koa');
const Router = require('koa-router');

const subRtr = new Router();
subRtr.all('*', function subRtrAll(ctx, next) {
  ctx.body += 'subRtrAll\n';
  return next();
});
subRtr.get('/', function subRtrGet(ctx, next) {
  ctx.body += 'subRtrGet\n';
  return;
});

const topRtr = new Router();
topRtr.param('id', function topRtrParam(id, ctx, next) {
  ctx.body += `topRtrParam with id ${id}\n`;
  return next();
});
topRtr.use('/top/:id/sub', subRtr.routes());

const app = new Koa();
app.use(topRtr.routes());
app.listen(3000);

Response:

undefinedtopRtrParam with id 1
subRtrAll
topRtrParam with id 1
subRtrGet

As we can see, topRtrParam is called twice during one request. But I expect topRtrParam to be called only once.

Is this an intentional design? How I should avoid this to get my expected response?

undefinedtopRtrParam with id 1
subRtrAll
subRtrGet

Digging into the source code of koa-router I find that it is because, when topRtr.use('/top/:id/sub', subRtr.routes()), in

https://github.com/alexmingoia/koa-router/blob/afb542bd9fbeec43f455f22b569c6e374a497dc8/lib/router.js#L272

topRtrParam is added to both subRtrAll and subRtrGet. The stacks of subRtrAll and subRtrGet ('s Layer) both contain topRtrParam.

Therefore, when a request for GET /top/1/sub comes, in

https://github.com/alexmingoia/koa-router/blob/afb542bd9fbeec43f455f22b569c6e374a497dc8/lib/router.js#L347

topRtrParam would be included into layerChain twice by the concatenation involving layer.stacks, namely [topRtrParam, subRtrAll] and [topRtrParam, subRtrGet].

behavior 2

Besides, I find that when I change

subRtr.all('*', subRtrAll);

to

subRtr.use(subRtrAll);

I get response:

undefinedtopRtrParam with id undefined
subRtrAll
topRtrParam with id 1
subRtrGet

In the first call of topRtrParam id receives undefined!

Digging into the source code I found it is because ignoreCaptures would be set to true (!hasPath) when subRtr.use(subRtrAll)

https://github.com/alexmingoia/koa-router/blob/afb542bd9fbeec43f455f22b569c6e374a497dc8/lib/router.js#L276

while ignoreCaptures would not be set when subRtr.get('/', subRtrGet)

https://github.com/alexmingoia/koa-router/blob/afb542bd9fbeec43f455f22b569c6e374a497dc8/lib/router.js#L202

Is this made intentionally? I could just add a string path like '*' to avoid this, but I believe the expected response should still be

undefinedtopRtrParam with id 1
subRtrAll
subRtrGet

Thanks for any help in advance.

Mensu commented 6 years ago

As a workaround, in my personal toy projects I would turn to koa-express-router, which is based on Express's Router, and doesn't have problems with shared nested routers, param issues, use issues, etc.

I've also investigated impress-router, koa-Router and koa-router2 before, only to find they don't seem to support .param, which feature has been widely used in some projects I am working for, and we still decide to use this most popular koa-router package in the community in future.

So I still expect to get a response about this issue: are those behaviors due to some intentional design, or just due to a bug for which it is possible to find workarounds without switching to other router packages?

jbielick commented 6 years ago

This is a bug. The router objects are mutable and calling prefix or use('prefix', subRtr) will mutate the original and break some assumptions about how that router will work. See #244