Add group routes support #69
Conversation
|
I pushed |
|
Maybe smth like that FlowRouter.group({
prefix: 'admin',
before: 'guest',
after: function() {
console.log('after filter');
},
subscriptions: function () {
this.register('services', Meteor.subscribe('services.all'));
}
}, function () {
FlowRouter.route('/services', {
name: 'admin.services',
before: function() {
console.log('before filter');
},
action: function () {
FlowLayout.render('adminLayout', {main: "adminServices"});
}
});
FlowRouter.route('/services/:slug', {
name: 'admin.service',
subscriptions: function (params, queryParams) {
this.register('additional', Meteor.subscribe('services.smth', params.smth));
},
action: function (params) {
FlowLayout.render('adminLayout', {main: "adminService"});
}
});
}); |
|
controller would be awesome here, but I think @arunoda doesn't want to have controllers. maybe we can have smth like flow-controllers. or not. routes.js FlowRouter.group({prefix: 'admin', before: 'guest'}, function () {
FlowRouter.route('/services', {
name: 'admin.services',
action: 'AdminServicesController@index'
});
FlowRouter.route('/services/:slug', {
name: 'admin.service',
action: 'AdminServicesController@show'
});
});controller AdminServicesController = AdminController.extend({
after: function() {
console.log('after filter');
},
subscriptions: function () {
this.register('services', Meteor.subscribe('services.all'));
},
index: function () {
this.before = function() {
console.log('before filter');
};
FlowLayout.render('adminLayout', {main: "home"});
},
show: function (params) {
this.subscriptions = function () {
this.register('additional', Meteor.subscribe('services.additional', params.smth));
};
FlowLayout.render('adminLayout', {main: "home"});
}
}); |
|
I think @delgermurun proposal looks clean and simple. |
|
I think the first version looks great. How about adding the prefix as the Anyway, is there any use case where we can make prefix optional. We don't need to work on before and middleware stuff here.
|
FlowRouter.route('/login', {
name: 'login',
action: function () {
FlowLayout.render('mainLayout', {main: "login"});
Meta.setTitle('Login Title');
GAnalytics.pageview("/login");
}
});
FlowRouter.route('/logout', {
name: 'logout',
action: function () {
// do smth
}
});
var servicesGroup = FlowRouter.group({
prefix: '/admin',
middlewares: [
function(path, next) {
console.log('running group middleware');
next();
}
]
});
servicesGroup.route('', {
name: 'admin',
action: function() {
FlowLayout.render('componentLayout', {content: 'admin'});
},
middlewares: [
function(path, next) {
console.log('running /admin middleware');
next();
}
]
});
servicesGroup.route('/posts', {
name: 'posts',
action: function() {
FlowLayout.render('componentLayout', {content: 'posts'});
}
});
FlowRouter.route('/somewhere', {
name: 'somewhere',
action: function () {
FlowLayout.render('mainLayout', {main: "somewhere"});
Meta.setTitle('somewhere');
GAnalytics.pageview("/somewhere");
}
});Still looks cool, simple and great ? Great is when you see line starting from FlowRouter and not searching for a var somewhere. Route definition should be the same and start the same. Smth like so: FlowRouter.group({
FlowRouter.route('/', {
});
FlowRouter.route('/posts', {
});
});This way, you can see immediately, what is the group and how many and what routes does it have. |
|
I think we can have both. So person can choose which one to use. It doesn't add too much complexity to code. How about that?
I think all options should be optional. So we can use them more freely. Maybe on one group just use subs, on another one use subs, middlewares etc...
Why don't we need middleware? How about checking permission for all sub routes?
I don't like controllers. I think we can do it by groups. |
|
@delgermurun that would be awesome! Btw, can we somehow divide middlewares into before and after or specify when they are fired? |
|
@vladshcherbin we planned hooks (#59), so I think group support hooks same as route. |
|
@delgermurun no don't want to have ambiguous APIs. I get it. We need to make the offset optional. So, current way is fine. We need middleware. I was reffering to hooks. We can deal with it later in a different PR. That's because, that's a huge change. I don't want to add it to this PR. |
|
Okay. Get it. So let's go with first way. How about sub group? I think we need |
|
Add group support to the Router class. So, after that return a new Router On Wed, Apr 8, 2015 at 5:52 AM Delgermurun Purevkhuu <
|
|
@arunoda I'm sorry I can't understand you well enough. Do you mean merging Group class into Router? or ...? |
|
May be not. This seems fine. On Wed, Apr 8, 2015 at 6:40 AM Delgermurun Purevkhuu <
|
|
@arunoda ok. No problem. |
| this.prefix = options.prefix || ''; | ||
|
|
||
| this._middlewares = options.middlewares || []; | ||
| }; |
There was a problem hiding this comment.
May be we need to introduce group prototype to here.
In that case, we could simply use this into the Router class.
I'm back to my previous comment?
What do you think? I'm okay for both.
|
I update PR. I added sub route and subscriptions. Can you guys review code? Here is example route declaration: FlowRouter.route('/', {
action: function() {
FlowLayout.render('templateLayout', {content: 'home'});
}
});
var group = FlowRouter.group({
prefix: '/admin',
subscriptions: function(params) {
console.log('subscribing from group');
this.register('adminGroup', Meteor.subscribe('adminGroup'));
},
middlewares: [
function(path, next) {
console.log('running group middleware');
next();
}
]
});
group.route('', {
action: function() {
FlowLayout.render('componentLayout', {content: 'admin'});
},
subscriptions: function(params) {
console.log('subscribing from route');
this.register('adminPage', Meteor.subscribe('adminPage'));
},
middlewares: [
function(path, next) {
console.log('running /admin middleware');
next();
}
]
});
group.route('/posts', {
action: function() {
FlowLayout.render('componentLayout', {content: 'posts'});
}
});
var subGroup = group.group({
prefix: '/pages',
subscriptions: function(params) {
console.log('subscribing from sub group');
this.register('adminSubGroup', Meteor.subscribe('adminSubGroup'));
},
middlewares: [
function(path, next) {
console.log('running sub group middleware');
next();
}
]
});
subGroup.route('', {
action: function() {
FlowLayout.render('componentLayout', {content: 'adminPages'});
},
subscriptions: function(params) {
console.log('subscribing from /admin/pages route');
this.register('adminPageList', Meteor.subscribe('adminPageList'));
},
middlewares: [
function(path, next) {
console.log('running /admin/pages middleware');
next();
}
]
}); |
|
Awesome. group.route('', {
action: function() {
FlowLayout.render('componentLayout', {content: 'admin'});
},
subscriptions: function(params) {
console.log('subscribing from route');
this.register('adminPage', Meteor.subscribe('adminPage'));
},
middlewares: [
function(path, next) {
console.log('running /admin middleware');
next();
}
]
});Even if the user gives Edit: |
| }; | ||
|
|
||
| Group.prototype.route = function(path, options) { | ||
| path = this.prefix + path; |
There was a problem hiding this comment.
We need to check this path starts with /. Otherwise throw an error.
We may can do this for Router's route method as well.
|
@arunoda updated. Please review changes. Thank you. |
| @@ -11,6 +11,10 @@ Route = function(router, path, options) { | |||
| this._middlewares = options.middlewares || []; | |||
| this._subsMap = {}; | |||
| this._router = router; | |||
|
|
|||
| if (group) { | |||
There was a problem hiding this comment.
Always try to define group here. Don't use if. (like this.group = group)
Then V8 can optimize it.
(Not much critical here since we don't have heavy use of this class, but still good to consider)
|
This is quite good. I'm happy to go with the PR. |
|
@arunoda lessons learned :). Thanks for the V8 tip. I updated PR. |
|
Great. On Thu, Apr 9, 2015 at 3:36 AM Delgermurun Purevkhuu <
|
|
I added tests. Please review. |
|
Hi guys, I previously had this code in my application to support groups var group = function (options) {
this._middlewares = options.middlewares || [];
this._prefix = options.prefix;
};
group.prototype._setMiddlewares = function (options) {
var middlewares = _.union(this._middlewares, options.middlewares || []);
if (middlewares.length)
options.middlewares = middlewares;
};
group.prototype.route = function (path, options) {
if (this._prefix)
path = this._prefix + path;
this._setMiddlewares(options);
FlowRouter.route(path, options);
};
group.prototype.group = function (options) {
if (this._prefix && options.prefix)
options.prefix = this._prefix + options.prefix;
this._setMiddlewares(options);
return new group(options);
};
FlowRouter.group = function (options) {
return new group(options);
};but I am very happy that there is already an official version and not have to change much my routes, I'll be waiting for its release if I can check the permissions of a user in a route (using 2 collections) without an autorun my migration to flow-router would be complete and no more reruns of iron-router in my application Thanks @delgermurun and @arunoda for help both the community, I hope to be like you someday |
|
Squashed commit |
|
Awesome @delgermurun |
[WIP] Add group routes support
|
Will you merge it before 2.X? @arunoda |
|
Yes. |
PS: If someone has better implementation than mine, feel free to create another PR. I'll close it and let's workon yours.