New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[REQUEST] callback handler for methods #3201
Comments
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 - |
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 |
I'm generally 👎 on this type over override as well, because we used to allow the same thing with |
@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); |
@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) |
Right, but that's how |
ok, so for now I'll leave my express wrapper and maybe in the future there will be a solution that supports this thing |
@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. |
@blakeembrey sorry for the delay, I'm on different timezone 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 |
@zecar I was working on a similar thing... (https://github.com/donode/donode) Its a nice to have functionality for |
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. |
The usecase for this is narrow but the API change is large. Closing as not planned |
I'm referring to this bit
I wanna be able to pass something like
Maybe you could implement something like:
express.callbackHandler()
that we should use like this
The text was updated successfully, but these errors were encountered: