Update to use Express 4.

[shortstuffsushi]

Decided that rather than just offer, I'd actually do it. These are the required changes if you want to use Express 4. Primarily, all the Middleware has been broken into separate modules, which I have included and referenced from the app.js.

[simov]

Thanks, @shortstuffsushi this is very helpful and most probably will go in the next release. I just need to add a few more system tests, and a couple of features, so this PR will stay open for a while 🍻

[shortstuffsushi]

Totally fine, I assumed since the tests passed it was ok, but it's hard for me to know if everything is completely correct. Let me know if there is anything else that needs fixing.

[simov]

These are the unit tests, the system tests are here

So they fail, but it's not a problem, I'm pretty sure it's something small, you don't have to waste time trying to run them.

So it's up to me, because I want to add more tests, before migrating to Express4, that's all.

[simov]

Hey, @shortstuffsushi

First of all migrating from express3 to express4 is not that easy especially when your code relies on the bodyParsers's ability to parse multipart encoded bodies (like for example express-admin's editview which supports file uploads),

But that's not the problem, my main concern was about the embedding, and I can't seems to get to work with cross express/admin configuration. I'm using something like this for the embed app

// express 3.4.4
var app = express()
    .use(express.bodyParser())
    .use(express.cookieParser())
    .use(express.session({key: 'embed-app', secret: 'secret'}))
    .use(express.csrf())
    .use(express.methodOverride())
    .use(express.static(__dirname));

app.get('/', function (req, res) {
    res.send('Hello World');
});

app.use('/admin', admin);


// express 4.4.4
var app = express()
    .use(bodyParser.json())
    .use(bodyParser.urlencoded({extended: true}))
    .use(multer())
    .use(cookieParser())
    .use(session({name: 'embed-app', secret: 'secret'}))
    .use(csrf())
    .use(methodOverride())
    .use(serveStatic(__dirname))

app.get('/', function (req, res) {
    res.send('Hello World');
});
app.use('/admin', admin);

As you can see I added just a bunch of common middlewares inside the embedd app, in fact that's exactly the same ones used in express-admin. So when I'm running express3 embed app on express3 admin or express4 embed app on express4 admin, everything is fine as it should be.

The problem is when I try to use express4 embed app on express3 admin or express3 embed app on express4 admin. So I'm wondering what your express app config exactly is.

[simov]

I think I got it, the admin mount point should be first inside the embed app

var app = express()
    .use('/admin', admin)
    .use(bodyParser.json())
    // ...

[shortstuffsushi]

I guess I didn't realize you were trying to do multipart uploads, I didn't look through all the code, just the express configuration you had. Were you just relying on built in multipart support previously? If you want, I can add another middleware library for that -- connect-multiparty is the one I've been using I believe.

As far as your second issue, I'm not 100% sure what you're referring to with 'embed app.' Do you mean when you run express-admin inside of another express based app? In that case, that's how I'm running my app, but I have express-admin added after all my other app configuration is completed. That said, I haven't tried since updating it to Express 4, so I'll take a look at that today. Other than that, it looks like you followed the same pattern I did with your middleware -- if you look at my commits, you'll see I did the same (any reason you didn't just use that?). That part seemed easy enough, though it does make the dependency list much longer. Perhaps a module that configures those standard items for you would be a good addition to NPM, if one doesn't already exist.

[simov]

Yes express-admin depends on the bodyParser found in express3 (which handles multipart encoding). I have your PR pulled in another branch locally and I'm working on top of that, don't worry.

I got almost everything working already, I'm using a combination between multer and qs directly, because that gives me the closest result to what the original bodyParser was providing.

My latest post was about the case when you embed the express-admin app into existing express app.
So in case your app use different version of express that the admin - the admin mount point should be placed on top, because if your app happen to use similar middlewares with the ones used in the admin - they'll clash.

[shortstuffsushi]

Any reason you need to use those two middleware libs instead of just a single item like connect-multiparty? I'm pretty sure, from what I've been reading through, that the default bodyParser in 3.4 was actually that. Any way, up to you obviously.

Regarding admin beside another app, that is the same set up I'm running locally, but I'm running 4 for both, so I don't get any conflicts of course. I'm not sure what the solution for what you're describing is -- the express docs around the use method mention that it's order dependent, but does that imply that putting the express-admin stuff first is correct? Won't the app it's embedded in then be dirtied by whatever express admin is using?

Actually, it also mentions the concept of 'mounting' to a specific path. Since express-admin is always located off of some root path, couldn't it just prefix all of its uses with its context root?

[shortstuffsushi]

Actually, I just realized that in our application we are already doing just that --

var expressAdmin = require('express-admin'),
      root = '/admin';

...

var admin = expressAdmin.initServer(expressAdminArgs);
app.use(root, admin);

This might again only work because I have the same all dependencies. Technically all the things that have been defined on that app will affect the adminApp returned from initServer. So I think you're right, apps would probably need to use express-admin before they dirty up the app with their own middleware. Not sure if there is a better solution.

[simov]

Wrong wording from my side. When mounting the admin on top you ensure that the admin mount point will use its own middlewares to handle the request. Otherwise the request should pass through your app's middlewares first, and in case there some differences in their usage it might not get to the admin mount point at all (like for example with the csrf middleware).

And the reason I'm interested in this scenario is because I don't want to cut off of new features the users that are using express3 in their apps and don't want/can't update to express4.

The middlewares are not overridden and it makes total sense, since you are mounting an actual app. You can easily test this: mount the admin on top and remove your app's body parser middleware, the result is that you don't have a body parser in your app, even though the admin have one on its own.

I'll take a look at connect-multiparty

[simov]

connect-multiparty was the right module, keep in mind that the latest code in master isn't published to npm yet 🍻

[shortstuffsushi]

I think we were agreeing on the ordering thing, seems like the only solution. Would probably be good to note it in your readme or something, but otherwise, good to go. Thanks for letting this in! 👍