Router.use do not receive ctx.param(s) when route has prefix with params

[cymen]

I've been stuck on koa-router v7.0.1 due to this bug. I finally spent the time to dig into it deeper and figure out the cause.

There was this bug: ZijianHe/koa-router#247

And it was fixed with this PR: ZijianHe/koa-router#287

However, for those of use using koa-router with the prior behavior, that broke our use on routers that had a prefix with a param (and the use expected the param to be preset). Bit of an edge case so now it makes sense why it broke and why most probably just ignored the bug.

This may resolve this issue: #38

I went with the simplest fix that got my added test to green and didn't break the existing tests. The fix is adding && !router.opts.prefix here:

ignoreCaptures: !hasPath && !router.opts.prefix

This is somewhat niave in that the router prefix could be just a string -- we could get more complicated and check if the prefix actually has params (update: I made it do this check with commit e06ac96).

Lastly, I discovered use can optionally take a path as the first argument and that would likely have resolved my issue too but I would argue the pre-existing behavior was present and it is a bug that if a prefix is present (with param(s)), then ctx.params should be populated for use even without a given path (logically, they should assume the path of the prefix if one is present).

[cymen]

I tried to simplify my test route as it seemed more complex than necessary however I discovered another bug. With test modified to this:

    it.only('populates ctx.params correctly for router prefix', function(done) {
      var app = new Koa();
      var router = new Router({ prefix: '/:category' });
      app.use(router.routes());
      router
        .use((ctx, next) => {
          ctx.should.have.property('params');
          ctx.params.should.be.type('object');
          ctx.params.should.have.property('category', 'cats');
          return next();
        })
        .get('/suffixHere', function(ctx) {
          ctx.should.have.property('params');
          ctx.params.should.be.type('object');
          ctx.params.should.have.property('category', 'cats');
          ctx.status = 204;
        });
      request(http.createServer(app.callback()))
        .get('/cats/suffixHere')
        .expect(204)
        .end(function(err, res) {
          if (err) return done(err);
          done();
        });
    });

I get:

$ npm run test

> @koa/router@8.0.8 test /Users/cymen/dev/cymen-koa-router
> NODE_ENV=test mocha test/**/*.js



  Layer
    Layer#match()

  AssertionError: expected Object { category: 'cats/suffixHere' } to have property category of 'cats' (got 'cats/suffixHere')
      at Assertion.fail (/Users/cymen/dev/cymen-koa-router/node_modules/should/cjs/should.js:275:17)
      at Assertion.value [as property] (/Users/cymen/dev/cymen-koa-router/node_modules/should/cjs/should.js:356:19)
      at /Users/cymen/dev/cymen-koa-router/test/lib/layer.js:66:34
      at dispatch (/Users/cymen/dev/cymen-koa-router/node_modules/koa-compose/index.js:42:32)
      at /Users/cymen/dev/cymen-koa-router/lib/router.js:370:16
      at dispatch (/Users/cymen/dev/cymen-koa-router/node_modules/koa-compose/index.js:42:32)
      at /Users/cymen/dev/cymen-koa-router/node_modules/koa-compose/index.js:34:12
      at dispatch (/Users/cymen/dev/cymen-koa-router/lib/router.js:375:31)
      at dispatch (/Users/cymen/dev/cymen-koa-router/node_modules/koa-compose/index.js:42:32)
      at /Users/cymen/dev/cymen-koa-router/node_modules/koa-compose/index.js:34:12
      at Application.handleRequest (/Users/cymen/dev/cymen-koa-router/node_modules/koa/lib/application.js:166:12)
      at Server.handleRequest (/Users/cymen/dev/cymen-koa-router/node_modules/koa/lib/application.js:148:19)
      at Server.emit (events.js:311:20)
      at parserOnIncoming (_http_server.js:784:12)
      at HTTPParser.parserOnHeadersComplete (_http_common.js:119:17)

      1) populates ctx.params correctly for router prefix


  0 passing (33ms)
  1 failing

  1) Layer
       Layer#match()
         populates ctx.params correctly for router prefix:
     Error: expected 204 "No Content", got 500 "Internal Server Error"
      at Test._assertStatus (node_modules/supertest/lib/test.js:268:12)
      at Test._assertFunction (node_modules/supertest/lib/test.js:283:11)
      at Test.assert (node_modules/supertest/lib/test.js:173:18)
      at Server.localAssert (node_modules/supertest/lib/test.js:131:12)
      at emitCloseNT (net.js:1653:8)
      at processTicksAndRejections (internal/process/task_queues.js:83:21)

So while I think I fixed my specific bug it looks like a more general fix would be good. It looks like the path-to-regexp for the prefix is basically .* with /:category as input but when we want that with the full path, we end up with cats/suffixHere as the match not cats.

[cymen]

Alright, I think I have it now -- by updating the default prefix matcher to be ([^\/]*) instead of (.*) we avoid the prefix regexp from later matching (and being used to parse) on the whole path.

I added more tests -- we still get the correct matches for say prefix: '/:abc/:xyz' and so forth.

[JacobMGEvans]

@niftylettuce this looks like a great addition. What do you think?

[cymen]

@niftylettuce Have you had a chance to look at this?

[JacobMGEvans]

Hey @cymen by chance are all your tests passing locally with the current up to date master?

[cymen]

@JacobMGEvans Yes -- are you referring to the CI failures? If you see the run for 1ee4d04 it passed on CI. There was a change in how path-to-regexp worked that I had to account for. Or is it failing for you on local?

[JacobMGEvans]

@JacobMGEvans Yes -- are you referring to the CI failures? If you see the run for 1ee4d04 it passed on CI. There was a change in how path-to-regexp worked that I had to account for. Or is it failing for you on local?

No not CI, I gotcha though. I was referring to running the automated tests locally, I pulled master and all my local tests except like two fails now lol

[cymen]

Oh, I was confused if you meant my master for this PR or master for this repo. You meant the later right? But yeah, it's passing for me (will double check tomorrow too).

[cymen]

@niftylettuce Any chance you can take a look at this?

[niftylettuce]

v9.2.0 released to npm @cymen

https://github.com/koajs/router/releases/tag/v9.2.0

[cymen]

@niftylettuce Yay! Thank you!