express
express copied to clipboard
expressjs
Allow to bypass middleware to last layer with next('last')
It adds a built-in way to skip to the final route of a layers stack (see this closed issue #1662).
Based on my experience, i was thinking it could be interesting to have this simple feature available.
Example :
function middleware1 (req, res, next) { next('last'); // will call lastMiddleware }
function middleware2 (req, res, next) { next('last'); // will call lastMiddleware }
function middleware3 (req, res, next) { next('last'); // will call lastMiddleware }
function middleware4 (req, res, next) { next('last'); // will call lastMiddleware }
function lastMiddleware (req, res, next) { next('last'); // will call the next error middleware }
app.get('/my_path', middleware1, middleware2, middleware3, middleware4, lastMiddleware);
In case of errors, this can allow to break the middleware cycle and jump to the last middleware of the route directly without the need to put a condition if (res.locals.err) in each middleware to jump to the next one, or to call an error-handling middleware with next(err); and to manage everything there, as redirection, render, controller call depending of the error, or to use next('route') with a route duplication each time... (which are three solutions not always very convenient).
(maybe there is already a way to do that ? I didn't find it yet. See this question and my own question)
Let me know your thoughts about it.
Hey @BigFax, thanks for the contribution! While interesting, I think we had wanted to move away from the "magic strings". I think the feature you want seems great, so maybe if we could think of an api to achieve this without the magic string last?
Also, I would look at opening this PR against the router project, as it is where this would land in the 5.x branch. https://github.com/pillarjs/router/
Note: I cannot find the specific issue where "magic strings" were discussed. So feel free to shoot me down if others disagree.
I don't know the discussion either, but I recall the same thing. The main annoying danger is that most people do like doThing((err) => { if (err) return next(err); ... } so random things are passed in as the argument, causing potential extremely hard to debug problem if it gave a string that matched a magic one.
That said, I there there is a more general issue with this solution, in that is is not general enough. For example I can 100% forsee a follow up request if this lands that person x needs it to jump to the 2nd to last middleware, not the last one.
I was reading your SO question and I think I know a workable solution based on your comments. I don't have a SO account to respond there and don't want to mix that into the conversation in this PR. You're welcome to open a new issue with the full background to explore that if you like.
I proposed a new "magic string" because it seemed to me to stick with the vision of the project and the normal behavior of the next() function (as 'route' and 'router' already existed). Now, in my personal opinion, "magic string" is not a good thing and should be removed from the next() function. I would prefer a function next() without parameter if it is not an error. I think it would be more logic and less confusing. @wesleytodd Do you think of a particular way to implement an API for this issue ?
@dougwilson When i was thinking to call next('last');, it is more to end the route that to call a specific middleware. When a route ends, i think that the expected behavior is often to end the request by doing the render. In my understanding of Express, the render is made mostly in the last route middleware if everything goes as planned. So jump all middlewares to arrive to the last one in the goal to render the "normal" response but with error messages about where it broke was my thought. I think it's a common behavior, which is less the case than jumping from specific number of middleware (which can still be achieved with next('route') if really need it ? Even if it duplicate routes). It's my point of view, but maybe my understanding of things is not right here.
So jump all middlewares to arrive to the last one in the goal to render the "normal" response but with error messages about where it broke was my thought. I think it's a common behavior, which is less the case than jumping from specific number of middleware. It's my point of view, but maybe my understanding of things is not right here.
Right, that's the problem: you're just thinking about your specific layout. Express makes no such requirement and you can easily have multiple handlers that do rendering based on perhaps what type of user was loaded for example. This is the type of thing someone who is already splitting up everything into separate middlewares is going to do, which is the use-case you're presenting. Just as a simple example of two renders at the end of a route:
app.get('/thing', [...],
(req, res, next) => { if (!req.user.isAdmin()) return next(); res.render(...) },
(req, res, next) => { res.render(...) }
)
Typically the if (...) is wrapped using a helper, like so:
const ifUserIsAdmin = (handler) =>
(req, res, next) => { if (!req.user.isAdmin()) return next() else handler(req, res, next) }
app.get('/thing', [...],
ifUserIsAdmin((req, res, next) => res.render(...) }),
(req, res, next) => res.render(...) }
)
The other thing is just a functional issue with the PR itself, which is that it doesn't work at all in the common case of handling a route-specific error handler on the end of your route (because even your rendering handler may throw an error). Example:
app.get('/thing', [...], (req, res, next) => res.render(...), (err, req, res, next) => ...)
I got your point.
To me, next('last') was like next('route'), a way to manage routing cycle. In your first example, this is indeed one way of doing rendering. In this case, next('last') doesn't make sense. Now, as you said yourself, Express makes no such requirement, so you can totally manage rendering with a custom function where next('last') would be great, no ? This is the same for next('route') and next('router'), it depends of your routing organization and sometimes it doesn't make sense to use them. In fact, i think that next('last') is one way to help to manage routing without the need to put "if" conditions in each middleware (as i mentionned in case of errors, but also in case of roles check...).
About the PR issue, you are right. One fix could be :
// bypass middleware to last layer
if (err && err === 'last' && idx < stack.length) {
for (let i = stack.length - 1; i > idx; i--) {
if (stack[i].handle.length < 4) {
idx = i;
break;
}
}
return stack[idx++].handle_request(req, res, next);
}
Hi,
I think we could rid of "magic string" and pass to next function another function like:
const checking = () => { return 'route'; // or return -1 to skip to the final }
and in next function we could do this:
if(typeof err === 'function'){ err = err(); }
This is a bit of an older issue, but I think an even better api would be something like:
-
next.router() -
next.route() -
next.last()
If we move forward with this I think that would be the api which would get my vote.
@BigFax @dougwilson what's the status on this? Is it still under consideration? I saw it needed tests and would be happy to help with that if appropriate!
@bpernick I don't know. I didn't follow this subject since a long time. I still think that if magic strings are kept, next('last') make sense, but that magic strings should be replaced by another API solution as proposed above.
This is a bit of an older issue, but I think an even better api would be something like:
next.router()next.route()next.last()If we move forward with this I think that would be the api which would get my vote.
Most of my current work requires building APIs with express and I see how this can be useful to me as a developer. But I believe this idea would lead to bad design choices when implementing it. Double edged sword imo. If at all it is acceptable, then the API makes more sense than the magic string.
