Update to Express 4

[Josebaseba]

IMPORTANT UPDATE!

Sails v1, will come with the latest version of Express4 even if this PR wasn't merged. Thanks to @sgress454 and @mikermcneil for all the hard work, and to all the people interested in this PR.

Joseba

---

Hi guys!

As you know Sails works with Express3 which is deprecated since August. I think that we've to move to v4 as soon as posible, so I've tried to help in this huge work.

I think that I have the 99% of the job done, but I need help doing more intensive testing to make sure that all works fine.

Check the migration guide

The changes that I've been forced to made:

HOOKS

Blueprints

• Express 4 deprecated the req.param method in favor of req.params['param'] and req.query

Controllers

• Indentation changes

Cors

• A small fix to make sure that routeCorsConfig is always an object

CRSF

• Removed connect dependency for CSURF, the official crsf express package

HTTP

• That's one of the hooks with biggest changes, the router now works different, removing app.use(app.router); that.
• Unbind works a little bit different, now is a little bit more complex but it's not a big deal.
• The 404 and 500 errors are added to the router in a different place, we are force to it because of the new router. I add those two middlewares at the absolutely end. When sails triggers the ready event.
• Now the session works with express-session package
• The default middleware order changed removing 404 and 500 from here and adding csrf (in case that the csrf hook is activated)
• Compression works with express compression package
• FAVICON now is served with serve-favicon, and we are forced to add a default favicon in hooks/http/assets/favicon.ico, that's because the serve-favicon is forced to had a favicon, if it couldn't find one the app crashes.

Session

• Again, removed connect session in favor of express-session.
• Added a couple of defaults attributes

User Hooks

• Indentation stuff

Views

• That was hard, express4 doesn't work with layouts, so I've to add a layout support with a couple of functions inside the res.view method.

That in the hooks, now:

ROUTER

• The same happens here, but the changes are really minimum.
• In the default handlers we use the new sintaxis to send responses .status().send()
• New way to unbind routes
• cookie-parser & express-session packages added

Package.json

• Added the latest express: 4.13.3
• cookie-parser
• csurf
• express-session

TEST
I've to change some test to make them work with the new Express

• The router test is updated, because of the router changes. Now to check if the routes are binded or unbinded we have to look inside the .stack object.
• In the sample app controller the way to check route.key and route.method is updated to route.stack[0].method and route.stack[0].keys. I realized that the keys now works differently and we don't be able to get them always, I'm not sure why express does that

CORS and CRSF

• In the CORS test, in the lines 64, 83 and 111 I can't get a 200 response code and I can't find why. That 3 tests are the only ones that don't work. I need some help there! And I can't get the HTTP METHODS complete list at 92 line. Those lines are commented right now.
• Also, I've changed the sampleApp/views/viewTest/crsf.ejs file because it was wrong, we need to check if _crsf is null not if it is undefined, so I changed to the method: _.isNull(). Now it works like it should.
• In the initializeHooks file, I removed supertest in favor of sails.request method, but the tests logic still as always and the same thing happen with the router.bind.test.js file.
• In the router test file, I've added the .stack method
• Unbind file with indentation changes

More or less that's all, I know that it's hard to make that kind of pull request work. But I would like to have some feedback about what do you guys have in mind for the express4 update.

Thanks!!

[listepo]

+1000 👍

[Josebaseba]

I'm happy to help! The first thing we should fix is:

In the CORS test, in the lines 64, 83 and 111 I can't get a 200 response code and I can't find why. That 3 tests are the only ones that don't work. I need some help there! And I can't get the HTTP METHODS complete list at 92 line. Those lines are commented right now.

If we fix those three test, we'll have everything working.

Pd: I had to close and open the pull request to force Travis to run the test again

[sgress454]

@Josebaseba Really appreciate your contributions to Sails and your dedication to the continued health of the project! I also want to apologize for not getting involved sooner when you posted your original issue about migrating to Express 4. Suffice it to say, we've been having a lot of discussion about this topic internally--specifically around how much Express should continue to be integrated into Sails core.

Something that's been on the roadmap for some time (admittedly, without much progress) is a plan for splitting much of the functionality currently provided by Express into separate modules. In some ways, this PR is a step in that direction, since Express 4 already jettisoned most of the functionality into different packages. But by simply upgrading all of our existing code to use those packages, we're still tying core to Express. The real goal should be to pull that functionality--things like the views, session, and HTTP hooks--into completely separate modules. If the first generation of those standalone hooks are Express 4-based, that's just fine with us.

What might be the best plan forward would be to take the work that's been done here, and use it to start fulfilling those items on the roadmap. A great place to start would be a standalone views hook--something that can be installed as a replacement for the current core hook, like sails-hook-sockets does for the former core "sockets" hook.

[listepo]

@sgress454, so it will be with this PR?

[Josebaseba]

Hi @sgress454,

I understand the goal of having all the hooks in separated npm modules, I think that it's really a good plan. But looking at the code, at least from my point of view, right now it's not close of being a reality.

And about being tied to express, what other options do you have in mind? Hapi, Koa, Connect... Your goal is keeping the http server into hooks.http.app and make it work with the rest of hooks in a agnostic way? That would be great but doing that all the hooks should change in a radical way, and the maintaining work would become really hard because of all the options and alternatives. Am I wrong?

More things, your intention is keeping Express3 and just make sure that there are not security issues? Don't you think that that can bring compatibility problems with new nodejs packages that only could work with express4? That could be a reason for making people not choosing Sails as a option for their projects, or if the new people that comes to Sails see the express3 dependency could think that the project is out of date...

I understand the fact that we don't need being updating all the packages because sometimes is not necessary, and only could bring problems. But I'm not sure if that's a good idea with the Express package.

Right now I see two options then, not updating to v4 and keep working in separating the hooks to external hooks trying to make the agnostic to everything and then choosing the http server. Or, try to update now to express4 and then keep working to arrive to that modular heaven.

I would like to have a little bit more information, and I'll be happy to help. At the end my business core is running Sails, so, we are in the same boat!

Pd: Sorry for my English! 👎

[sgress454]

@Josebaseba your English is great, no worries. Bit of a crunch here today, but will get back to you on this ASAP.

To put it succinctly: Our main concern compatibility-wise with Sails is that it works with the latest stable version of Node (which...sigh). As long as the version of Express we're using meets that standard, we're not that inclined to upgrade.

I understand the hesitation around splitting things into separate modules that need to be maintained--our experience with maintaining the core generator modules has been mixed, at best. On the other hand, moving the sockets hook into sails-hook-sockets has been a real success, allowing for improvements and updates that don't require releasing a new version of Sails core. I expect similar benefits from moving the other core hooks into separate modules.

More things, your intention is keeping Express3 and just make sure that there are not security issues?

Our eventual intention is to dump the core dependency on Express entirely. Ideally, the only part of Express that Sails core should be using is the router, to implement _privateRouter. If there were a standalone version of the Express 3 router, we would gladly use that. Unfortunately there isn't, and Express 4's router has significant changes that you saw firsthand in your refactor--this is one of the main reasons we stalled on upgrading in the first place. As far as security / compatibility issues, if all we're using from Express is the router, it's something we should be able handle--most of those issues come up in the middleware.

What does Sails look like without Express? To the app developer, nothing would change, since there would be default modules installed for all of the former hooks (like sails-hook-sockets is for the former core sockets hook). But under the hood, Sails would just be providing an interface for those hooks, instead of being responsible for their implementation. For example, the interface for an HTTP hook would require it to handle the router:bind and router:unbind events from Sails, emit the hook:http:listening event when the server starts, and to allow the end-user to specify the middleware they wanted using sails.config.http.middleware. But sails-hook-http-koa would use the Koa router and middleware instead of Express*, and Sails wouldn't know or care. The core HTTP hook is already pretty close to being able to be removed, except for its entanglement with views. That's why pulling out the views hook is a good place for us to start.

_{*this is by no means an easy thing to do, since Koa middleware are completely different from Express, but it would be pretty great for us to be engine-independent--and even if we only ever supported Express as an HTTP server, splitting it into a separate module would let us handle updates without having to patch core.}

[Josebaseba]

Thanks @sgress454 you've left me everything pretty clear. I just have a couple of questions more:

• Why keep Express3 inside the private router if we can replace it for pillarjs router? I know that there are some changes to do (the biggest ones in unbind and reset methods) if we add the package but if I made it work you won't have any problem to make it too.
• Are you going to manage all these changes internally? (Treeline / Balderdash team) Or the community could help there?

Thanks!

[listepo]

@Josebaseba 👍

[sgress454]

Why keep Express3 inside the private router if we can replace it for pillarjs router?

It's really just been a case of, "if it ain't broke, don't fix it". Especially since we were still tied to Express for other things. We're not opposed to changing the private router, ever, it just needs to be done super carefully.

Are you going to manage all these changes internally?

I frankly doubt it will happen at all without community help. The basis of a standalone HTTP hook are here in this PR already. The sticky wicket is the reliance of the views hook on Express's res.render functionality...

[feitian124]

👍

[Josebaseba]

Ups @tjwebb , sorry for my late answer! I've been working hard these last weeks.

I didn't develop nothing new, I didn't want to work in a bad direction so I was waiting for more clues or some help to keep improving this pull request.

However few weeks ago I tried to separate the Views hook but I couldn't make it, the only way that I found was creating an own package for the res.view() method that express give us, but it's a little bit difficult. So much work that maybe goes nowhere.

I'll be glad to help if you need it, but for that I'll need the rules or steps that you think that will be the best ones to afford this challenge.

[Josebaseba]

Great, I'll do my best.

[Josebaseba]

Hi @tjwebb, I've been working these days in this pull request, specifically in:

• The pullRequest is up to date
• Sessions are working now with latest version of connect-redis and connect-mongo (probably this new version will work with almost all the connect-* session adapters)
• Some fixes in .ejs multiple layout support
• A facade for the res.param('sth') method

To make a more detailed tests I run my own project with my express4 sails brach, the project it's a big one, +50 Controllers +40 Models, +200 routes etc, and all the tests are passing. It's working with Node 0.10, 0.12 and 4.2, I couldn't find any kind of problem. Waterline is working well too, with sails-mongo adapters v0.10.x and v0.11.x.

This project it's not using the sockets hook, so I run the sockets-hook tests against my sails brach and everything works fine too.

The only 3 tests that I couldn't be able to make them pass are:

• CORS returning a 404 instead of 200
• CORS 404 + Get the list of all the http methods
• CORS 404 instead of 200 again

Obviously this pull request needs more test against real projects as I did with mine. But I think that we are really close to make it work 💪

[feitian124]

@Josebaseba thank you for your hard work, it is nice to use express 4 in sails.

[luke3butler]

👍

[luislobo]

@Josebaseba Man, you are awesome. +1

[Josebaseba]

I realized that the CORS hook it's working perfectly ok, but the default Express handler has change and that's the main reason that make the tests fail.

Those three or four assertions that are failing are expecting the old default handler response - a 200 status code + 'CHECKOUT,CONNECT,COPY,DELETE,GET,HEAD,LOCK,M-SEARCH,MERGE,MKACTIVITY,MKCOL,MOVE,NOTIFY,PATCH,POST,PROPFIND,PROPPATCH,PURGE,PUT,REPORT,SEARCH,SUBSCRIBE,TRACE,UNLOCK,UNSUBSCRIBE' response body. The rest of asserts, such as, 'access-control-allow-origin' 'access-control-allow-methods' 'access-control-allow-headers' etc... Are working perfectly.

I think that we should change a little those breaking assertions to adapt them to the Express4 logic and then start testing this fork against real world sails app, as I did with mine (and all went OK), to check if everything works as it should.

cc/ @tjwebb

[niallobrien]

Would like to see some more movement on this too. :)

[moisesrodriguez]

@Josebaseba are these the assertions #3322 you are talking about? seems like someone already made a PR for that fix

[Josebaseba]

@moisesrodriguez yes and no, yes that's one of the assertion that changes with express4 and node4 but that pull request doesn't fix the express4 new behavior. The default handler that returns a 200 with all those http methods is not working on that way anymore. Or that's what I understand at least...

@niallobrien any help is welcome! :)

[Josebaseba]

Hi @mikermcneil & @sgress454, reading this comment #3460 (comment) I think that we can close this pull request. I'll keep the forked repo in my code just in case, could be interesting having it as a reference at some points.

I understand your next steps with Sails, moving to Express5 and making everything more modular, it'll be a hard work. But why not make this version update and then work in that way? And another question more, isn't it the Express5 development stopped?

Now here it comes my opinion about the last "events":

I'm really happy with Sails, it's the core of my business and after two years I still enjoying it, I never regret my choice. But thinking about the future of the framework and the community, and especially after this last month with Trails and all the posts about Sails dying, I think that you need to adapt/update the roadmap (it's being stopped for a long time) and make your move. These days you're been more active in the repos and that feels great, but maybe you need more active contributors that help you closing these roadmap features. Removing the old contributors that aren't working on it, and making some kind of a new start. As a heavy user of this framework I think that there're a lot of things to do.

I think that this project has the potential to become (for a long time) the best MVC framework. Trails is a good approach, but until the end of this year it'll be so immature, so Sails should use this time to show to the community that it still being a great option to make profesional and personal projects.

P.S: I really enjoyed making this PR, I've learn a lot about the internals and I'm pretty happy with the result. My intention was helping you guys to keep the project strong, no regrets!

P.S 2: I really DO like Trails 👍, and I'll keep an eye on it, but it's too young and it'll be for a while.

[mikermcneil]

I really enjoyed making this PR, I've learn a lot about the internals and I'm pretty happy with the result. My intention was helping you guys to keep the project strong, no regrets!

@Josebaseba I really appreciate that, as well as all of your help over the years.

First of all, I'd like to apologize for the distracting, misleading, and ofttimes disrespectful dialogue carried out on our support channels and social media in recent weeks, as well as the confusion about Sails. As I mentioned in the "Status of Sails" issue, in retrospect, it is clear our team should have publicly responded much earlier. The fact that we didn't was my fault-- I actively suggested that we all stay silent and take the higher road. Clearly that was a mistake that just made the situation worse. Lesson learned.

For posterity: Cody, Scott, Rachael, Irl, and I have no beef with anyone doing a fork or rewrite of Sails or Waterline-- it has happened before, and it will happen again (and judging from my discussions with my friends and acquaintances who are other open-source maintainers, it's a part of everyday life as a project grows).

What made this situation so unfortunate is the misleading way in which the new project was presented. That said, it seems like everyone is mostly clear on the facts now, which is great news. I believe we should be using GitHub for discussion of a technical nature; so while I'm more than willing to enlighten anyone who feels it would be helpful to know the background of the "events", I do not plan to discuss it any further in a public setting. As I've stated elsewhere, please don't hesitate to contact me directly if you have any questions. I hope this allows us all to move on and get back to work.

Aside from everything else going on, it is clear we need a better line of communication between the user base as a whole, active contributors, and the Sails core team. Part of the upcoming v0.12 release is a major rewrite of our contribution guide with the goal of refining the process, and introducing some ground rules that will make it easier to quickly respond to issues and pull requests without everyone having to feel guilty all the time. I spent most of yesterday putting that together, and I'm looking forward to finishing it early this week, along with some new documentation about the myriad other ways Sails users can contribute back to the framework, including our approach for translating the documentation, supporting or starting a local Sails meetup group, and even supporting your local Node.js meetup group. Yesterday, I also posted the first version of the Sails community's code of conduct, which I hope will help set up clear expectations for communication in GitHub and our other support channels in the future.

Sails should use this time to show to the community that it still being a great option to make profesional and personal projects.

Agreed. I feel that much of the recent discussion has distracted everyone from what actually matters-- including the great work you've begun here, much of which is relevant and important in future development in Sails core.

isn't it the Express5 development stopped?

I'm not up to date with the latest enough to really provide any additional insight there, but I hope this helps explain why I feel that it is so important for Sails' long term strategy to use the underlying, federated modules in the jshttp and pillarjs projects. I'm referring to that as "Express 5" just because that's the way it's been explained to me in the past. I'd suggest bringing any other questions about that to Doug or any of the other guys who originally took over the Express project from TJ Hollawaychuk. They are leading the charge there, doing great work, and I am 100% supportive of their efforts. If you're interested in contributing to the bare metal of the Express project long term, Doug is the guy to talk to.

In the mean time, we're committed to taking ownership over fixing any major security, performance, or stability bugs that arise in Express 3. If anyone reading this has doubts about our commitment to do so, please realize that our core team is running all of our own servers using the latest stable version of Sails in production (like always), and have every reason to make sure Sails and its dependencies are clean and well-oiled.

At the end of the day, what's best for everyone is to share as many trusted, high quality, well-tested dependencies as possible, and for those dependencies to become stable-- i.e. change as little as possible. If frameworks like Sails, Express, Kraken, Connect, etc. can work together to share as many of those building blocks as we can, it will allow each of us to focus on the individualities which make each framework distinctly useful, instead of spending time reinventing the wheel.

[Josebaseba]

@mikermcneil everything it's clear, thanks for your time!

As I told you I'm going to close this pull request but I'll keep the fork in my profile.

I'll keep around this project for a while so I'll try to keep contributing (maybe in more useful ways haha!) in the near future.

[niallobrien]

@mikermcneil Fantastic response. Love it, keep up the great work! :)

[luislobo]

For the ones coming here, Express 4 will be default in Sails 1.0

[Josebaseba]

@luislobo I'm gonna edit the first post to make It more clear

[mikermcneil]

@Josebaseba @luislobo nice catch thanks guys

[kristianmandrup]

Good job! When can we expect 1.0 release with Express 4 support?