ZijianHe / koa-router

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

Why 'use' middleware not called? #257

Open EduardoRFS opened 8 years ago

EduardoRFS commented 8 years ago

This return 'Not called' body

const app = new (require('koa'))();
const router = require('koa-router')();

app.use((ctx, next) => {
  ctx.body = 'Not called';
  return next();
});

router.use((ctx, next) => {
  ctx.body = 'Called';
  return next();
});

app.use(router.routes());
app.use(router.allowedMethods());

app.listen(3000);

But this return 'Called' body

const app = new (require('koa'))();
const router = require('koa-router')();

app.use((ctx, next) => {
  ctx.body = 'Not called';
  return next();
});

router.use((ctx, next) => {
  ctx.body = 'Called';
  return next();
});

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

app.use(router.routes());
app.use(router.allowedMethods());

app.listen(3000);

WHY? When i use 'use' function from koa-router, and not exists a 'get' handler, the middleware not called

inca commented 8 years ago

+1, seems like entire middleware stack is not invoked if no routes match (master)

jbielick commented 8 years ago

Can you explain a use-case in which you want middleware that is mounted on the router to execute if there is no matched route?

If it isn't a concern of the router, it should be mounted on the application middleware stack.

inca commented 8 years ago

Mine was (pseudo):

router.use(async (ctx, next) => {
  const pageId = await resolvePage(ctx.path);
  if (pageId) {
    ctx.path = `/page/${pageId}`;
  }
  await next();
});

router.get('/page/:id', servePage);

Arguably enough, but I initially assumed that every piece of code that deals with urls or pathnames (especially rewriting) is in fact a concern of router (otherwise, who's concern it would be? or, why pathname matching is concern of router while pathname rewriting isn't).

Personally for me, it was a bit counter-intuitive to see router behaving this way because of my two assumptions:

I'm also okay with accepting new rules of the game. Just saying that it should at least be documented, because most (if not all) devs come to koa/koa-router from Express background.

EduardoRFS commented 8 years ago

@jbielick for me koa-router 'use', and koa 'use' are same style, but koa-router have path for routing, like express. And a example is for error handler 404

router.use((ctx, next) => {
  return next().catch(err => {
    ctx.status = err.status || 500;
    return ctx.render('error', {
      message: err.message,
      error: err
    });
  })
});

router.get('/url', ctx => {
  ctx.body = 'A random URL';
});

router.use(ctx => {
  var err = new Error('Not Found');
  ctx.throw(err, 404);
});

If path not match, send a 404 error, like express

inca commented 8 years ago

To be completely honest, this issue was quite a frustration for me. I basically ended up stuffing code with console.logs just to figure what's executed and what's not — just until I realized that this is how koa-router actually works.

Maybe my particular usecase just isn't a koa-router material (middleware mixed with routes, order matters). But either way I switched to using koa-route and koa-mount — things are now crystal clear for me.

Not antagonising, nor implying that koa-router is bad, just wanna help those who are stuck with the same thing.

alekbarszczewski commented 8 years ago

👍 In my opinion request should run through router even none of routes matches request (middlewares set up on router should run). Additionally if "prefix" option passed to router, then only if request matches prefix. My usecase is:

I want to provide middleware that will serve REST API under /api prefix. Since this is REST API, I want to respond with proper JSON response for example if route was not found (501 Not Implemented for example). So it would be nice to do something like this:

const Koa = require('koa')

const router = require('koa-router')({ prefix: '/api' })

// some API endpoint
router.get('/test', function (ctx, next) {
  ctx.body = { ok: true }
})

// If no endpoint matches request return 501and some json
router.use(function (ctx, next) {
  ctx.statusCode = 501
  ctx.body = { message: 'Not implemented' } 
})

app.use(router.routes())

// if not found (but not under /api) then return proper html page
app.use(function (ctx, next) {
  ctx.body = '<h1>Not Found - sorry :(</h1>'
})

app.listen(3000)

// GET /api/test -> { ok: true }
// GET /api/not-implemented -> { message: 'not implemented' } <-- this does not work like this right now :(
// GET /not-implemented.html ->  <h1>Not Found - sorry :(</h1>

There could be option passed to router that would enable such behaviour - for example passthrough: true

jbielick commented 8 years ago

Having not used express much, I wasn't aware that what you're explaining is how express's router works. Clarity is definitely important; sorry that this was frustrating. If this were to be implemented, my guess would be that it would warrant a major release because of the backwards incompatibility + API change.

If the situation is as simple as middleware that needs a prefix, I would second @inca's suggestion for using koa-mount

amacneil commented 8 years ago

I also found this issue very confusing/frustrating. It's strange that koa-router's use() method would behave differently from koa's own use() method.

I had a similar use case to @alekbarszczewski. I wanted to add a custom 404 handler for my koa-router instance which handled all /api routes.

const router = new Router();
router.use('/users', users.routes());
router.use(async () => {
  ctx.status = 404;
  ctx.body = { message: "Not Found" };
});

It took a long time to work out why this middleware was never called. In the end I had to create a koa instance to wrap my API router, just to handle my catch-all route, which seems unnecessarily convoluted.

I'd love to see this method's behavior made consistent with koa's use() method in either 7.x or 8.x release. I do not think it should be an opt-in configuration, there should be least surprise for the user by default.

amacneil commented 8 years ago

Prior discussions of this issue for reference: https://github.com/alexmingoia/koa-router/issues/182 https://github.com/alexmingoia/koa-router/issues/239

defunctzombie commented 8 years ago

Routers and apps are really the same thing. Would not have expected this behavior especially the silent failure. Maybe .use functions should always be invoked?

inca commented 8 years ago

Well, people tend to have slightly different opinions on what routers really are and how should they work.

So yet another alternative is just to hack together a router which does things exactly as you would expect; with path-to-regexp available as a separate module it's actually pretty easy.

mike-marcacci commented 8 years ago

Does anyone here know of a maintained router lib that behaves as we all expect (those of us following this issue)? I understand that it's quite easy to roll your own (as I've already done), but this project's scope is quite well defined and generally useful...

inca commented 8 years ago

For what it's worth I maintain this one for my projects. It works in line with aforementioned expectations, so feel free to use it if it works for you.

Please see koa-router2 for more information.

EduardoRFS commented 8 years ago

I make my own koa-router too https://github.com/EduardoRFS/koa-Router https://www.npmjs.com/package/koa-Router

Is a direct fork from express router, to use in koa

dts commented 8 years ago

A specific use case for this might be helpful to make it clear why this is a useful feature: I want to build a RESTful endpoint, with some additional methods:

router.use('/api',RestfulMiddleware); // <-- this behaves as a Koa app, but leaves open special uses by later middlewares.
router.get('/api/movies/:id',function *(next) { special-case-handler });
// I want it to be easy to define more simple cases.

I want most of the behavior to just be handled by RestfulMiddleware, but delegate certain routes to the special case handler. Obviously, I could a combination of koa-mount and other tools to do this, but the above was my naive typing on the first try, and I was very surprised to find that the router didn't behave as expected.

ceeji commented 7 years ago

I also met this question. Using use for common tasks is very useful.

For example I want all requests which is not matching other routers go to static file serve middleware. But now it is not executed.

Is there any workaround?

demurgos commented 7 years ago

@dts You should be able to achieve your behaviour by simply swapping your two lines if the proposed change is implemented. On the other hand, to get catch-all behaviour on a sub-route with a router, it is way more complex currently. (Implementing the change to match express's Router would allow both use cases to work easily).

smcmurray commented 7 years ago

Router.prototype.use registers middleware with methods=[]

Router.prototype.match sets matched.route to false, only letting it be true if the route has a matching path and a method. So, for router.use() routes, matched.route will always be false.

Consequently, Router.prototype.routes will skip that middleware because if (!matched.route) return next();

So as near as I can tell, router.use puts your middleware in a blackhole? Am I wrong?

This bug seems to have been introduced in commit 9e742f1153a04d6cefa80fc550de62f62c0a8721

octatone commented 7 years ago

Is there any movement planned for this issue to resolve the inconsistent behavior between app.use and the blackhole-ness of router.use? I also ran into the unexpected requirement of paths for router.use where I was hoping to simply compose middle where where needed for specific routers.

It is really weird to me that router.use(() => dosometing) doesn't just execute for all route definitions on a given router instance.

EduardoRFS commented 7 years ago

@octatone https://github.com/EduardoRFS/koa-Router i'm using that in production, in almost a year now, don't have some features but works exactly express router. And available as npm install koa-Router

manuel-di-iorio commented 6 years ago

+1 for consistent behavior. I have the same use case of @amacneil

const router = new Router({ prefix: "/api" });
router.get('/users', myUserRoute);
router.use(async (ctx) => {
  ctx.status = 404;
});

I ended up for now by using app.use() after the koa-router stack:

app.use(async (ctx, next) => {
   if (ctx.url.startsWith("/api")) return ctx.status = 404;
   await next();
});
noinkling commented 6 years ago

Yeah I've also had to do something similar.

app.use(apiRouter.routes());  // router with prefix: "/api"
app.use(mount('/api', (ctx) => ctx.throw(404)));

It's pretty absurd that the 404 handler can't live inside the router itself - I shouldn't have any occasion to use koa-mount if I already have a router.

You can maybe make an argument for not changing the behaviour of the non-prefix case, but as soon as you have a prefix then the only sensible thing to do is run a use middleware on any path beginning with that prefix - it doesn't make sense to ignore it.

YanLobat commented 6 years ago

@manuel-di-iorio Just executed the following code snippet and it works as expected

const sharedRouter = new Router();
sharedRouter.use(async (ctx, next) => {
  ctx.status = 404;
});
sharedRouter.get('/hello', (ctx) => {
  console.log(ctx.body);
  ctx.body = 'Hello World!';
});

I mean when I tried to get /hello page I got 404 status

YanLobat commented 6 years ago

@noinkling What are you trying to achieve here?

YanLobat commented 6 years ago

@EduardoRFS regarding to your issue(Maybe it's outdated though)

WHY? When i use 'use' function from koa-router, and not exists a 'get' handler, the middleware not called

How do you suppose middlewares without particular routes in router instance should be handled? If you mean records like router.use('/mycustomroute', async (ctx, next) => {}); so then yeah, you should use router.all() (koa-router allows you do it, while pure koa not, because it is based on middlewares only)

YanLobat commented 6 years ago

@EduardoRFS Also, if you want any limitations on your middlewares(I mean middleware applicable only to particular routes) then you'll write router.use('/mycustomroute', async (ctx, next) => {}); and route logic(GET, POST and etc.) afterwards in particular handler.

inca commented 6 years ago

@YanLobat I am sorry, but I think you should consider reading the original thread more carefully. It has been explicitly stated that middleware functions are not invoked unless one of the route matches — and this is the only confusing behavior being discussed here (mostly because most users simply expect it to behave in a same way Express middlewares did).

YanLobat commented 6 years ago

@inca Maybe my fault, but still don't get complaints. Why middleware should be invoked if there is no any matching routes for this router instance? For 404 handlers in express as far as I remember We(devs) used also global app handler.

inca commented 6 years ago

Why middleware should be invoked if there is no any matching routes for this router instance?

Because router is a middleware itself. Once the control flow is delegated to it, its expected behavior is to execute its own middleware stack.

In general, routers are expected to be composable in a same matter as Koa app itself can be composed of other sub-apps.

A most common example here is:

rainbowsRouter.get('/', ctx => ctx.body = 'Behold the rainbows!');
rainbowsRouter.use(ctx => ctx.body = 'Rainbows are not there yet :sob:');

app.use(mount('/rainbows', rainbowsRouter.routes()));
app.use(mount('/ponys', ponysRouter.routes()));

Now, request like GET /rainbows/foo/bar/baz is dispatched to rainbowsRouter by mount. Given that routers are supposed to be composable and self-sufficient, the expected outcome is for a middleware to match the request, even though router does not declare the /foo/bar/baz route.

What happens instead is that rainbowsRouter exits silently with no middleware executed.


I'm not interested in arguing whether these expectations and assumptions are valid and correct — I realize this is mostly a subjective thing, driven by past experience and symmetry arguments to Koa itself. But the fact is: this issue draws a lot of attention, and remains open for more than a year now with no resolution or conclusions.

It's ok for koa-router to behave as it does, and I'm sure lots of people are happy with it. Everyone else at least deserve to be aware of alternative solutions:

YanLobat commented 6 years ago

@inca Thanks for detailed response! I didn't try to argue with you, but worth saying that author mentioned this behavior in readme

fritx commented 6 years ago

Thanks, router.all('*', notFound) did the trick!

<router url="/api">
  <api :use="apiMiddleware"></api>
  <api url="/users">
    <api method="get" url="/list" :use="listUsers"></api>
    <api method="post" url="/create" :use="[koaBody, createUser]"></api>
  </api>
  <api method="all" url="*" :use="apiNotFound"></api>
</router>

-- https://github.com/fritx/vue-server-demo

jonaskello commented 6 years ago

I too got bit by this today. I tried to beat it with hours of console.log. But after reading this whole issue I understand this problem can not be beaten with the otherwise so mighty console.log :-).

My use-case is that I have a pattern where I create a separate router for every sub-path. One of my sub-paths was something that should proxy to another REST service. Basically if /my-app is called on my site then I should proxy to / on another host. So I did something like this:

const app = new Koa();
const mainRouter = new Router();
const myAppProxyRouter = createMyAppRouter("http://api.my.app");
mainRouter.use("/my-app", myAppProxyRouter.routes());

function createMyAppRouter(baseUrl) {
  const router = new Router();
  var proxy = require('koa-better-http-proxy');
  router.use("*", proxy(baseUrl, {}));
  return router;
}

This would result in the black hole problem mentioned above. I think @smcmurray has the best description of what is going on in his comment above.

I like this approach since each router is unaware of their place in the main router. However I have now realised that I should probably use separate Koa apps and compose them using koa-mount. But I think at least some warning or something should be written to the console when you register a middleware that will never be called. That would have saved me several hours today.

LvChengbin commented 6 years ago

So does it mean that the router.use is useless in Koa?

Telokis commented 5 years ago

That would be very nice to have this fixed.