Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

added a more powerful route group class 馃敟 #212

Merged
merged 4 commits into from
Jul 11, 2018

Conversation

josephmancuso
Copy link
Member

@josephmancuso josephmancuso commented Jul 10, 2018

This PR adds a much better implementation for route groups.

We can now use route groups like this:

from masonite.routes import RouteGroup
from masonite.helpers.routes import get

ROUTES = [
    get('/url1', ...),
    get('/url/2', ...),

    RouteGroup([
        get('/url3', ...),
        get('/url4', ...),
        get('/url5', ...),
    ]),

]

Not only can we do this seemingly useless route group but we also have a few keywords we can use with it:

ROUTES = [
    get('/url1', ...),
    get('/url/2', ...),

    RouteGroup([
        get('/url3', ...),
        get('/url4', ...),
        get('/url5', ...),
    ], middleware = ('auth', 'login')),

]

or

ROUTES = [
    get('/url1', ...),
    get('/url/2', ...),

    RouteGroup([
        get('/url3', ...),
        get('/url4', ...),
        get('/url5', ...),
    ], domain = ('www')),

]

note - we can use a list as well but i think a tuple looks cleaner.

We have access to adding these attributes on each route: middleware, domain, prefix (route prefix), name.

The name would look something like:

ROUTES = [
    get('/url1', ...),
    get('/url/2', ...),

    RouteGroup([
        get('/url3', ...).name('create'),
        get('/url4', ...).name('update'),
        get('/url5', ...).name('delete'),
    ], name = 'post.'),

]

which will generate routes with names like post.create, post.update and post.delete

@josephmancuso josephmancuso self-assigned this Jul 10, 2018
@coveralls
Copy link

coveralls commented Jul 10, 2018

Coverage Status

Coverage increased (+0.2%) to 91.5% when pulling 41ec292 on add-more-group-options into f2853d7 on master.

@josephmancuso
Copy link
Member Author

josephmancuso commented Jul 10, 2018

Added the ability for multiple nested route groups. This nest can be as deep as you like. I have only tested 3 routes deep thought. This will look something like:

ROUTES = [
    get('/url1', ...),
    get('/url/2', ...),

    RouteGroup([
        get('/url3', ...).name('create'),

        RouteGroup([
             get('/url4', ...).name('update'),
             get('/url5', ...).name('delete'),
        ], prefix = '/dashboard'),

    ], name = 'post.'),
]

will generate something like:

/url/1
/url/2
/url/3 with the name of post.create
/dashboard/url/4 with the name of post.update
/dashboard/url/5 with the name of post.delete

@josephmancuso
Copy link
Member Author

josephmancuso commented Jul 10, 2018

Here is a real world representation of what this would look like after being refactored in one of my applications:

ROUTES = [

    RouteGroup([
        # Dashboard Routes
        RouteGroup([
            # App Routes
            RouteGroup([
                Get().route('', 'AppController@show').name('show'),
                Get().route('/create', 'AppController@create').name('create'),
                Post().route('/create', 'AppController@store').name('store'),
                Post().route('/delete', 'AppController@delete').name('delete'),
            ], prefix='/apps', name='app.'),
            
            Get().route('/plans', 'PlanController@show').name('plans'),
            Post().route('/plans/subscribe', 'PlanController@subscribe').name('subscribe'),
            Post().route('/plans/cancel', 'PlanController@cancel').name('cancel'),
            Post().route('/plans/resume', 'PlanController@resume').name('resume'),
        ], prefix="/dashboard", middleware=('auth',)),

        # Login and Register Routes
        Get().route('/login', 'LoginController@show').name('login'),
        Get().route('/logout', 'LoginController@logout'),
        Post().route('/login', 'LoginController@store'),
        Get().route('/register', 'RegisterController@show'),
        Post().route('/register', 'RegisterController@store'),
        Get().route('/home', 'HomeController@show').name('home'),

        # Base Routes
        Get().route('/', 'WelcomeController@show').name('welcome'),
        Post().route('/invite', 'InvitationController@send').name('invite'),
    ], domain='www'),


    # Subdomain invitation routes
    Post().domain('*').route('/invite', 'InvitationController@subdomain').name('invite.subdomain'),
    Get().domain('*').route('/', 'WelcomeController@subdomain').name('welcome'),
]

which is refactored from this:

ROUTES = [
    Get().domain('www').route('/', 'WelcomeController@show').name('welcome'),
    Post().domain('www').route('/invite', 'InvitationController@send').name('invite'),
    Get().domain('www').route('/dashboard/apps', 'AppController@show').name('app.show').middleware('auth'),
    Get().domain('www').route('/dashboard/apps/create', 'AppController@create').name('app.create').middleware('auth'),
    Post().domain('www').route('/dashboard/apps/create', 'AppController@store').name('app.store'),
    Post().domain('www').route('/dashboard/apps/delete', 'AppController@delete').name('app.delete'),
    Get().domain('www').route('/dashboard/plans', 'PlanController@show').name('plans').middleware('auth'),
    Post().domain('www').route('/dashboard/plans/subscribe', 'PlanController@subscribe').name('subscribe'),
    Post().domain('www').route('/dashboard/plans/cancel', 'PlanController@cancel').name('cancel'),
    Post().domain('www').route('/dashboard/plans/resume', 'PlanController@resume').name('resume'),

    Post().domain('*').route('/invite', 'InvitationController@subdomain').name('invite.subdomain'),
    Get().domain('*').route('/', 'WelcomeController@subdomain').name('welcome'),
]

ROUTES = ROUTES + [
    Get().domain('www').route('/login', 'LoginController@show').name('login'),
    Get().domain('www').route('/logout', 'LoginController@logout'),
    Post().domain('www').route('/login', 'LoginController@store'),
    Get().domain('www').route('/register', 'RegisterController@show'),
    Post().domain('www').route('/register', 'RegisterController@store'),
    Get().domain('www').route('/home', 'HomeController@show').name('home'),
]

@josephmancuso
Copy link
Member Author

josephmancuso commented Jul 10, 2018

I would like to add a controller prefix like so:

# App Routes
RouteGroup([
    Get().route('', 'show').name('show'),
    Get().route('/create', 'create').name('create'),
    Post().route('/create', 'store').name('store'),
    Post().route('/delete', 'delete').name('delete'),
], prefix='/apps', name='app.', controller='AppController'),

but the issue is that we find the controller and attach it to the route on bootup time when that route method is originally called so it will look for create but will throw a whole bunch of errors.

One solution is to delay that finding of the controller until after and put the logic of finding the controller in the route provider but then that will be called on every request is slow down the framework. I would rather load all the classes somehow on bootup time.

We could possibly move it somewhere else before bootup time like adding a private method in the AppProvider that runs through the WebRoutes and then attaches the controllers to them but maybe that should be a 2.1 thing? I'm open to ideas.


This would also only work for string controllers since we can't break up something like:

from app import Controller
# App Routes
RouteGroup([
    Get().route('', update).name('show'),
], prefix='/apps', name='app.', controller=Controller),

since we can't throw update in there so I'm thinking we just never make this possible? maybe? or if we do it will deff be a 2.1 fix

@josephmancuso josephmancuso changed the title added a more powerful route group class added a more powerful route group class 馃敟 Jul 10, 2018
@josephmancuso
Copy link
Member Author

josephmancuso commented Jul 10, 2018

Also by the way, for documentation purposes, it may look weird to specify everything below the route. Since all parameters are keyword arguments we can do that simply:

RouteGroup(prefix='/apps', name='app.', middleware = ('auth',), routes = [
    Get().route('', 'AppController@show').name('show'),
    Get().route('/create', 'AppController@create').name('create'),
    Post().route('/create', 'AppController@store').name('store'),
    Post().route('/delete', 'AppController@delete').name('delete'),
]),

Copy link
Contributor

@aisola aisola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job!

@josephmancuso josephmancuso merged commit e54ad6c into master Jul 11, 2018
@josephmancuso josephmancuso deleted the add-more-group-options branch July 11, 2018 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants