Express: [REQUEST] callback handler for methods

[zecar]

I'm referring to this bit

Route.prototype[method] = function(){
    var handles = flatten(slice.call(arguments));

    for (var i = 0; i < handles.length; i++) {
      var handle = handles[i];

      if (typeof handle !== 'function') {
        var type = toString.call(handle);
        var msg = 'Route.' + method + '() requires callback functions but got a ' + type;
        throw new Error(msg);
      }

I wanna be able to pass something like

app.get('/api/user', 'User@get');

Maybe you could implement something like:
express.callbackHandler()
that we should use like this

express.callbackHandler(function(cb){
if(cb.constructor === String){
return require(cb)
}
return cb;
})

[blakeembrey]

That doesn't sound like a good idea for a few different reasons. Mostly, it would make it very hard to reason about the code and the functionality you want seems very specific to you. There's also better ways to do this that works for everyone - for instance, just write it - app.get('/api/user', require('User@get')). Less magic is usually better.

[zecar]

yeah I already have a wrapper for this. I know it sounds specific to me, but I also think this would help. I mean it can help get rid of more writing.. I shouldn't have to use a wrapper or wrap the string with a function for every route that i have

[dougwilson]

I'm generally 👎 on this type over override as well, because we used to allow the same thing with app.param, but it has lead to continual confusion about how to not only implement the overrides (issue #3202 just happened about this), but then also people confused about why things were not working, not realizing that some example somewhere was override the behavior and continuously results in confused users and increased support cost.

[pronebird]

@zecar why can't you use objects and functions directly?

var user = {
    getRandom: function (req, res) {
        res.send({ "username": "john doe" });
    }
};

app.get('/api/user', user.getRandom);

[zecar]

@pronebird i'm keeping separate files for controllers and routes. And for the sake of keeping only routes logic in the routes file I needed a wrapper so I would not be forced to require the controller inside the routes file

@dougwilson why would that lead to confusion? I'm talking strictly about the routing part. I mean the behaviour would not change at all. The only thing that would change is that the router would also accept strings or objects with the condition that a handler is defined. Else an error would be given.

As I see it, this thing would lead to changing the way an express based app is structured in files. In my vision, a well structured app should have a separate routes file, a separate folder for controllers and separate files for controllers, and separate files/folders for every component. In my head it doesn't make much sense to include all my controllers in the routes file just so I can put them in the route logic (see @pronebird 's example)

[dougwilson]

The only thing that would change is that the router would also accept strings or objects with the condition that a handler is defined. Else an error would be given.

Right, but that's how app.param works today with dozens and dozen of issues logged here regarding to that exact confusion. I also forgot to mention that this also really breaks down with tools like TypeScript and other systems with are checking types, because the type that would be accepted is no longer predictable and depends on whatever someone has done to modify the app.

[zecar]

ok, so for now I'll leave my express wrapper and maybe in the future there will be a solution that supports this thing

[blakeembrey]

@zecar I think keeping as a wrapper and publishing it is better. The core should be solid and have less opinions/easier to test. The wrapper itself might not fulfil everyone's needs - for instance, I use TypeScript and having magic strings makes sure that all that logic now offloads onto the runtime instead of being possible at the compiler time. Of course, it _may_ be possible to catch this at compile time depending on how the wrapper is written, but it's just a lot of ifs and maybe.

Maybe you could share an example of what you want to do and we could suggest a wrapper-style that works for you? For instance, it's probably better to abstract away Express methods entirely and instead write a little function that does what you want:

function createRouter (routes) {
  const router = express.Router()

  // Iterate over the routes object somehow and do your require/mapper logic here.

  return router
} // Now you have the router set up how you want, just put it somewhere.

[zecar]

@blakeembrey sorry for the delay, I'm on different timezone
this is my wrapper class

class Route {
    constructor() {
        methods.forEach((method) => {
            this[method] = (...callbacks) => {
                var route = callbacks.shift();
                callbacks = callbacks.map((cb) => {
                    if(cb.constructor === String){
                        cb = use(cb);
                    }
                    return cb;
                })
                callbacks.unshift(route);
                Express.app[method].apply(Express.app, callbacks);
            }
        })
    }
}

and this is my routes.js file

var Route = use('Route');

Route.get('/', 'User@get');
Route.get('/create', 'User@create');

the use() function just does some regex validation/processing and then requires the result

I'm wrapping only for app.method for now. I'll extend later to use express.Router() and later I want to add functionality like this

var Router = use('Router');

Router.group('/api', (Router) => {
    Router.get('/user', 'User@get');
    Router.group('/conversation', (Router) => {
        Router.get('/', 'Conversation@get');
        Router.get('/messages', 'Conversation@getMessages');
    })
})
// This will create the following routes
// /api/user, /api/conversation, /api/conversation/messages

With all the work I have, I'll probably finish it somewhere at the begining of march. I'll publish it for sure :)

P.S.: just in case you ask me why don't I use that 'use()' function because it's short and less to write: I'm just weird, I find it more cleaner with simple strings. Plus, if the 'use' function will become obsolete or deprecated or whatever, I don't have to modify the routes file

[madhusudhand]

@zecar
In case of Conversation@get, what would be the type of Conversation ?
A wrapper function or an object ?

I was working on a similar thing... (https://github.com/donode/donode)
ClassName@method having the route handler as string.

Its a nice to have functionality for express.
Let me know the branch you are working on.

[Leestex]

@zecar @madhusudhand

You guys are trying to achieve something like sails.js has in sails.config.routes:

module.exports.routes = {
    'GET /api/user': { controller: 'User', action: 'get' },
    // or
    'GET /api/user': 'User.get'
}

I like it, but in this case express should know where this kind of controller is located, which seems to be an overhead for such library.