Feature / Feedback: Private middleware stack per router

This feature request is for adding convenience in the following situation: Suppose you have a hierarchy of express apps and want to share middleware per route within a specific app/router.

var app = express(),
    middleApp = express(),
    leafApp = express();

app.use(function(req, res, next) {
  // some global middleware
  next();
});

app.use('/middle', middleApp);
middleApp.use('/leaf', leafApp);

var commonMiddleware = function(req, res, next) {
  // some private stuff that should only happen in middleApp
  next();
});

middleApp.get('/something', commonMiddleware, function(req, res) { ... });
middleApp.get('/another', commonMiddleware, function(req, res) { ... });

leafApp.get('/evenmore', function(req, res) { ... });

I think it would be nice if .use() could be used to store middleware that is only executed by routes within a given app/router, something like this:

middleApp.use(function(req, res, next) {
  // something only executed by routes within middleApp
  next();
}, true);

Is this something that might be useful for others, or do you feel there's a better way to accomplish this? It becomes more useful if there's many routes. I've got an implementation of it here if you'd like to see it in action.

You might need to elaborate on the issue you're trying to solve, since everything you wrote, from what I understand, is already supported and you're code almost looks correct already. What behavior does this flag change? I tried checking out the code you linked, but the only change I see is that you're making the flag put middleware before the others - which is confusing to me.

I will try to get a more elaborate example, but the gist of it is that this:

var commonFn = function(req, res, next) {
  // do stuff here that only this app needs, sub-apps don't want to call this middleware
  next();
};

middleApp.get('/foo', commonFn, function(req, res) { res.send(...) };
middleApp.get('/bar', commonFn, function(req, res) { res.send(...) };
middleApp.get('/baz', commonFn, function(req, res) { res.send(...) };

becomes:

middleApp.use(function(req, res, next) {
  // do stuff here that only this app needs, sub-apps don't want to call this middleware
  next();
}, true);

// Each of the following implicitly execute the common middleware just before 
// the route-level middleware (but after the global app/router middleware)
middleApp.get('/foo', function(req, res) { res.send(...) };
middleApp.get('/bar', function(req, res) { res.send(...) };
middleApp.get('/baz', function(req, res) { res.send(...) };

It looks almost the same, but it becomes very handy when you have lots of routes or have built a library using express and you don't want a particular piece of middleware to be executed by sub-apps. It's a convenient way to avoid remembering that every route within an app needs to have commonFn.

@demalus

Hi @demalus , how will the following code work with your given proposal?

middleApp.use(function(req, res, next) {
  // do stuff here that only this app needs, sub-apps don't want to call this middleware
  req.someProperty = true;
  next();
}, true);

middleApp.use('/foo', function(req, res, next) {
  // what will the value of req.someProperty be in this example?
  console.dir(req.someProperty)
  next();
});

// Each of the following implicitly execute the common middleware just before 
// the route-level middleware (but after the global app/router middleware)
middleApp.get('/foo', function(req, res) { res.send(...) };
middleApp.get('/bar', function(req, res) { res.send(...) };
middleApp.get('/baz', function(req, res) { res.send(...) };

And as an additional question, does the true at the end work for when you have middleware scoped by path (i.e. app.use('/foo', function(){}, true))?

Just a note, it seems like it'll be quite confusing for this method to create a separate out-of-band stack which runs before/after the other stack. It breaks the order of definition quality that Express has currently. I do understand your proposal now though, but I'm still missing the whole "sub-apps" part. How will you know that in the future a sub-app will be called? I think it's quite a complex addition for something that already exists. Does using all solve your use-case? Since .all will only match on an exact route, it won't be executed with sub-apps.

Edit: I thought a bit more and I don't think it does exist, but it sounds like an incredibly hard thing to solve properly (E.g. imagine every uses or route before the sub-app). Seems easy to support negated routes or isolated sub-apps. app.use('!/subapp', something). I think the thing that is missing is the definition of what exactly this achieves - what's the goal and define what "private" means.

@dougwilson

@dougwilson I missed path-scoped middleware in my implementation. Ideally req.someProperty would be undefined if you hit a route that was in a sub-app like leafApp.get('/', ...), but if you hit any route middleApp.post('/', ...) it would be true. I could make my implementation work with this, but I don't think it actually makes sense because it's just supposed to be syntactic sugar for passing in a list of middleware with the route.

I agree with @blakeembrey as well about confusing the current API. I'm going to rework my approach and get back to you guys because I think this feature would be helpful for others.

I think it works best when you have something like a library that defines some express apps / routers, and a downstream project that might add routes. See this gist

I think maybe the word private is confusing and overloading the use method isn't the best idea, but having a new method register functions as a stack that gets used by each route within that app/router is very convenient.

@demalus

Hi @demalus , I was to let you know I'm not out-right dismissing your idea, more like trying to iron it out. Before even getting read to add an API like this, we want to get it ironed out, work well with all existing Express routing features, etc.

Ideally req.someProperty would be undefined if you hit a route that was in a sub-app like leafApp.get('/', ...), but if you hit any route middleApp.post('/', ...) it would be true. I could make my implementation work with this, but I don't think it actually makes sense because it's just supposed to be syntactic sugar for passing in a list of middleware with the route.

I'm not sure what you mean. In my example, all those were on middleApp, there were no other apps involved in the example. Does that make a difference in your answer? Your answer doesn't make sense to me based on the code I provided.

Also, I just thought of a new case, and wondering how it would work:

middleApp.use(myFn1, true);
middleApp.get('/foo', function(req, res) { res.send(...) };

middleApp.use(myFn2, true);
middleApp.get('/bar', function(req, res) { res.send(...) };
middleApp.get('/baz', function(req, res) { res.send(...) };

If it were to play with the existing Express paradigms, like "always run strictly in declared order", then to me, the above should end up identical to the following, right?

middleApp.get('/foo', myFn1, function(req, res) { res.send(...) };
middleApp.get('/bar', myFn1, myFn2, function(req, res) { res.send(...) };
middleApp.get('/baz', myFn1, myFn2, function(req, res) { res.send(...) };

@blakeembrey

Sorry if my explanations are unclear.

First, to clarify my last response. I was assuming there was more code to your example similar to mine. This is what I imagined the rest of the code was:

var app = express(),
  middleApp = express(),
  leafApp = express();

middleApp.use(function(req, res, next) {
  // do stuff here that only this app needs, sub-apps don't want to call this middleware
  req.someProperty = true;
  next();
}, true);

middleApp.use('/foo', function(req, res, next) {
  // what will the value of req.someProperty be in this example?
  console.dir(req.someProperty)
  next();
});

app.use('/middle', middleApp);
middleApp.use('/leaf', leafApp);

// Each of the following implicitly execute the common middleware just before 
// the route-level middleware (but after the global app/router middleware)
middleApp.get('/foo', function(req, res) { res.send(...) };
middleApp.get('/bar', function(req, res) { res.send(...) };
middleApp.get('/baz', function(req, res) { res.send(...) };

What I was trying to say about this is best illustrated as the outputs of some requests:

/: Nothing (404)
'/middle': Nothing (404)
'/middle/foo': req.someProperty is true and it gets logged (note that I did not handle "private middleware" scoped by a path in my initial comment)
'/middle/bar': req.someProperty is true and it gets logged in my implementation, but if scoping worked it should not get logged
'/middle/baz': Same as /bar above
'/middle/leaf': req.someProperty is undefined

Given the comments that you and @blakeembrey gave me, I don't actually think it should work the same way as I have suggested / implemented - it should not overload .use() because it's supposed to be a shortcut for automatically appending functions to a route (not as middleware to a router/app). In the gist I linked to, I think I have illustrated a better use case. So instead it would become:

Old: middleApp.use(fn, true)  // Problematic because this function supports scoping by path, but we don't use it
New: middleApp.usePrivate(fn)  // This will only accept functions, so we don't worry about scope.

To answer your second question: that is one way it could work. I was actually imagining a different case where all three of them result in .get('/..', myFn1, myFn2, function(req, res) { ... }. This is mostly a design decision as it should be really easy to modify what I've done to achieve your result. I think the big difference here is that the "private" middleware is loaded lazily. For example:

middleApp.use(myFn1, true);  // middleApp.privateStack = [myFn1];
middleApp.get('/foo', function(req, res) { res.send(...) };  // New Layer with route.stack = [fn(res,req){}]

middleApp.use(myFn2, true); // middleApp.privateStack = [myFn1, myFn2]
middleApp.get('/bar', function(req, res) { res.send(...) }; // New layer with route
middleApp.get('/baz', function(req, res) { res.send(...) }; // New layer with route

When /foo Layer is handled from above, it can either call the private middleware from middleApp followed by route.stack, or we could have just copied the private middleware into the route when it was created to get your result.

I might still be missing something obvious, but my next question is now - what's the difference between this and just calling .use before the ordinary middleware? From what I'm reading, the way you intend to achieve "private middleware" is by pulling to the beginning of the stack. What happens when the middleware doesn't have a handler and it 404s, does it still fall through as normal?

I'm reading this as "isolated middleware", where you can guarantee the middleware wasn't affected by others, but I could be wrong - please correct me if that is the wrong read of this feature. However, with "isolated middleware" and this solution, it only sounds like a temporary unless the 404 cases, etc are thought about. In which case, what about something like app.mount(subapp) instead of "private". Private is kind of confusing to me, since it doesn't really tell me anything about the feature. If there's no special handling, perhaps this is a case for improved documentation about middleware and declaration ordering.

@dougwilson

I think my terminology is off. There are two stacks in the express library (correct me if I'm wrong):

Router.stack here: this is a stack of Layers which have the functions that are passed in as middleware
route.stack here: the comments in that file say that this stack of layers behaves like middleware, but is this usually referred to as "middleware" or is it something else like "route handlers"?

I'm proposing adding additional syntax for the second bullet, the route handlers. Something like @dougwilson 's example, this:

var app = express();

var myFn1 = function(req, res, next) { next() };
var myFn2 = function(req, res, next) { next() };

app.get('/foo', myFn1, myFn2, function(req, res) { res.send(...) }

could also be specified by doing something like:

var app = express();

// Two functions equivalent of myFn1 and myFn2
app.usePrivate(function(req, res, next) { next() }, function(req, res, next) { next() });
// Note: usePrivate might be a bad name because the word private doesn't really describe this feature well enough

app.get('/foo', function(req, res) { res.send(...) });  // myFn1 and myFn2 are implicitly called before the handler we passed into .get()

Ok, completely understand now. I don't think it's simple to achieve that. Yes, there's two stack properties but they all contribute to a nested hierarchy of "stacks". E.g. that route stack will actually be nested inside the application stack - it's actually all of Express right there, stacks in stacks in stacks.

I think I'm back at the same question, sorry! What does it mean for the private middleware to run? They should be implicitly called only when a route matches. What is a route match defined as - does that mean the last function is the route is special here? Have you look at routers?

Anyway, it does look like I was thinking back to front from your proposal and that you wanted isolation, but instead you want implicit middleware execution before only route handlers. Correct?

Edit: Sorry it took so long. Perhaps the right word/method is more like app.with(fn). That'll build a "private stack" and whenever route handlers are called the private stack is prepended. There's still issues with the current stack handling though. When a route handler does not respond and passes along next. There might need to be some dynamic protection from double calling this middleware (maybe a use-case for router exiting, but that'll break backward compatibility). This implementation just ends up just being a simple stack within a stack.

So the reason why I ended up calling this a "private stack" is because I'm thinking of it as this: whenever a route gets dispatched to (which is typically last because you invoke .method() after any .use() calls), it should use it's own stack as well as the "private" stack from the Router that the route is registered in. Since the route's stack ends with a handler that terminates the chain, the correct order is to use the Router's "private" stack, followed by the route's stack.

What it means for private middleware to run should be no different than doing this:

myApp.get('/foo', fn1, fn2, fn3);

If that route doesn't match, those three functions won't run. Likewise, if instead you had setup the "private router" stack (on myApp.router), none of those private ones will execute. The "private middleware" will only execute if a route is being executed. So to answer your question, it should fall through like normal.

Yes, I think implicit execution of an array of functions whenever a route is executed is a good way to look at this. The one additional piece of information though is that the array of extra functions is not global - instead each Router has it's own array of functions that should be used by routes within that Router.

app.with(fn) is pretty good, but I wonder if there is a way to make it clear that the functions you are passing in are being used for routes-only, not the general middleware stack (maybe that's just a documentation issue).

Regarding your last statement of a route handler not responding - is there an example of problem you are mentioning? I haven't encountered any errors like that before. I'll dig into the code a little bit more and try my luck at a new implementation that is clearer and hopefully addresses the problems you two have brought up.

It's not a problem, just something that's possible and needed to be thought about when you say a "route handler". For example:

app.usePrivate(fn1) // Using your fn.
app.get('/foo', function (req, res, next) {
  return next()
})
app.all('/foo', function (req, res, next) {
  res.end()
})

How do you make sure it's not run the private stack twice here. What about making app.with(fn) a specialized router that you need to chain to solve your other problem?

var usePrivate = app.with(fn1, fn2)
upPrivate.get('/foo', handler)

Basically, it's more than just knowing that it's going to run, you also need to know that it's the last in the stack (assuming your being implicit with everything). If you make it explicit like above, I think it'll just be a matter of documenting the feature. This also becomes a more useful feature since it's no longer just about routes/middleware/apps/etc and is actually just syntactic sugar for saying this stack comes before any route in here. In an interesting way, this is how Express is/can actually be built: function express () { return Router().with(expressSetUp) }.

Ah, apparently I had a misunderstanding of what .all() was doing. Now I understand what you were initially suggesting. My request is actually pretty similar to all, except mine is router-centric instead of path-centric.

For example:

var app = express(), apiApp = express(), usersApp = express();

app.use('/api', apiApp);
apiApp.use('/users', usersApp);

app.all('/api', mySpecialFn);

As is:
/api/foo: Executes mySpecialFn
/api/users/foo: Executes mySpecialFn

Basically, the syntax of .all is desired but mySpecialFn shouldn't be called by /api/users/foo (because /users is the mount point for a different router).

Alright, back to your questions. I think it would actually be pretty easy to make sure the private stack does not get executed twice:

app.usePrivate(fn1)  // This line puts fn1 in app.router.privateStack

// This next call creates a new Layer with a new route. We pass in the router to this route.  
// We do the same for any other HTTP verb.
app.get('/foo', function (req, res, next) {  
  return next()
})

// This next call creates a new Layer with a new route, but does NOT pass the router.
// When this layer is executed, it will not know that it should execute a private stack before
// the route handlers.
app.all('/foo', function (req, res, next) {
  res.end()
})

I do like your idea of a specialized router, but wouldn't this have the same issues? For example:

var usePrivate = app.with(fn1, fn2)
usePrivate.get('/foo', function(req, res, next) {
  next()
})
// Couldn't you do the following?
usePrivate.all('/foo', ...)

Is the assumption that the specialized router would only support a subset of the router api, or just handle certain calls differently?

@blakeembrey

I think app.all(['/api', '!/api/users'], fn) would be similar to what I'm suggesting. Personally, I think having a method on the router like app.with is more convenient, but what you originally suggested @blakeembrey could be a potential solution for not cascading middleware to certain subpaths.

The reason I think this isn't as useful as having a method on the Router is illustrated better by this gist. If you are building a library with express and have a downstream project able to add routers, routes, etc, then the downstream project doesn't have to change anything in the library. If it were part of .all you'd have to change the lib when the downstream project makes additions. For example, if you add a router mounted at /api/services, you have to remember to go back to the library and add an exception like: app.all(['/api', '!/api/users', '!/api/services'], fn)

How do you program it to know not to pass the router down a layer? That's the thing here. Since .all handles all methods and get is only using the get, but during a get both get and all will be called. What if all should call the private stack at all?

Using the with syntax means it'd create a new router for you to use and you'd not use that when you don't want to with function.

var usePrivate = app.with(fn1, fn2)
usePrivate.get('/foo', function(req, res, next) {
  next()
})
app.all('/foo', ...)

Obviously this is not perfect, but it's declaring what you had in comments above.

@dougwilson

I've got two new implementations using the feedback you guys gave me. Feel free to play around with them and see if you guys think it would be something useful in express.

App/Router .with(implicitMiddleware): see this branch
This works similar to what I had before. You setup Apps/Routers exactly the same, but there is a new method on instances: .with([fns]). This is much better than overloading .use because it won't confuse the current functionality and doesn't allow you to use implicit middleware with a mount-path (doesn't make sense because the route has the path, not the implicit middleware). Note: this now behaves better with declared order as @dougwilson mentioned.

Full example:

var express = require('express');
var app = express();
var middleApp = express();
var leafApp = express();

middleApp.with(function(req, res, next) {
    req.someProperty = true;
    next();
});

function sendSomeProp(req, res) {
    res.send(req.someProperty || 'not set');
}

app.get('/', sendSomeProp);
middleApp.get('/', sendSomeProp);
leafApp.get('/', sendSomeProp);

app.use('/middle', middleApp);
middleApp.use('/leaf', leafApp);

var server = app.listen(1234, function() {});

// RESULTS:
//   GET '/': 'not set'
//   GET '/middle': 'true'
//   GET '/leaf': 'not set'

Implicit Middleware as a create option: see here
This is something closer to what @blakeembrey was suggesting. Instead of some wrapper though, I put the implicit middleware in the options that get passed into the router:

var router = new Router({
  caseSensitive: false,
  implicitMiddleware: [fn1, fn2, ...]
});

I also changed the create method of an express app to take in an options object similar to Router.

Full example:

var express = require('express');
var app = express();
var middleApp = express({implicitMiddleware: [function(req, res, next) {
    req.someProperty = true;
    next();
}]});
var leafApp = express();

function sendSomeProp(req, res) {
    res.send(req.someProperty || 'not set');
}

app.get('/', sendSomeProp);
middleApp.get('/', sendSomeProp);
leafApp.get('/', sendSomeProp);

app.use('/middle', middleApp);
middleApp.use('/leaf', leafApp);

var server = app.listen(1234, function() {});

// RESULTS:
//   GET '/': 'not set'
//   GET '/middle': 'true'
//   GET '/leaf': 'not set'

Some Notes

Both of the implementations do not handle fallthrough with implicit middleware. Example:

...

middleApp.with(function(req, res, next) {
    req.someProperty = 1;
    next();
});

function sendSomeProp(req, res) {
    res.send(req.someProperty ? '' + req.someProperty : 'not set');
}

app.get('/', sendSomeProp);
middleApp.get('/', function(req, res, next) {
    req.someProperty = req.someProperty + 1;
    next();
});
middleApp.all('/', sendSomeProp);
leafApp.get('/', sendSomeProp);

...

// RESULTS:
//   GET '/': 'not set'
//   GET '/middle': '1', should be '2' but the implicit middleware was called twice
//   GET '/leaf': 'not set'

It wouldn't be hard to make it so the implicit middleware is only called once during the lifetime of a request, but I feel like this could be more confusing than just knowing the pitfalls of chaining with .all.

Because this is a conversation regarding altering the router, the discussion is likely best moved over to https://github.com/pillarjs/router as the current state for any router changes is that changes to Express 4.x routing is landed in router 1.x and then cherry picked into this repository, otherwise landed in router 2.x, which is a direct Express 5.x dependency.

Reposted this to pillarjs/router #35

Navigation Menu

Repository navigation

Feature / Feedback: Private middleware stack per router #2828

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Feature / Feedback: Private middleware stack per router #2828

Description

Activity

blakeembrey commented on Dec 15, 2015

frankxw commented on Dec 15, 2015

dougwilson commented on Dec 15, 2015

dougwilson commented on Dec 15, 2015

blakeembrey commented on Dec 15, 2015

frankxw commented on Dec 16, 2015

dougwilson commented on Dec 16, 2015

frankxw commented on Dec 16, 2015

blakeembrey commented on Dec 16, 2015

frankxw commented on Dec 16, 2015

blakeembrey commented on Dec 16, 2015

frankxw commented on Dec 16, 2015

blakeembrey commented on Dec 16, 2015

frankxw commented on Dec 16, 2015

frankxw commented on Dec 16, 2015

blakeembrey commented on Dec 16, 2015

frankxw commented on Jan 6, 2016

dougwilson commented on Feb 15, 2016

frankxw commented on Feb 17, 2016

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions