-
Notifications
You must be signed in to change notification settings - Fork 140
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
Allow Passing in Route Middleware on Resource #8
Comments
yeah its a bit tough since certain actions will want some middleware, some will not, which is why in general I'm not a huge fan of rails-ish stuff haha, but we will figure out something that works |
what about allowing for the action to be a function, or an array of functions if middleware is involved? exports.index = function(req, res){ or exports.index = [authenticateUser, function(req, res){ |
i dunno, i just dont want it to become more ugly than just using app.get() and friends |
Yeah, app.get works fine, especially with app.param, thats what I'm doing now. The routes are very clean, I'm just lazy and don't like typing them all out :p |
It looks like this isn't being advanced. How can I get middleware working? Instead of app.resource Use app.(get|post|etc) ? |
+1 Middleware working would be a huge plus, since I am trying to do a API that requires auth for it routes, like so:
And this isn't working. Any insights? |
that looks ok @flockonus, that's essentially what express-resource middleware would be expanding to |
Thanks! In this case, the Middleware request may be postponed :P I just had 1 slash wrong.. For anyone looking for future references;
That means that all requests beginning by /admin... will be filtered and resources bellow will be unavailable unless |
I implimented applying middleware to all resource routes however the middleware array must always be present because i havent worked out checking types and moving params yet. Also i plan to switch the middleware array for an object that would let u define seperate middleware to each action, here what i got so far: |
@flockonus. That's works. and that will have to till they decide to include middleware route to the module. Thxs |
Any updates on this issue? |
i still feel gross about the api, at this point the resource stuff looks worse than some simple app.VERB calls, i wouldnt mind brainstorming some nicer alternatives |
I hear that. I've been trying to think of alternatives myself, and have resorted to namespaces until I find a better resource oriented API. I'll try to put some time in on this idea during the week ahead. I've reached the point where namespacing is getting ugly, and I crave something cleaner. |
yeah i dont like namespaces either. personally i think there's nothing cleaner or simpler than just listing them out https://github.com/visionmedia/express/blob/master/examples/route-separation/app.js#L24 sure it's a bit verbose, but it's dead simple to implement, simple to scan, simple to grep if necessary. perhaps just: app.resource('users', users)
.use(authenticateUser)
.use(whatever) but that obscures placing middleware specifically after, though that is usually less important |
I guess my point is that these sort of abstractions always get in the way, but I agree we need something |
Not to bash the current API, because you did great work with it, but what exists is tolerable. If we could add in a way to pass in route specific middleware, then I think everyone would be happy. But on my previous attempts to add route middleware to the current API, I always made it dirtier. Perhaps a re-think is in order. |
Been using this resource before, I found it kinda hard to fit it in any use other than JSON API, how are you guys use-case? I find it most useful on administrative interface only, in such case where URL don't really matter. This is that case I believe a such a router may solve. As @visionmedia is accepting some brainstorm, I came up with this sort of CRUD code generator (using Mongoose), very alpha, https://gist.github.com/1495306 let me know what you guys think of it. My ultimate goal with this is to make something generic enough, so people can easily build some administrative interface basically out of models( with validations and associations). |
We've been just listing out the app.VERBs and that works well at the beginning - but eventually you have hundreds of lines of routes and it can become a headache. Also with several devs working on a project something like express-resource can help to enforce a standard routing pattern, make updates faster, etc... So I would love to see this worked out. Just ended up here after trying this, hoping it might work like the arrays you can pass to VERBs: module.exports = {
create: [
filters.is_user,
function(req, res) {
// ...
}
],
}; What are people's thoughts on an API like that? I don't like the idea of enforcing the middleware to all routes of a given resource because, as TJ said, most of them will be unnecessary so now your middleware logic gets messy since you're ignoring middleware half the time. |
@hunterloftis That's the kind of API I would prefer. Simple, and to the point. It also would allow us to auto-generate the routes from the extension, and easily add custom routes and verbs as well. It definitely improves the existing extension, while retaining the good parts. |
Just issued a pull request for this: Drop separate functions per format?I omitted more complex handling of formats in middleware'd routes. What do you guys think? I recommend we drop that feature since it makes complex something that is pretty simple. eg: we're already creating routes that include ?format & saving req.format, so: exports.show = function(req, res) {
// common business logic here, no matter what format we're returning
// ...
// now that we've done our logic we can respond based on the format:
if (req.format === 'json') // ...
else if (req.format === 'xml') // ...
else // ...
}; Is the separate function thing actually something people use? Am I overlooking a use-case? |
A better solution that breaks backwards-compatibility for function-mapped-formats: |
The way of use looks really fine, definitely a improvement |
if you break down some of the business logic into middleware you can use the format callbacks quite easily but I dont disagree with what you're saying |
IMHO it's still easier to reason with: exports.show = [
loadUser,
ensureRole('admin'),
function(req, res) {
if (req.format == 'json') {
...
} else {
...
}
}
]
app.get('/users*', loadUser, ensureRole('admin'));
app.get('/users.json', users.json);
app.get('/users', users.html); but I'll pull the request down and have a better look |
I agree that middleware is an even better solution for most use cases. I just wanted to be clear that my implementations omit the |
too bad they're not like lua tables we could just have both cleanly. for now i'll probably merge the one that doesn't break backwards compat |
I'm not the best with js but thought I'd share the hack I'm considering running with as a solution to this problem: https://gist.github.com/3102077. Just out of interest has anything been decided on this? |
FYI, I've create a pull request supporting middleware. It's fully backward compatible and it includes tests and documentation. |
What is the status on this? Is express-resource discontinued? |
it's not discontinued, im just not a huge fan of this sort of API |
I can understand that - maybe close the pull then, so people won't wait for it? I went with the |
It would be nice to be able to set route middleware. Specifically, when mapping the users resource:
var user = app.resource('users', require('./user'));
It would be great to do something like:
var user = app.resource('users', require('./user'), { before: authenticateUser });
which is would be the equivalent of
app.get('/users', authenticateUser, require('./user').index);
The text was updated successfully, but these errors were encountered: