Skip to content

Commit

Permalink
Upgrade Express to v4
Browse files Browse the repository at this point in the history
This was accomplished by installing Express 4, attempting to run the tests,
observing things that broke, and then fixing them. In some cases, I leaned on
the work done in balderdashy#3235 to figure out
how to do something. The most comprehensive change is to the router, which is
its own Router object, and no longer a function on an Express `app`.

Adds two new dependencies (which were removed from Express core): cookie-parser
and compression. In some cases we removed the dependency on Express instead of
upgrading - we no longer try to serve favicons, or deal with sessions.
  • Loading branch information
Kevin Burke committed Feb 25, 2016
1 parent 96bc850 commit 39317fb
Show file tree
Hide file tree
Showing 16 changed files with 497 additions and 275 deletions.
24 changes: 22 additions & 2 deletions lib/app/request.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,26 @@ module.exports = function request( /* address, body, cb */ ) {
var address = arguments[0];
var body;
var cb;

var method;
var headers;
var url;

// Usage:
// sails.request(opts, cb)
// • opts.url
// • opts.method
// • opts.params
// • opts.headers
//
// (`opts.url` is required)
if (_.isObject(arguments[0]) && arguments[0].url) {
url = arguments[0].url;
method = arguments[0].method;
headers = arguments[0].headers || {};
body = arguments[0].params || arguments[0].data || {};
}

if (arguments[2]) {
cb = arguments[2];
body = arguments[1];
Expand All @@ -60,9 +80,9 @@ module.exports = function request( /* address, body, cb */ ) {
}

// If route has an HTTP verb (e.g. `get /foo/bar`, `put /bar/foo`, etc.) parse it out,
var method = sails.util.detectVerb(address).verb;
method = method || sails.util.detectVerb(address).verb;
method = method ? method.toUpperCase() : 'GET';
var url = sails.util.detectVerb(address).original;
url = url || sails.util.detectVerb(address).original;

// Parse query string (`req.query`)
var queryStringPos = url.indexOf('?');
Expand Down
16 changes: 5 additions & 11 deletions lib/hooks/http/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,17 @@
*/

var path = require('path');
var _ = require('lodash');

var cookieParser = require('cookie-parser');
var express = require('express');
var _ = require('lodash');


module.exports = function(sails) {


var initialize = require('./initialize')(sails);




/**
* Expose `http` hook definition
*/
Expand Down Expand Up @@ -102,13 +101,8 @@ module.exports = function(sails) {


// Cookie parser middleware to use
// (or false to disable)
//
// Defaults to `express.cookieParser`
//
// Example override:
// cookieParser: (function cookieParser (req, res, next) {})(),
cookieParser: express.cookieParser,
// XXX remove this
cookieParser: cookieParser,
}
};
},
Expand Down
24 changes: 4 additions & 20 deletions lib/hooks/http/middleware/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@
* Module dependencies
*/

var _ = require('lodash'),
util = require('util');

var util = require('util');

var compression = require('compression');
var _ = require('lodash');

module.exports = function(sails, app) {

Expand Down Expand Up @@ -55,7 +55,7 @@ module.exports = function(sails, app) {
return cookieParser && cookieParser(false);
})(),

compress: IS_PRODUCTION && express.compress(),
compress: IS_PRODUCTION && compression(),

// Use body parser, if enabled
bodyParser: (function() {
Expand Down Expand Up @@ -146,21 +146,5 @@ module.exports = function(sails, app) {
return res.send(400, bodyParserFailureErrorMsg);
},

// By default, the express router middleware is installed towards the end.
// This is so that all the built-in core Express/Connect middleware
// gets called before matching any explicit routes or implicit shadow routes.
router: app.router,

// 404 and 500 middleware should be attached at the very end
// (after `router`, and `www`)
404: function handleUnmatchedRequest(req, res, next) {

// Explicitly ignore error arg to avoid inadvertently
// turning this into an error handler
sails.emit('router:request:404', req, res);
},
500: function handleError(err, req, res, next) {
sails.emit('router:request:500', err, req, res);
}
});
};
5 changes: 2 additions & 3 deletions lib/hooks/request/validate.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,8 @@ module.exports = function (req, res) {
});

if (redirectTo) {
res.redirect(redirectTo);
}
else {
res.redirect(302, redirectTo);
} else {
throw e;
}
}
Expand Down
2 changes: 1 addition & 1 deletion lib/router/bind.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ function bindRedirect(path, redirectTo, verb, options) {

bind.apply(this,[path, function(req, res) {
sails.log.verbose('Redirecting request (`' + path + '`) to `' + redirectTo + '`...');
res.redirect(redirectTo);
res.redirect(302, redirectTo);
}, verb, options]);
}

Expand Down
11 changes: 8 additions & 3 deletions lib/router/bindDefaultHandlers.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,12 @@ module.exports = function(sails) {
// Log a message and try to use `res.send` to respond.
sails.log.error('Server Error:');
sails.log.error(err);
if (process.env.NODE_ENV === 'production') return res.send(500);
return res.send(500, err);
if (process.env.NODE_ENV === 'production') {
return res.status(500);
} else {
res.status(500);
return res.send(err);
}
}

// Serious error occurred-- unable to send response.
Expand Down Expand Up @@ -77,7 +81,8 @@ module.exports = function(sails) {
// Log a message and try to use `res.send` to respond.
try {
sails.log.verbose('A request did not match any routes, and no `res.notFound` handler is configured.');
res.send(404);
res.status(404);
res.send('Not found');
return;
}

Expand Down
33 changes: 21 additions & 12 deletions lib/router/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ function Router(options) {
// TODO:
// instead, use: https://www.npmjs.org/package/path-to-regexp
// (or: https://www.npmjs.org/package/path-match)
this._privateRouter = express();
this._privateRouter = express.Router();

// Bind the context of all instance methods
this.load = _.bind(this.load, this);
Expand Down Expand Up @@ -164,13 +164,13 @@ Router.prototype.route = function(req, res) {
return res.send(400, err && err.stack);
}

bodyParser(req,res, function (err) {
bodyParser(req, res, function (err) {
if (err) {
return res.send(400, err && err.stack);
}

// Use our private router to route the request
_privateRouter.router(req, res, function handleUnmatchedNext(err) {
_privateRouter(req, res, function handleUnmatchedNext(err) {
//
// In the event of an unmatched `next()`, `next('foo')`,
// or `next('foo', errorCode)`...
Expand Down Expand Up @@ -226,12 +226,16 @@ Router.prototype.unbind = function(route) {

// Remove route in internal router
var newRoutes = [];
_.each(this._privateRouter.routes[route.method], function(expressRoute) {
if (expressRoute.path != route.path) {
// Logic taken from https://github.com/balderdashy/sails/pull/3235/files
_.each(this._privateRouter.stack, function(expressRoute) {
if (!_.isObject(expressRoute.route)) {
return newRoutes.push(expressRoute);
}
if (expressRoute.route.path !== route.path || !expressRoute.route.methods[route.verb]) {
newRoutes.push(expressRoute);
}
});
this._privateRouter.routes[route.method] = newRoutes;
this._privateRouter.stack = newRoutes;

};

Expand All @@ -249,15 +253,20 @@ Router.prototype.reset = function() {
var sails = this.sails;

// Unbind everything
_.each(this._privateRouter.routes, function(routes, httpMethod) {

// Unbind each route for the specified HTTP verb
var routesToUnbind = this._privateRouter.routes[httpMethod] || [];
_.each(routesToUnbind, this.unbind, this);
_.each(this._privateRouter.stack, function(stack) {
if (!_.isObject(stack.route)) {
return;
}
var route = {
path: stack.route.path,
target: stack.handle,
};
route.verb = Object.keys(stack.route.methods)[0];
return this.unbind(route);

}, this);


this._privateRouter.stack = [];
// Emit reset event to allow attached servers to
// unbind all of their routes as well
sails.emit('router:reset');
Expand Down
Loading

0 comments on commit 39317fb

Please sign in to comment.