Add support for route definition in modules

[damianham]

Process route definitions in src/modules/**/*_routes.cr.

• add parse_routes_file() to src/amber/cli/command/routes.cr
• process all src/modules/**/*_routes.cr files before config/routes.cr

Description of the Change

Modules (e.g. using the misc/modular recipe) organise code in modules
located in src/modules. This change allows the module to also contain
the route definitions.

Alternate Designs

None

Benefits

Modules become more self contained with routes specified inside the module.

Possible Drawbacks

In order to take advantage of this change the modular scaffold generator would need to add routes in the module which differs from the way routes are currently added.
No tests added to the test suite to test this feature. In order to test this change fully we need to create a modular test application with the routes defined in the module. The current modular recipe (misc/modular) does not define the routes in the module, it uses the standard route definition process. This illustrates that we could benefit from having some recipes for test purposes only, the question arises then of where these test recipes would be stored.

[faustargz]

Nice change, this would require more reviews from core team, though 👍

[faustargz]

You can use Dir.glob("src/modules/**/*_routes.cr") as well

[eliasjpr]

Can we make "src/modules/**/*_routes.cr" a setting in the amber.yml file? This will allow some flexibility and avoid hard coding this value

[eliasjpr]

Also what do you think of creating a method and removing the comment.

def parse_modules_routes_file
  Dir.glob(["src/modules/**/*_routes.cr"]) do |file|
   parse_routes_file(file)
  end
end

[robacarp]

This needs some sort of test to go along with it.

The big caveat here is as I mentioned in my comment. When multiple routes match an incoming URL, the first route defined is what is served by Amber. In a single route file, this is pretty straightforward. When the framework globs a bunch of routes files together, this becomes less obvious.

I'd like to propose an alternate method which is more explicit. What about adding a macro method to the route definition which allows an additional file to be included inline in the route file?

For example:

config/routes.cr

Amber::Server.configure do |app|
  pipeline :web do
    # ...
  end

  routes :web do
    # ...
    include_routes "routes/admin"
    # ...
  end
end

config/routes/admin.cr

get "admin/dashboard"

[robacarp]

I would have expected to read the global routes first because which are defined first have a higher priority in the router, and the config/routes.cr file seems like a place of higher priority.

[damianham]

I purposefully put the module routes first as they will/should be more specific and thus should have a higher priority. In the case where you use the scaffold generator to create a new module, the routes will all have the module name prefix and it will be up to the developer to ensure the prefix does not clash with existing routes in config/routes.cr. In the case in the future where a module is added to the application via a plugin then I expect the routes in the plugin module should be namespaced to avoid clashes with other routes.

[robacarp]

In all situations where there is a prefix and there is no conflict, it's fine.

As a developer with an existing application in production, when I install a plugin which conflicts with my existing routing, I expect my routing to take precedence, not the plugin.

Moreover, when a conflict inevitably happens, I want a way to override it. When a plugin is installed which conflicts with my existing routing, I shouldn't have to edit the plugin code or generated code to get my existing routes back.

I think our disagreement on this serves to illustrate why implicitly merging routes files is going to cause confusion, regardless of the order of operations.

To amend my previous example for a shard-based plugin or recipe:

  routes :web do
    # ...
    include_routes "routes/admin"
    include_routes "../lib/plugin/routes"
    # ...
  end

[damianham]

not a disagreement.... more a discussion 😄
You are right a plugin routing should not conflict with your existing routing. With plugins I envisage the routes would be namespaced - if we curate the plugins - at least curate a list of plugins that conform to a format - we can ensure that plugin routes are namespaced. We can ask that plugin authors namespace the plugin routes with a namespace unique to the plugin.

[damianham]

BTW good idea about ../lib/plugin/routes if plugins are installed as shards (but not sure if they are going to be added to the app/shard.yml yet)

[damianham]

@robacarp the end goal here is to have drop in code modules via plugins that add considerable functionality to your app, e.g. calendar, photo gallery etc. I think it is better to keep everything related to a module in the module folder including it's routes so that the only changes adding a plugin makes is adding a folder to src/modules (or src/plugins if we want to differentiate user developed code from plugins). That also makes it simple to remove the plugin if you change your mind. The advantage of including a macro in the global routes file is that you can explicitly see the inclusion of a routes file and where it comes in the routes order. However you can see the routes order with the 'amber routes'
command and using a routes file in the module I think allows for maximum flexibility.

[robacarp]

@damianham Your end goal is 👍 I've written code to solve this problem for a dozen rails projects because it's so easy for a routes file to get out of control.

But implicitly loading routes does not allow for maximum flexibility. Allowing the developer to declare where routes are injected into their application allows more flexibility.

What if I want the calendar plugin because I want some library code, but I don't want it's routes at all? Should I have to fork the plugin?

What if I want the photo gallery plugin but I don't like the way the #index view is written, should I have to fork the plugin to simply override one view action in the same namespace? If I can define my own route with higher precedence, it's easy. If hidden route files take precedence over my own route definitions, it's impossible without a fork.

[damianham]

In the case where you want the code for the plugin but not the routes you would remove the routes file. If the plugin adds a module to src/modules it becomes application code like everything else that you can modify as you wish so I don't think you will need to fork in either situation. I don't think anything will be hidden - I would prefer plugins to add boilerplate code alongside my application code which I can then modify to suit. BTW this is the way that MEAN.js organizes front and backend code - it isn't my original idea.

[damianham]

@robocarp on reflection I cannot disagree with you - I think there is a case for having a macro to include routes in a pipeline from another file. However I think there is still the case for defining routes in files in modules and may be the preferred option for some people, it is definitely my preferred option. Also by defining routes in a file in a module the routes can also be inserted into a specific pipeline for the module, e.g. a pipeline without authentication for routes pertaining to signin in, resetting password, oauth etc.

[faustargz]

@robacarp I like your idea:

  routes :web do
    # ...
    include_routes "routes/admin"
    include_routes "../lib/plugin/routes"
    # ...
  end

Looks very nice IMO 👍

[Sija]

Just to add my 2c to the discussion: rails allows for mounting rack/rails sub-apps, with individual route definitions:

# ...

constraints :subdomain => 'resque' do
  authenticated :user, -> (user) { user.admin? } do
    mount Resque::Server => '/' # that's the one
  end
end

# ...

Personally, I'm finding it as quite clear and flexible way of managing your routes.
Doing sth similar here (with include_routes or otherwise) would be imo already an improvement.
In amber-way I'd go for DSL similar to:

mount "/", Resque::Server

[damianham]

@Sija we can achieve the same with route scopes and I envisage plugins mounting on a route scope.

[drujensen]

@damianham I agree with @robacarp that a more explicit approach is better than the glob. Changing the label to WIP.

BTW, at some point, we should have a discussion of modules and the up/down side of that kind of structure. I personally prefer all the models, views, controllers, mailers, jobs, services, etc. neatly grouped together and a clear separation of concern but I come from a day before MVC and you would find db calls in your asp/jsp/php templates. My fear is you lose that clean separation and go back to a day when it gets all mixed together again.

Technology is always on a sine wave. I see old trends/patterns come back over and over again. Modules is obviously not new and we moved away from it back in the day for some very good reasons. Maybe those reasons are not well founded anymore, not sure. I'm just an old geezer that has been there and done that (several times). ;-)

[damianham]

@drujensen yeah I have been around the blocks a few times too. I don't think you lose clean separation - things are still in their respective files. Furthermore modules elegantly answer the question of where to put your business logic if you want to keep both your controllers and models skinny. Modules provide high cohesion for all units concerned with a particular feature and by allowing modules to define their own routes in a file within the module that high cohesion is maintained.

[faustargz]

@damianham Is this a breaking change? Does this change default MVC behavior on amber? or is it just an addition to support routes inside modules optionally?

[damianham]

@faustinoaq it is definitely NOT a breaking change - it only adds support for routes inside modules optionally.

[faustargz]

@damianham Oh, ok, nice, no problem 👍

[drujensen]

@damianham Having separate route files for each feature does have some merit. I usually try to keep the routes clean with clear separation of features but I have seen route files get fairly ugly.

In your layout, how do you handle inheritance? Where does shared stuff go like the application_controller? Please don't say shared folder. ;-)

[damianham]

@drujensen application_controller stays in src/controllers as does home_controller.cr. When I have shared stuff I would put common crystal libraries in src/lib and common javascript libraries in src/lib/js.

[eliasjpr]

I am not sure about this I still have to test it, but I was thinking that it might not be necessary to add more magic to support this. I believe that users of the framework can define their routes in directories under src and just require the file in the src/app.cr we can obtain the same effect with not added code.

For instance, define a route file in a subdirectory in src as such src/module_name/route.cr, we can basically define a "mountable" app with views and models as a module.

Defining a Module

src/module_name/route.cr
src/module_name/views/
src/module_name/models/
src/module_name/controllers/

Contents for src/module_name/route.cr

Amber::Server.configure do |app|
   # Only define module specific routes to separate concerns
   # And scope if necessary
   routes :web, '/module_name' do
     resources :something ...
   end
end

Then require in src/app.cr

require './module_name/**'
require '../config/*'

Amber::Server::Start

This is a really just changing the conventions of Amber as where Amber is looking for controllers directory in ./src/controllers it would be now imported from the controllers directory. There is some re-defining the conventions of Amber. It takes more work but is more explicit

WDYGT?

[damianham]

@eliasjpr you are right that this can all be achieved manually by the developer and is not strictly neccessary, it is more of a convenience. The impetus behind this change is that a generator or plugin may create a self contained module without the developer needing to modify config/application.cr to require the new module/plugin routes.

[drujensen]

@damianham why wouldn't the generator add this to the config/application.cr?

[damianham]

@drujensen I think the answer would be that a plugin module would then have to be a generator program instead of simply a set of template files that we process with Liquid. We have talked previously about having hooks for plugins and recipes which could possibly amend config/application.cr and perform other integration tasks but that has not been worked out yet.

[drujensen]

@damianham ahhh! I just put 2 and 2 together. Thanks.

[eliasjpr]

@damianham in that case how can we guarantee that the developer will always define modules under the src/modules as is defined https://github.com/amberframework/amber/pull/810/files#diff-eb22ae110d5e9148a4e2657f50faf2e7R48 and not under a different directory?

While I think adding support for modular approach is a great idea, I think we should consider spending some more time trying to understand how we should approach this particular problem, if we merge this change we will be going down a path that is going to add lots of complexity down the road.

On another note the CLI is growing in size and complexity with the recent additions, we should strongly consider moving it to its own shard. I noticed some extensive duplication between the Recipes and the Templates implementation.

[damianham]

@eliasjpr we always have to adopt some conventions, we have done that with every other folder name. It seemed to me that 'modules' was the most appropriate name for a folder that contained modules of MVC code.

Regarding the duplication of code in the CLI and recipes I suggested alleviating that by adopting a default base recipe so that the (app,model,controller and scaffold) templates can be removed from the CLI but that suggestion was not popular. Furthermore @drujensen api generator is adding more complexity to the CLI rather than slimming it down which I feel is going in the wrong direction. I am sure that every generator that is internal (now and in the future) could be provided by recipes and plugins if there was effort expended in that direction rather than adding to the internal CLI.

[eliasjpr]

Let’s get together to discuss in detail what can we do to implement modules and come to a consensus. They way we always thought of modular applications was more of the Rails Engine way rather than the directory approach.

In regards to the duplication I like the idea of a default recipe and again we just have coke to consensus and agree on an approach.

We can layout all our concerns and then weight what directions should we take. We as a team also should have a clear roadmap so people can contribute with a clear direction

[faustargz]

@damianham I think the recipe idea is very nice (and is already working just fine), although, I think because recipes plugins are not ready yet, we can add an api generator to fullfill our most wanted use-case of generate APIs using amber 😉

Also recipes still need to be upgraded to Boostrap 4, so we can keep amber cli templates 👍

Maybe we should move amber cli templates to the default recipe in the future, this can reduce amber repo size and allow us to compile amber cli faster.

Currently all ECR templates all included within amber binary, this is bad, because makes amber compilation
very very slow, since ECR involves macros and macros in crystal are incredibly slow 😓

Using recipes and interpreted liquid templates should fix this issue 👍

[damianham]

Just to add some more reason d'etre to this discussion, I am currently engaged on a Ruby on Rails contract that has 254 controllers, 335 models, 888 views, 223 shared logic lib files and a 973 line routes file. I ever there was a case for modularisation it is this, Because they started out with the Rails paradigm it has grown into an unwieldy monolith. There are many namespaces but navigation around the codebase involves lots of scrolling all the time. Rails is great for many projects - but when a project grows to a considerable size the Rails code organisation becomes unwieldy. The principle argument against modular by default is that devs coming from Rails will find the organisation familiar. I had absolutely no trouble adapting to MEAN.js modular organisation when I worked on my first MEAN.js project. If a developer cannot understand the concept that the model, controller, views, logic and routes (along with the javascript) for a feature/module are all located in the same folder then they should be doing something else. What would be perfect for me would be to also have the tests in a subfolder of the same module folder.

[eliasjpr]

I have suggested some small changes. Also it would be great to have tests for this, specially around namespaced module and non namespaced modules. It should probably raise errors when a module is not namespace

[drujensen]

@damianham what is the status of this? Should we close it?

[drujensen]

Closing this in favor of recipes being installed as a shard so the path will be lib