Make middleware usage independent from load order about untool

[dmbch]

@robin-drexler I do see your point wrt logging/error handling - that is why I am proposing an ‘override’able ‘finalizeServer’ hook.

Other than that, I would like to push the issue downstream, i.e. where requirements are less cloudy and can be addressed in a more specific manner.

With regards to a custom middlewares, we could just build them in a way that makes them run only once..:

(req, res, next) => {
  if ('user' in res.locals) return next();
  // do the usual auth dance
}

As you already said, this approach will not help us with external middlewares, but I am sure we can come up with something...

[dmbch]

I think I see your point, but (as mentioned IRL), I am afraid I am not quite convinced we need to change as much in @untool/express as you propose. Why can we not just add one or two hooks (e.g. optimizeServer) and rely on express' middleware chaining, e.g.:

const cookieParser = require('cookie-parser');
const bodyParser = require('body-parser');

module.exports = {
  initializeServer(app) {
    app.use(cookieParser(), bodyParser(), (req, res, next) => {
      console.log(req);
      next();
    });
    return app;
  }
}

That would, to my mind, mean we have two sorting dimensions (preset/mixin order and hook calling order) plus manual middleware sorting. Combined with a nice organisation of mixins/middlewares into packages we ought to be able to tackle all middleware registration requirements I personally can think of... We should probably make sure we expose all mixins/middlewares consumers might want to reshuffle in our downstream (ahem, hops) packages.

Of course, we cannot enable mixins to get rid of middlewares registered by other mixins, but I am not even sure we should support that - Express.js itself does not as far as I can tell.

@jhiode are there any specific use-cases that we really cannot support with what we have and cannot easily add..? Please excuse me if I am being thick, here. 😉

[robin-drexler]

@dmbch +1 for keeping it as simple as possible and not adding another sorting dimension if we can avoid it.

The app.use approach looks fine, although I can imagine issues arising once the same middleware is used by multiple mixins.

This might not be much of a problem for middlewares such as cookie-parser where the only downside is a tiny performance penalty. However, there are also middlewares that take care of logging or authentication, where multiple execution could lead to unwanted results.

That being said, I'm not entirely sure how often that will actually occur with those middlewares, but I still wanted to voice that concern.

[dmbch]

Having thought about this a bit more - and having been pointed to what others are doing by @ZauberNerd, what do you think of a single hook working essentially like this:

const configureExpress = (app, middlewares) => {
  app.use((req, res, next) => next());

  middlewares.intial.push((req, res, next) => next());

  middlewares.middlewareOrder.unshift('foo');
  middlewares.foo = [(req, res, next) => next()];

  return app;
};

We could start by having the same phases Strongloop has and add other phases on the fly. So, the middlewares object would look something like this:

{
  initial: [],
  session: [],
  auth: [],
  parse: [],
  files: [], // contains static and webpack dev middlewares
  routes: [], // contains the transpiled React middleware
  final: []
}

The phases array would of course look like this:

['initial', 'session', 'auth', 'parse', 'files', 'routes', 'final']

I am convinced we would go further down this nice rabbit hole if we try moving things like middleware order into config, so I would really like to keep this things runtime-y. What do you think?

[ZauberNerd]

I don't think we need that many phases right now because untool itself doesn't use that many.
I'd propose something like:

• initial (for bootstrapping things)
• parse (for cookie/body parser)
• routes? (do we have "routes" as such? or is it all handled in the universal react middleware)
• files (for static file serving)
• universal (for our react render middleware)
• final (for error handling and such)

Also: The code snippet above doesn't make sense with what you wrote - could you take another look and edit it please? (middlewares.initial should be an array right? So you can't just assign it to a single middleware? Also middlewareOrder is nowhere declared)

Also: How about handling post* / pre* like npm does? instead of shifting/unshifting an array?

[dmbch]

@ZauberNerd PTAL

[dmbch]

@ZauberNerd I think we might want to have routes for things like (mock) API endpoints, but I would be fine to remove it. After all, we can easily add it downstream. But why keep initial, parse and final around, then? Keep 'em all or drop 'em all, I guess. :)