expressjs / express

Fast, unopinionated, minimalist web framework for node.
https://expressjs.com
MIT License
65.47k stars 16.05k forks source link

Simplify app.all and app.VERB #2563

Open ibc opened 9 years ago

ibc commented 9 years ago

https://github.com/strongloop/express/blob/5.0/lib/application.js

Currently:

/**
 * Delegate `.VERB(...)` calls to `router.VERB(...)`.
 */

methods.forEach(function(method){
  app[method] = function(path){
    if ('get' == method && 1 == arguments.length) return this.set(path);

    var route = this.route(path);
    route[method].apply(route, slice.call(arguments, 1));
    return this;
  };
});

/**
 * Special-cased "all" method, applying the given route `path`,
 * middleware, and callback to _every_ HTTP method.
 *
 * @param {String} path
 * @param {Function} ...
 * @return {app} for chaining
 * @api public
 */

app.all = function(path){
  var route = this.route(path);
  var args = slice.call(arguments, 1);
  methods.forEach(function(method){
    route[method].apply(route, args);
  });

  return this;
};

But the following simplified code does exactly the same (in fact lib/Router/index.js does it):

/**
 * Delegate `.all(...)` and `.VERB(...)` calls to `Router#all(...)` and `Router#VERB(...)`.
 */

methods.concat('all').forEach(function(method){
  app[method] = function(path){
    if ('get' == method && 1 == arguments.length) return this.set(path);

    var route = this.route(path);
    route[method].apply(route, slice.call(arguments, 1));
    return this;
  };
});
dougwilson commented 9 years ago

It may seem different from the outside, but in fact is very different in the internal structure it creates. We can look into the consequences of changing this as part of 5.0.

dougwilson commented 9 years ago

Actually it changes the outside behavior as well; right now app.all only responds to known verbs. This change will make it also respond to unknown verbs.

ibc commented 9 years ago

Well, Router#all() already respond to unknown verbs, am I wrong?:

// create Router#VERB functions
methods.concat('all').forEach(function(method){
  Router.prototype[method] = function (path) {
    var route = this.route(path)
    route[method].apply(route, slice.call(arguments, 1))
    return this
  }
})
dougwilson commented 9 years ago

Right, you are not wrong. But app.all doesn't, so it would be a change of behavior for app.all

ibc commented 9 years ago

Since app.xxxx() is supposed to be a simple proxy to the main Router#xxxx(), shouldn't the behavior be the same?

dougwilson commented 9 years ago

In 4.x, it is not a simple proxy. Perhaps it can be in 5.0, but the behavior change just needs to be evaluated first.