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.

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

Why 'use' middleware not called? #257

Open
EduardoRFS opened this issue Apr 3, 2016 · 34 comments
Open

Why 'use' middleware not called? #257

EduardoRFS opened this issue Apr 3, 2016 · 34 comments

Comments

@EduardoRFS
Copy link

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
Copy link

inca commented Apr 3, 2016

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

@EduardoRFS EduardoRFS changed the title Why use middleware not called? Why 'use' middleware not called? Apr 3, 2016
@jbielick
Copy link
Collaborator

jbielick commented Apr 3, 2016

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
Copy link

inca commented Apr 4, 2016

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:

  • it works akin to Express (all routes/middleware are executed in series, routes conditionally, middleware — not)
  • in my head router.use(handler) is kinda equivalent of router.all('*', handler), with small optimisation of avoiding matching

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
Copy link
Author

@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
Copy link

inca commented Apr 4, 2016

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
Copy link

alekbarszczewski commented Apr 21, 2016

👍 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
Copy link
Collaborator

jbielick commented Apr 22, 2016

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
Copy link

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
Copy link

Prior discussions of this issue for reference: #182 #239

@defunctzombie
Copy link

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
Copy link

inca commented May 19, 2016

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
Copy link

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
Copy link

inca commented Aug 17, 2016

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
Copy link
Author

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
Copy link

dts commented Aug 22, 2016

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
Copy link

ceeji commented Nov 6, 2016

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
Copy link

@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).

UnwashedMeme added a commit to iDigBio/idigbio-search-api that referenced this issue Jan 27, 2017
When I tried to make the conditional request only apply to the /v2/
prefix I had mistakenly thought that koa-router `.use(path, middleware)`
applied the middleware if the path was a prefix of the request.

This is wrong (and I'm not the only one:
ZijianHe/koa-router#257); so make a new
middlware that is just a prefix matcher and applies the given
middlware, otherwise skip to the next one. Use this to apply the
lastModified conditional request guard to only the `/v2/` prefix
@smcmurray
Copy link

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 9e742f1

@octatone
Copy link

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
Copy link
Author

@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
Copy link

manuel-di-iorio commented Sep 16, 2017

+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
Copy link

noinkling commented Sep 16, 2017

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
Copy link

@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
Copy link

@noinkling What are you trying to achieve here?

@YanLobat
Copy link

@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
Copy link

@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
Copy link

inca commented Sep 19, 2017

@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
Copy link

@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
Copy link

inca commented Sep 19, 2017

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
Copy link

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

@fritx
Copy link

fritx commented Sep 24, 2017

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
Copy link

jonaskello commented Oct 23, 2017

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
Copy link

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

@Telokis
Copy link

Telokis commented Sep 17, 2018

That would be very nice to have this fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests