express icon indicating copy to clipboard operation
express copied to clipboard

Simplify app.all and app.VERB

Open ibc opened this issue 9 years ago • 6 comments

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;
  };
});

ibc avatar Feb 23 '15 17:02 ibc

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 avatar Feb 23 '15 17:02 dougwilson

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.

dougwilson avatar Feb 23 '15 17:02 dougwilson

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
  }
})

ibc avatar Feb 23 '15 17:02 ibc

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

dougwilson avatar Feb 23 '15 18:02 dougwilson

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

ibc avatar Feb 23 '15 18:02 ibc

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.

dougwilson avatar Feb 23 '15 18:02 dougwilson