Middlewares aren't called if there's no matched route

[julienw]

I found that this was also discussed in the old repository in ZijianHe/koa-router#182 ZijianHe/koa-router#257 ZijianHe/koa-router#239.

The problem can be seen with a code like this. The testcase could be made simpler but I left it like this so that it's closer to my real-world case, implementing CORS.

// @flow
const Koa = require('koa');
const Router = require('@koa/router');
const cors = require('@koa/cors');

function getRouter() {
  const router = new Router();

  router.get('/hello1', ctx => {
    ctx.body = 'world1';
  });
  router.use(cors());
  router.post('/hello2', ctx => {
    ctx.body = 'world2';
  });

  return router;
}

const app = new Koa();
const router = getRouter();
app.use(router.routes());
app.use(router.allowedMethods());

app.listen(20000);
console.log('server started on port 20000');

This is a curl command you can use to test this (this is a socalled "preflight CORS request"):

curl -i -H 'Origin: foo' -H 'Access-Control-Request-Method: POST' -X OPTIONS http://localhost:20000/hello2

The answer should be something like:

HTTP/1.1 204 No Content
Vary: Origin
Access-Control-Allow-Origin: foo
Access-Control-Allow-Methods: GET,HEAD,PUT,POST,DELETE,PATCH
Date: Fri, 10 Jan 2020 18:36:33 GMT
Connection: keep-alive

But instead it is:

HTTP/1.1 200 OK
Content-Type: text/plain; charset=utf-8
Content-Length: 0
Allow: POST
Date: Fri, 10 Jan 2020 18:36:28 GMT
Connection: keep-alive

This shows that this is the middleware of allowedMethods that runs, instead of the "cors" middleware.

Note this does work:

// @flow
const Koa = require('koa');
const Router = require('@koa/router');
const cors = require('@koa/cors');

function getRouter1() {
  const router = new Router();

  router.get('/hello1', ctx => {
    ctx.body = 'world1';
  });

  return router;
}

function getRouter2() {
  const router = new Router();

  router.post('/hello2', ctx => {
    ctx.body = 'world2';
  });

  return router;
}

const app = new Koa();
const router1 = getRouter1();
app.use(router1.routes());
app.use(router1.allowedMethods());

app.use(cors());

const router2 = getRouter2();
app.use(router2.routes());
app.use(router2.allowedMethods());

app.listen(20001);
console.log('server started on port 20001');

My main issue is that there's a surprise here: the router doesn't behave like koa and ignores all middleware if there's no matching route, and it's not documented anywhere. Koa instead will always run all middleware.

After various tries, I believe that this simple patch makes it work properly and doesn't have much impact:

diff --git a/lib/router.js b/lib/router.js
index a97941f..e681b6f 100644
--- a/lib/router.js
+++ b/lib/router.js
@@ -331,23 +331,24 @@ Router.prototype.routes = Router.prototype.middleware = function () {
     if (ctx.matched) {
       ctx.matched.push.apply(ctx.matched, matched.path);
     } else {
       ctx.matched = matched.path;
     }
 
     ctx.router = router;
 
-    if (!matched.route) return next();
-
     var matchedLayers = matched.pathAndMethod
-    var mostSpecificLayer = matchedLayers[matchedLayers.length - 1]
-    ctx._matchedRoute = mostSpecificLayer.path;
-    if (mostSpecificLayer.name) {
-      ctx._matchedRouteName = mostSpecificLayer.name;
+
+    if (matched.route) {
+      var mostSpecificLayer = matchedLayers[matchedLayers.length - 1]
+      ctx._matchedRoute = mostSpecificLayer.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);
         ctx.routerName = layer.name;
         return next();

Please tell me if you'd like that I do a PR with some test changes, I can do it.

[Sinewyk]

I don't agree with you. The only problem here is documentation. Not code.

Put in the documentation that middlewares that are used on the router are executed if and only if a route is matched. Problem solved. You already solved it yourself.

if your premise is

Koa instead will always run all middleware.

you are wrong

app.use(something_that_may_not_await_next)
app.use(whatever)

whatever may not execute based on the decision of the parent, no surprise there.

Refs:

• Allowed methods broken in 5.2.0, 5.2.1 ZijianHe/koa-router#182
• ZijianHe/koa-router@e486a93

• router/test/lib/router.js

  Lines 197 to 222 in 2e69eca

  	it('matches middleware only if route was matched (gh-182)', function (done) {
  	var app = new Koa();
  	var router = new Router();
  	var otherRouter = new Router();
  	router.use(function (ctx, next) {
  	ctx.body = { bar: 'baz' };
  	return next();
  	});
  	otherRouter.get('/bar', function (ctx) {
  	ctx.body = ctx.body || { foo: 'bar' };
  	});
  	app.use(router.routes()).use(otherRouter.routes());
  	request(http.createServer(app.callback()))
  	.get('/bar')
  	.expect(200)
  	.end(function (err, res) {
  	if (err) return done(err);
  	expect(res.body).to.have.property('foo', 'bar');
  	expect(res.body).to.not.have.property('bar');
  	done();
  	})
  	});

  <= we still test for this behavior that was considered a bug

TL;DR: it's not a bug, it's a feature.

[julienw]

Yes, I wrote in the first line of the initial comment that this has been discussed before. I've seen it. I took the time to write this issue and this PR especially to discuss this more, and possibly decide to change the intended behavior. I understand this wouldn't be a light decision, so I would totally expect some sort of discussion.

That's why I came with arguments and a use case. And to be honest I expect a bit more than "nope, it's expected, we always did this so we'll always do this."
Especially I'd like to understand why the current behavior makes more sense than what I propose. I'd like use cases and arguments, as well as some sort of vision.

Thanks for reading!

[Sinewyk]

For me it's about cross cutting concerns.

Current case:

// isolated in a file
router1.use(something);

// in another file
router2.use(something_else);

// app file
app.use(router1.routes());
app.use(router2.routes());

app.use(web_socket_handler) //

You do not have to know what's inside router1 and router2 if testing your websocket stuff.

You said "give me the routes middleware".

You have one way to do one thing.

Your case:

// isolated in a file
router1.use(something);

// in another file
router2.use(something_else);

// app file
app.use(router1.routes());
app.use(router2.routes());

app.use(web_socket_handler)

Here you said "give me the routes middleware and also every middleware you used on the router that will NOT match a route", weird 🤔

More importantly: something and something_else need to be aware of each other, because they can interact with each other. But it's not explicit. weird, two routers that can run things on different routes that they are not concerned with ? 🤔

Now there are two ways to do one thing (run a global middleware), and no way to do the other thing: NOT run used middleware on the router when nothing matched.

In the current case: there are no wins, but there are no loss, in terms of functionality. You already have your solution: move cors() in the global middleware stack.

In your case: we gained another way to define a global middleware, and we lost the granularity of a global middleware scoped to the router when something is matched. I consider this a loss. Now I'm asking you to define a middleware that is only going to be run when a route is matched in a concise way, can you do it ? There is also a workaround, duplicate the middleware across all .get calls or something. But then, if one of the library goal is to be concise (IDK)... 🤷‍♂️

In hope that I was clear, have a good day !