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

Add group routes support #69

Merged
merged 1 commit into from Apr 16, 2015

Conversation

Projects
None yet
5 participants
@delgermurun
Contributor

delgermurun commented Apr 7, 2015

var group = FlowRouter.group({
  prefix: '/admin',
  middlewares: [
    function(path, next) {
      console.log('running group middleware');
      next();
    }
  ]
});

group.route('', {
  action: function() {
    FlowLayout.render('componentLayout', {content: 'admin'});
  },
  middlewares: [
    function(path, next) {
      console.log('running /admin middleware');
      next();
    }
  ]
});

group.route('/posts', {
  action: function() {
    FlowLayout.render('componentLayout', {content: 'posts'});
  }
});

PS: If someone has better implementation than mine, feel free to create another PR. I'll close it and let's workon yours.

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented Apr 7, 2015

I pushed prototype. What do you think about API syntax?

@vladshcherbin

This comment has been minimized.

Contributor

vladshcherbin commented Apr 7, 2015

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"});
    }
  });
});
@vladshcherbin

This comment has been minimized.

Contributor

vladshcherbin commented Apr 7, 2015

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"});
  }
});
@jhuenges

This comment has been minimized.

jhuenges commented Apr 7, 2015

I think @delgermurun proposal looks clean and simple.

@arunoda

This comment has been minimized.

Contributor

arunoda commented Apr 7, 2015

I think the first version looks great. How about adding the prefix as the
first argument.

Anyway, is there any use case where we can make prefix optional.

We don't need to work on before and middleware stuff here.
On 2015 අප්‍රේල් 7, අඟහ at ප.ව. 6.36 jhuenges notifications@github.com
wrote:

I think @delgermurun https://github.com/delgermurun way looks clean and
simple.


Reply to this email directly or view it on GitHub
#69 (comment)
.

@vladshcherbin

This comment has been minimized.

Contributor

vladshcherbin commented Apr 7, 2015

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.

@delgermurun delgermurun referenced this pull request Apr 7, 2015

Merged

Add group routes support #61

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented Apr 7, 2015

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?

@arunoda

Anyway, is there any use case where we can make prefix optional.

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...

We don't need to work on before and middleware stuff here.

Why don't we need middleware? How about checking permission for all sub routes?

@vladshcherbin

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.

I don't like controllers. I think we can do it by groups.

@vladshcherbin

This comment has been minimized.

Contributor

vladshcherbin commented Apr 7, 2015

@delgermurun that would be awesome!

Btw, can we somehow divide middlewares into before and after or specify when they are fired?

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented Apr 7, 2015

@vladshcherbin we planned hooks (#59), so I think group support hooks same as route.

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented Apr 7, 2015

@arunoda regarding middlewares, do you mean we should add hooks to group on #59 :)? If so I think it's good idea.

@arunoda

This comment has been minimized.

Contributor

arunoda commented Apr 7, 2015

@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.

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented Apr 8, 2015

Okay. Get it. So let's go with first way.

How about sub group? I think we need subGroup = mainGroup.group(options) smth like that.

@arunoda

This comment has been minimized.

Contributor

arunoda commented Apr 8, 2015

Add group support to the Router class. So, after that return a new Router
object. So, we can have nested groups as we need.

On Wed, Apr 8, 2015 at 5:52 AM Delgermurun Purevkhuu <
notifications@github.com> wrote:

Okay. Get it. So let's go with first way.

How about sub group? I think we need subGroup = mainGroup.group(options)
smth like that.


Reply to this email directly or view it on GitHub
#69 (comment)
.

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented Apr 8, 2015

@arunoda I'm sorry I can't understand you well enough.

Do you mean merging Group class into Router? or ...?

@arunoda

This comment has been minimized.

Contributor

arunoda commented Apr 8, 2015

May be not. This seems fine.
Sorry. I didn't read your code first.

On Wed, Apr 8, 2015 at 6:40 AM Delgermurun Purevkhuu <
notifications@github.com> wrote:

@arunoda https://github.com/arunoda I'm sorry I can't understand well.

Do you mean merging Group class into Router? or ...?


Reply to this email directly or view it on GitHub
#69 (comment)
.

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented Apr 8, 2015

@arunoda ok. No problem.

this.prefix = options.prefix || '';
this._middlewares = options.middlewares || [];
};

This comment has been minimized.

@arunoda

arunoda Apr 8, 2015

Contributor

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.

@delgermurun delgermurun force-pushed the delgermurun:group-routes branch from 22ae565 to f21c49a Apr 8, 2015

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented Apr 8, 2015

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();
    }
  ]
});
@arunoda

This comment has been minimized.

Contributor

arunoda commented Apr 8, 2015

Awesome.
Regarding this route:

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 / instead of "", we should accept it. What do you think?

Edit:
We should avoid user to put "" and throw him an error. We need to accept a path starting with /

};
Group.prototype.route = function(path, options) {
path = this.prefix + path;

This comment has been minimized.

@arunoda

arunoda Apr 8, 2015

Contributor

We need to check this path starts with /. Otherwise throw an error.
We may can do this for Router's route method as well.

options.middlewares = this._middlewares.concat(middlewares);
var route = this._router.route(path, options);
route.group = this;

This comment has been minimized.

@arunoda

arunoda Apr 8, 2015

Contributor

Accept group via the third parameter.

@@ -62,5 +62,9 @@ Route.prototype.callAction = function(current) {
Route.prototype.callSubscriptions = function(current) {
this.clearSubscriptions();
if (this.group) {

This comment has been minimized.

@arunoda

arunoda Apr 8, 2015

Contributor

Define group in the route's constructor.

EDIT: This is implemented now.

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented Apr 8, 2015

@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) {

This comment has been minimized.

@arunoda

arunoda Apr 8, 2015

Contributor

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)

if (parent) {
this.prefix = parent.prefix + this.prefix;
this._middlewares = parent._middlewares.concat(this._middlewares);
this.parent = parent;

This comment has been minimized.

@arunoda

arunoda Apr 8, 2015

Contributor

May be we can define parent always. try this.

this.parent = parent;
if(this.parent) {
   ...
}
@arunoda

This comment has been minimized.

Contributor

arunoda commented Apr 8, 2015

This is quite good. I'm happy to go with the PR.
Now we can write some tests.
This time, I'll let wait until you squash commits :)

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented Apr 8, 2015

@arunoda lessons learned :).

Thanks for the V8 tip. I updated PR.
So I'll write tests shortly (maybe tommorrow)

@arunoda

This comment has been minimized.

Contributor

arunoda commented Apr 8, 2015

Great.

On Thu, Apr 9, 2015 at 3:36 AM Delgermurun Purevkhuu <
notifications@github.com> wrote:

@arunoda https://github.com/arunoda lessons learned :).

Thanks for the V8 tip. I updated PR.
So I'll write tests shortly (maybe tommorrow)


Reply to this email directly or view it on GitHub
#69 (comment)
.

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented Apr 9, 2015

I added tests. Please review.

@lfades

This comment has been minimized.

lfades commented Apr 10, 2015

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

Delgermurun

@delgermurun delgermurun force-pushed the delgermurun:group-routes branch from d268ede to 4471cb4 Apr 16, 2015

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented Apr 16, 2015

Squashed commit

@arunoda

This comment has been minimized.

Contributor

arunoda commented Apr 16, 2015

Awesome @delgermurun

arunoda added a commit that referenced this pull request Apr 16, 2015

Merge pull request #69 from delgermurun/group-routes
[WIP] Add group routes support

@arunoda arunoda merged commit 0d542e5 into kadirahq:group-routes Apr 16, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@delgermurun delgermurun deleted the delgermurun:group-routes branch Apr 16, 2015

@delgermurun

This comment has been minimized.

Contributor

delgermurun commented Apr 16, 2015

Will you merge it before 2.X? @arunoda

@arunoda

This comment has been minimized.

Contributor

arunoda commented Apr 16, 2015

Yes.
see: #61 (comment)

@delgermurun delgermurun changed the title from [WIP] Add group routes support to Add group routes support Jun 4, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment