Skip to content

Commit

Permalink
remove old OPTIONS default response
Browse files Browse the repository at this point in the history
relatively useless since its so non-informative,
let me know if anyone has an objection to this,
i think its best to define these for your API
  • Loading branch information
tj committed Aug 29, 2012
1 parent 4403f13 commit 2bba69f
Show file tree
Hide file tree
Showing 2 changed files with 0 additions and 75 deletions.
38 changes: 0 additions & 38 deletions lib/router/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,6 @@ Router.prototype._dispatch = function(req, res, next){
// match route
req.route = route = self.matchRequest(req, i);

// implied OPTIONS
if (!route && 'OPTIONS' == req.method) return self._options(req, res);

// no route
if (!route) return next(err);
debug('matched %s %s', route.method, route.path);
Expand Down Expand Up @@ -173,41 +170,6 @@ Router.prototype._dispatch = function(req, res, next){
})(0);
};

/**
* Respond to __OPTIONS__ method.
*
* @param {IncomingMessage} req
* @param {ServerResponse} res
* @api private
*/

Router.prototype._options = function(req, res){
var path = parse(req).pathname
, body = this._optionsFor(path).join(',');
res.set('Allow', body).send(body);
};

/**
* Return an array of HTTP verbs or "options" for `path`.
*
* @param {String} path
* @return {Array}
* @api private
*/

Router.prototype._optionsFor = function(path){
var self = this;
return methods.filter(function(method){
var routes = self.map[method];
if (!routes || 'options' == method) return;
for (var i = 0, len = routes.length; i < len; ++i) {
if (routes[i].match(path)) return true;
}
}).map(function(method){
return method.toUpperCase();
});
};

/**
* Attempt to match a route for `req`
* with optional starting index of `i`
Expand Down
37 changes: 0 additions & 37 deletions test/app.options.js

This file was deleted.

6 comments on commit 2bba69f

@kolektiv
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although there are obviously alternatives, this was actually quite a handy thing to have, specifically in our usage for probing in load balancers. Our HAProxy instances do an httpchk (a probe) using OPTIONS on /, which returned a 200 response (as we've generally always got at least a GET for / set up, that's pretty safe). This was a very simple and unobtrusive way to do probing without having to bake in a specific route for that in to multiple apps. It now returns a 404, which the load balancer considers to be down. I spent a while scratching my head earlier working out what had changed! :)

I'm not saying that it should go back in, just that it did have some uses! What do you think?

@tj
Copy link
Member Author

@tj tj commented on 2bba69f Sep 6, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah for this we have a GET /health end-point

@kolektiv
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah - I've just written a GET /probe end-point (I assume you mean that you've written a /health end-point - if there's actually one built in which i've missed ignore the following) - maybe it would be a nice feature to have a response baked in to express which was a standard probe-able end-point - /health or something perhaps. Although it's not much code, I've now got a duplicate route across 8 apps and counting :)

@tj
Copy link
Member Author

@tj tj commented on 2bba69f Sep 6, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nahh IMO it's simple so why not leave it out? don't gain much by adding that sort of thing in, other than having people skip over it and implement their own anyway haha

@kolektiv
Copy link

@kolektiv kolektiv commented on 2bba69f Sep 6, 2012 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kolektiv
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To close that loop, there's now connect-probe middleware for this. It's in npm, if anyone ever finds this and wants to know! (The middleware way will handle OPTIONS, GET, etc. without writing multiple routes, and has a customisable path, as well as returning uptime if desired). Trivial, but handy.

Please sign in to comment.