-
-
Notifications
You must be signed in to change notification settings - Fork 16.5k
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
Method for bailing out of a router and returning to the previous middleware level #2241
Comments
Right. It's a good idea, but especially what the wording should be is up in the air. Even though "router" is too close, it still doesn't seem like an abstract enough term to me :) |
Maybe |
|
That's just an implementation detail and I'm not sure the name of that in the codebase make sense anyway :) Users certainly won't be seeing "layer" pop up anywhere. |
Could we get a bit more detail about why this is seen as useful or the specific use case? |
I believe the use-case from @ChiperSoft is conditional middleware groups or something like that, IDK. |
Yes, the original discussion was related to my package, pitstop, which lets you group collections of middleware that will only evaluate under conditional criteria (eg, only running session related middleware when a session cookie is present). If it were possible to bail out of a router, then pitstop wouldn't be needed, you could just create a middleware only router where the first middleware function tests the condition and either bails out of the router or continues evaluating. |
Doug asked me to provide an example. var passport = require('passport');
var flash = require('connect-flash');
var expressSession = require("express-session")({
key: 'session'
});
var sessioned = express.Router();
sessioned.use(require('cookie-parser'));
sessioned.use(function (req, res, next) {
if (req.cookies.session) {
// session cookie exists, let the router continue running the rest of the middleware
next();
} else {
// no session cookie found, no reason to continue, bail out of the router
next('router');
}
});
sessioned.use(expressSession);
sessioned.use(flash());
sessioned.use(passport.initialize());
sessioned.use(passport.session());
var app = express();
app.use(sessioned);
app.get('/login', expressSession, function (req, res) {
res.render('login');
}); In this scenario, every request gets its cookies parsed, but sessions, passport and flash only process for the request if the request already has an active session, and a session is only created if the user visits the login page. This saves us from creating sessions for every single visitor, regardless of if we actually need them. |
I had a similar problem and the solution I came with was making use of the fact that routers are just request handlers. The code @ChiperSoft provided would look like var whenSession = express.Router();
whenSession.use(expressSession);
whenSession.use(flash());
whenSession.use(passport.initialize());
whenSession.use(passport.session());
var sessioned = express.Router();
sessioned.use(require('cookie-parser'));
sessioned.use(function (req, res, next) {
if (req.cookies.session) {
whenSession(req, res, next);
} else {
next();
}
}); In our case the problem was that we had to support in the same api two completely different datasources and from the API's user point of view there had to be no difference. The thing is that one of the two datasources is likely to get removed in the future so I wanted to write the code in a way I could just remove the code corresponding to that datasource. I ended up writing something that looks like the following: var legacyRouter = require('./legacy');
var modernRouter = require('./modern');
router.use('/user/:userId', function(req, res, next) {
if (parseInt(req.params.userId) < 10000) {
legacyRouter(req, res, next);
} else {
modernRouter(req, res, next);
}
}); This way when we stop supporting the legacy datasource I don't need to go through all the route handlers removing legacy stuff but instead I can just do the following: router.use('/user/:userId', require('./modern')); |
👍 A specific use case for me would be setting up a |
Vhost already does this, no? On Thursday, December 25, 2014, Kevin Ingersoll notifications@github.com
|
Wondering if these was any ground made up on this? I was actually surprised to see that it did not already do this - In my case, I quite like the idea of mounting multiple routers onto the same hostname and letting them figure out themselves if they should take action or not. Most of our routes have separate views (logged in, logged in w/ admin, logged out, etc), so it would be much nicer to encapsulate all of the logic here inside the router with a single middleware statement (i.e. will this router accept this request? if yes, continue, if no, bail out and proxy to the next router in line). It's a very nice way to sort through view logic, progressing from full permissions for some requests to no permissions (public). The fact that it does not already do this leads me to think that I'm thinking about this wrong? Is the current way of doing this really just having the single route and then manually checking conditionals for each request over and over again? I understand that this is exactly what what I'm asking would suggest, but organizationally it's quite different. |
+1 |
I wanted to add another use case for this. At the moment, we use our route to set req.body values. We use middleware to add bits an pieces to the response after the router has set the body. For instance, we add metadata to the body, etc. The result is that we call next() in the router instead of send() and at the moment, there's no way to prevent the next route from also matching the request. |
Per conversation with @dougwilson on IRC, it would be very useful to have an equivalent for
next('route')
, but at the router level. Calling it would cause a router to skip all remaining middleware and routes on that router.This would allow for conditional execution of middleware, such as session handling and session related middleware only being evoked for users with an existing session cookie.
next('router')
could work, but that might be too close of terms.The text was updated successfully, but these errors were encountered: