Is the middlewares ordering correct?

[msathis]

As per this line
https://github.com/webpack/webpack-dev-server/blob/master/lib/Server.js#L320

the middlewares ordering is ["setup", "headers", "middleware"]. Due to this if i have a middleware for * in setup, it will consume all wepack output routes as well. For e.x

setup: function(app) { app.use('*', function(req, res) { res.render('index'); }) }

this configuration will server index html even for all webpack bundle paths.But if the middleware ordering is [ "headers", "middleware", "setup"], this problem won't occur.

Wepack: 2.2.1
Webpack-dev-server: 2.3.0

[shellscape]

I think the argument is valid, and we should probably provide more options for middleware overrides. I could see pre and post middleware hooks being useful and not bloating.

We'd happily review a Pull Request to extend the functionality there.

[mrtnbroder]

@shellscape if we agree on 'pre' and 'post', I can submit a PR, however, I DO think the current order is just not correct. I'd expect the webpackMiddleware to be invoked first, so exactly like @msathis suggests it.

I've stumbled upon an issue with how it is setup currently, so I'd rather like to submit a PR for the correct ordering, instead of adding additional hooks.

My current workaround is, using the undocumented options.feature hook, to get the correct order. However, this is not really stable since I do not know in the future which 'features' will be added to this property.

[shellscape]

@mrtnbroder this is where Git blame comes in handy; Add setup option to prepend custom middlewares #279. In the context/angle you're viewing this from, the middleware ordering doesn't look correct because of the behavior you're seeing and the goal you're trying to achieve. However, there was a good reason that setup was added before middleware in this case. It's always important to try and find the "why"
when something looks off or peculiar.

So I'd argue that we don't want to move the position of setup in the order. Rather, pre and post can be positioned around middleware without constituting a breaking change or interfering with the original intent of setup.

[mrtnbroder]

I don't see a usecase for pre yet as you can simply use setup for that, however, I think we introduce a hook within the middleware though that looks like this:

    middleware: () => {
      // include our middleware to ensure it is able to handle '/index.html' request after redirect
      app.use(this.middleware);
      if (typeof options.middleware === 'function') { options.middleware(app, this); } // or options.postMiddleware
    },

[shellscape]

I'm not sure I agree with your proposed approach. I see pre replacing setup ("setup" is rather ambiguous anyhow) and setup being deprecated. Here's what I would prefer at the moment:

const server = new Server(compiler, {
  middleware: {
    pre: () => { },
    post: () => { }
  }
});

and while I'm thinking on it, a better pattern here that everyone is probably already familiar with if they've done any kind of test writing:

const server = new Server(compiler, {
  middleware: {
    before: () => { }, // applied before the built-in middleware
    after: () => { } // applied after the built-in middleware
  }
});

before would be synonymous with setup and setup marked deprecated. after would have to be pushed to defaultFeatures before the call to createServer.

[mrtnbroder]

But pre and post is something already introduced in the codebase with e.g. a webpack Rule, https://webpack.js.org/configuration/module/#rule-enforce

I'd try to stick to this pattern. Agreed?

[shellscape]

Perhaps in the context of a webpack Rule loader it makes more sense. But in terms of middleware, I don't believe it's a better choice than before and after.