Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

[WIP] Add app.subroute option for method chaining middleware for routes, as well as automatic OPTIONS responses and 405 Method Not Allowed error codes #1511

Closed
wants to merge 1 commit into from

5 participants

@jonathanong
app.subroute('/foo')
  .use(allMiddleware)
  .get(getMiddleware)
  .post(postMiddleware)
  .all(allMiddleware2)
  .end()

Will be equivalent to:

app.all('/foo', allMiddleware)
app.get('/foo', getMiddleware)
app.post('/foo', postMiddleware)
app.all('/foo', allMiddleware2)
app.options('/foo', function(req, res, next){
  res.setHeader('Allow', 'GET,HEAD,POST,OPTIONS')
  res.end()
})
app.all('/foo', function(req, res, next){
  var err = new Error()
  err.status = 405
  next(err)
})

.end() adds the app.options() and the last app.all() middleware and is completely optional.
There is also a app.subroute().allow() and disallow() method to add/remove methods from app.subroute().options().

This kind of implements @guille's #1501 alternative route syntax.
This also alleviates #1499 405 Method Not Allowed routing.

There are, however, issues with this code. If you're interested in merging this, I can work on it more, otherwise I'm not.

  • If a response is sent via app.subroute().all, that method will not be added to OPTIONS since express doesn't know what method was used.
  • I was under the impression that app.get accepted HEAD requests but doesn't send a body as per HTTP spec, but I guess that isn't the case in express (I have a test commented out). Not sure how to handle it.
  • All routes have to be declared in the same app.subroute() for OPTIONS to work.

Also routes for app.subroute(routes) can be an array, though I'm not sure if you'd want that.
I added that because in my case, I have a lot of similar routes.

Then there's less important issues like I don't know where to put stuff, and coding styles.

Let me know what you think.

@adambrod

This looks really interesting. I have found myself wanting to group sets of subroutes together and I think this would help with that. Lemme play around with it and see how it works.

@adambrod

I really like this syntax. It feels like it definitely cleans things up and simplifies routing. One question I have: if I want to enable CORS, would I need to access subroute.methods? I'm thinking something like adding middleware after the subroute definition...

fooRoute = app.subroute('/foo')
  .use(allMiddleware)
  .get(getMiddleware)
  .post(postMiddleware)
  .all(allMiddleware2)
  .use(corsMiddleware)
  .end()
fooRoute.use( function(req, res, next) {
    res.header('Access-Control-Allow-Origin', '*');
    res.header('Access-Control-Allow-Methods', fooRoute.methods);
    res.header('Access-Control-Allow-Headers', 'Content-Type, Authorization, Content-Length, X-Requested-With');
    return next();
  };
);

Or is there a more elegant way to do this? I feel like there is probably a better way to do it. I just need the ability to get fooRoute.methods after the methods are defined.

@tj
tj commented

you can already do affectively the same thing as-is without additional api. I had a similar chainable issue open years ago but I cant find it now :( there was some discussion there as well. If anything more reflection apis would be nicer I think. We used to have a default OPTIONS behaviour but it's not really all that useful, tough call, I'll see if I can dig up that old issue

@tj
tj commented

found it: #812

this would have to be a 4x change in case people are chaining methods currently which just return the app again

@jonathanong

I guess for CORS, it would be something like options. and you should be doing it before .end().

app.subroute() is just syntactical sugar on top of app[VERB] so it shouldn't break backwards compatibility, so people can use app[VERB] normally

and no way to hierarchical routes!

@tj
tj commented

well all of the CORS header fields are really app-specific, no point adding that stuff to express at all

@adambrod

Yeah, I agree that Express shouldn't do the CORS stuff automatically. It just feels like CORS is the type of thing you'd want to attach to a subroute (e.g., allow CORS on /content routes but not /user routes). It's nice if you have access to route.methods so you know which verbs are allowed.

@jonathanong

shouldn't be hard. i have an object of methods used at app.subroute().methods = {}. you can just lazy load those methods in a CORS middleware, but it should be one of the first middleware.

@shesek

I'm playing around with an alternative implementation, with a subroute() function that calls another function with the context of a new app that's bound to a base/default path. Might be easier to just see the code: https://gist.github.com/shesek/5122225 (written in CoffeeScript, compiled source here)

@jonathanong
  • i can't read coffeescript and i dont think tj can either
  • it looks hierarchical, which tj said no to
  • app.configure() is deprecated
  • each instance of an app creates significant overhead. express shouldn't be adding this overhead itself, or should at least be using a Router() instance.
@shesek
  • i can't read coffeescript and i dont think tj can either

Sorry about that, its just much quicker to hack on coffeescript :) I can write a JavaScript version if the compiled source isn't readable enough.

  • it looks hierarchical, which tj said no to

I didn't know that. Why was that decided?

  • app.configure() is deprecated

yeah, I know, but its a quick way to execute a function in the context of the app. I usually use an helper function for that (using express(), -> ...), but I didn't want to clutter the example with that.

  • each instance of an app creates significant overhead. express shouldn't be adding this overhead itself, or should at least be using a Router() instance.

I'm just creating new objects with their [[Prototype]] set to the app, and override some methods. It shouldn't take significant overhead to do that.

It should probably be noted that I'm not saying its better than the original proposal... I just stumbled on this ticket and wanted to share the way I ended up implementing subroutes. Also, since function definitions and accessing this properties is more verbose with JavaScript, the original API is much more concise than my proposal with non-CoffeeScript code.

Edit: An interesting use-case for my variation is that it lets you call other modules with an app bound to some base path, so that all the routes added from that module are relative (or defaults) to the base path.

// main
app.subroute('/user', require('./user'))

// user.js
module.exports = function (app) {
  app.get(function(){ }); // GET /user
  app.post(function(){ }); // POST /user
  app.get('/signup', function(){ }); // GET /user/signup
}

Currently you can do something similar by mounting sub-apps, but they might not be appropriate in some cases.

@jonathanong

#812 which was mentioned above. you didn't state your intentions (alternative, in express or as a plugin, etc.) so i just stated my thoughts.

@shesek

@jonathanong you meant #812 w.r.t hierarchical structure? I saw it, but I don't see anything tj said against that.

@shesek

If anyone is interested, I modified my script to support OPTIONS and 405 Method Not Allowed, and moved it to https://github.com/shesek/express-subroute.

@shesek

I was under the impression that app.get accepted HEAD requests but doesn't send a body as per HTTP spec, but I guess that isn't the case in express (I have a test commented out). Not sure how to handle it.

I was just browsing the Sinatra's code and noticed that "Defining a GET handler also automatically defines a HEAD handler" [1]. I think is makes sense for express to do this too, doesn't it? Anyone knows how other frameworks are handling this?

[1] https://github.com/sinatra/sinatra/blob/v1.3.5/lib/sinatra/base.rb#L1245

@tj
tj commented

@shesek we already delegate HEAD to GET if there's no HEAD route defined

@rauchg

-1

@jonathanong

-1 for me too now. Thinking of a layer on top of express instead now.

@jonathanong jonathanong closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 18, 2013
  1. @jonathanong

    initial subroute commit

    jonathanong authored
    well the tests work
This page is out of date. Refresh to see the latest.
Showing with 187 additions and 2 deletions.
  1. +58 −0 lib/application.js
  2. +41 −2 lib/utils.js
  3. +88 −0 test/app.subroute.js
View
58 lib/application.js
@@ -8,6 +8,8 @@ var connect = require('connect')
, middleware = require('./middleware')
, debug = require('debug')('express:application')
, locals = require('./utils').locals
+ , sendOptions = require('./utils').sendOptions
+ , methodNotAllowed = require('./utils').methodNotAllowed
, View = require('./view')
, utils = connect.utils
, path = require('path')
@@ -436,6 +438,62 @@ app.all = function(path){
app.del = app.delete;
+app.subroute = function (routes) {
+ return new Subrouter(this, routes)
+}
+
+function Subrouter(app, routes) {
+ this.app = app;
+ this.routes = Array.isArray(routes) ? routes : [routes];
+
+ this.methods = {};
+}
+
+methods.concat('all').forEach(function (method) {
+ Subrouter.prototype[method] = function () {
+ var app = this.app;
+ var args = [].slice.call(arguments);
+
+ this.routes.forEach(function (route) {
+ app[method].apply(app, [route].concat(args));
+ })
+
+ if (method !== 'all') this.methods[method] = true;
+ if (method === 'get') this.methods.head = true;
+
+ return this
+ }
+})
+
+Subrouter.prototype.use = Subrouter.prototype.all
+Subrouter.prototype.del = Subrouter.prototype.delete
+
+// Must be set last
+Subrouter.prototype.end = function () {
+ if (!this.methods.options) {
+ this.methods.options = true;
+ this.options(sendOptions(this.methods));
+ }
+
+ this.use(methodNotAllowed);
+
+ return this
+}
+
+// Must be set immediately before `end()`
+Subrouter.prototype.allow = function (method) {
+ this.methods[method.toLowerCase()] = true;
+
+ return this
+}
+
+// Must be set immediately before `end()`
+Subrouter.prototype.disallow = function (method) {
+ delete this.methods[method.toLowerCase()];
+
+ return this
+}
+
/**
* Render the given view `name` name with `options`
* and a callback accepting an error and the
View
43 lib/utils.js
@@ -20,8 +20,8 @@ exports.etag = function(body){
/**
* Make `locals()` bound to the given `obj`.
- *
- * This is used for `app.locals` and `res.locals`.
+ *
+ * This is used for `app.locals` and `res.locals`.
*
* @param {Object} obj
* @return {Function}
@@ -279,4 +279,43 @@ exports.pathRegexp = function(path, keys, sensitive, strict) {
.replace(/([\/.])/g, '\\$1')
.replace(/\*/g, '(.*)');
return new RegExp('^' + path + '$', sensitive ? '' : 'i');
+}
+
+/**
+ * Create a middleware function to send
+ * an Allow header for an OPTIONS request.
+ *
+ * @param {String|Array|Object} methods
+ * @return {function} middleware
+ */
+
+exports.sendOptions = function(methods){
+ if (Object(methods) === methods) {
+ methods = Object.keys(methods).filter(function(x) {
+ return methods[x];
+ })
+ }
+
+ if (Array.isArray(methods)) {
+ methods = methods.map(function(x) {
+ return x.toUpperCase();
+ }).join(',');
+ }
+
+ return function(req, res, next) {
+ res.setHeader('Allow', methods);
+ res.end();
+ }
+}
+
+/**
+ * Middleware to send a 405 Method Not Allowed response
+ * instead of a 404 page not found.
+ * Should use connect.utils.error but it isn't public.
+ */
+
+exports.methodNotAllowed = function(req, res, next) {
+ var err = new Error();
+ err.status = 405;
+ next(err);
}
View
88 test/app.subroute.js
@@ -0,0 +1,88 @@
+var express = require('../')
+ , request = require('./support/http')
+ , methods = require('methods');
+
+describe('app.subroute.js', function(){
+ describe('methods supported', function(){
+ methods.forEach(function(method){
+ it('should include ' + method.toUpperCase(), function(done){
+ if (method === 'delete') method = 'del';
+ var app = express();
+ var calls = [];
+
+ app.subroute('/foo')[method](function(req, res){
+ if ('head' == method) {
+ res.end();
+ } else {
+ res.end(method);
+ }
+ }).end();
+
+ request(app)
+ [method]('/foo')
+ .expect('head' == method ? '' : method, done);
+ })
+ })
+ })
+
+ describe('options supported', function(){
+ methods.forEach(function(method){
+ it('should include ' + method.toUpperCase(), function(done){
+ if (method === 'delete') method = 'del';
+ var app = express();
+ var calls = [];
+
+ if (method !== 'options') {
+ app.subroute('/foo')[method](function(req, res){
+ if ('head' == method) {
+ res.end();
+ } else {
+ res.end(method);
+ }
+ }).end();
+ } else {
+ app.subroute('/foo').end();
+ }
+
+ request(app)
+ .options('/foo')
+ .expect('Allow', method == 'options' ? 'OPTIONS'
+ : method == 'del' ? 'DELETE,OPTIONS'
+ : method == 'get' ? 'GET,HEAD,OPTIONS'
+ : method.toUpperCase() + ',OPTIONS', done);
+ })
+ })
+ })
+
+ it('should disallow methods', function(done){
+ var app = express();
+ var calls = [];
+
+ app.subroute('/foo').get(function(req, res){
+ res.end('get');
+ }).end();
+
+ request(app)
+ .post('/foo')
+ .expect(405, done);
+ })
+
+ /*
+ it('show allow HEAD for GET', function(done){
+ var app = express();
+ var calls = [];
+
+ app.subroute('/foo').get(function(req, res){
+ if (req.method === 'HEAD') {
+ res.end();
+ } else {
+ res.end('get');
+ }
+ }).end();
+
+ request(app)
+ .head('/foo')
+ .expect(200, done);
+ })
+ */
+})
Something went wrong with that request. Please try again.