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

Calling next('route') in middleware is ignored if a router is next in the chain #2591

Closed
cryptism opened this issue Mar 13, 2015 · 5 comments
Assignees
Labels

Comments

@cryptism
Copy link

The following test :

describe('when next("route") is called on a mounted route with prefixed middleware', function(){
  it('should jump to application route', function(done){
    var app = express();
    var router = express.Router();

    function fn(req, res, next){
      res.set('X-Hit', '1');
      next('route');
    }

    router.get('/bar',  function(req,res,next){
      res.end('failure');
    });

    app.use('/foo', fn, router);

    app.get('/foo/bar', function(req, res){
      res.status(200);
      res.end('success');
    });

    request(app)
    .get('/foo/bar')
    .expect('X-Hit', '1')
    .expect(200, 'success', done);
  })
})

When applied to test/app.router.js will fail. Instead of jumping to the next registered '/foo/bar', it ends up calling the mounted '/bar' route anyway.

@dougwilson
Copy link
Contributor

This is correct. Things inside app.use() are not routes, so next('route') is not meaningful.

@dougwilson
Copy link
Contributor

To sum up what next('route') actually means is that if you call it, it simply skips the rest of the handlers in the currently executing route, rather than actually jumping to the next thing in Express that is a route. You can think of it as actually exit_route().

app.use defines middleware, rather than routes. Just bunching them up into a single line doesn't group them, though, just a short-hand for app.use(fn).use(router).

If you provide the use-case for what you are trying to achieve, I can certainly point you into the right direction for implementing it.

@dougwilson dougwilson self-assigned this Mar 13, 2015
@dougwilson
Copy link
Contributor

I double-checked in the docs (http://expressjs.com/4x/api.html) to see if something in there was mis-leading, but we clearly only document next('route') when you're using it from a app.get, app.post or app[method], not from within an app.use (which has no meaning). Please also let us know if there is a way we can clarity our documentation regarding this as well.

@cryptism
Copy link
Author

Looking at it now, it appears quite obvious this wouldn't work.

I can see my misunderstanding came from app.use(some_route, fn1, fn2) would be used to mount a chain of middleware into a route, which functionally ends up doing something very similar to app.all and I didn't bother to draw a distinction between anything using app.use to what was happening in app[method]. I managed to forget about the reason why app.all is used entirely. I suppose what the document could do with is just a short paragraph in the guide explaining what the difference is in using app.use in this way and actually defining routes. Middleware that is applied to app[method] in my mind contractually resolves the HTTP response (is this true?), so next('route') would in this sense be a deferral of responsibility to the next matching route.

Anyway, my use case came from trying to build authentication middleware that would have routers either throw an error or continue based on the route that was called, based on a certain condition. The idea was to leave it up to the application route definitions to determine what would happen to each route without relying on state or having the router care about whether or not they've been authenticated

@dougwilson
Copy link
Contributor

Ah, I see. If you have any suggestions for improving our documentation, we have a great team over at https://github.com/strongloop/expressjs.com who can take care of it :) Just file an issue/PR with your suggestions. We try to think of everything, but since we are not new users, it's hard to think in their shoes, so getting feedback from people like you on our documentation is always great :)

As for the use-case, I'm not sure if I 100% follow, but two different ways to directly translate app.use('/foo', fn, router); would be:

app.use('/foo', function fn(req, res, next) {
  if (/* some condition you would call next('route') */) {
    return next();
  }

  return router(req, res, next);
});

or if you want to keep things encapsulated:

app.use('/foo', createFn(router));

function createFn(proceed) {
  return function fn(req, res, next) {
    if (/* some condition you would call next('route') */) {
      return next();
    }

    return proceed(req, res, next);
  };
}

@expressjs expressjs locked and limited conversation to collaborators Mar 24, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants