varvet/godmin

Look at if extending modules is still preferable to inheriting from classes

Opened this issue · 3 comments

Take a good hard look at if extending modules is still preferable to inheriting from classes. The file resolver has been the cause of much pain.

The reason we have it is because we want to support template and partial overrides. A template placed in views/articles/index.html.erb works as expected. A partial placed in views/articles/_table.html.erb overrides the table for articles, while views/resources/_table.html.erb overrides the table for all resources. This would work out of the box if we inherited instead of included modules, like so:

class ArticlesController < Godmin::ResourcesController; end

The reason we do use modules instead is because of problems with what the above base controller would inherit from. Consider this, which is what gems like Devise and Clearance do:

class Godmin::ResourcesController < ::ApplicationController;

This means all resource controllers would inherit from the main app's application controller, which does not work for engine installs. We basically have the requirement that our gem needs to work in isolation within engines. Consider the following scenario:

admin_one
  controllers
    admin_one
      application_controller.rb
      articles_controller.rb
admin_two
  controllers
    admin_two
      application_controller.rb
      articles_controller.rb
app
  controllers
    application_controller.rb
    articles_controller.rb

AdminOne::ArticlesController needs to inherit from AdminOne::ApplicationController, AdminTwo::ArticlesController needs to inherit from AdminTwo::ApplicationController and
::ArticlesController needs to inherit from ::ApplicationController.

And for this reason, we went with modules, and that way, we do not get template overriding out of the box, which meant we had to implement our own resolver.

The question is, is there a better way to solve this? Do we need the ability to override by placing files in /resources at all? Administrate solves this differently by using a generator: https://administrate-prototype.herokuapp.com/customizing_page_views. Also take into consideration that we're thinking about different ways to do column overrides.

If there would always be a ResourceController, generated by the installer, would that solve basic overriding? We tend to always create such a class anyway because we want shared behaviour for all resource controllers.

I experimented a bit with this and tried the following:

module Godmin
  module Controllers
    def self.resources_controller(parent)
      resources_controller = Class.new(parent) do
        def index; end
      end

      Godmin.const_set("ResourcesController", resources_controller)
    end
  end
end
class ResourcesController < Godmin::Controllers.resources_controller(ApplicationController)
end
require_dependency "admin/application_controller"

module Admin
  class ResourcesController < Godmin::Controllers.resources_controller(ApplicationController)
  end
end

This works for template overriding without the need of a custom resolver. Godmin templates are found by apps and engines, and app templates override the Godmin templates.

As for partial overriding, it's trickier. If we render partials from the same folder, e.g. godmin/resources/index.html.erb wants to render godmin/resources/_table.html.erb, it works:

<%= render "table" %>

However, if we need to render something from another folder, it does not, because Rails goes into absolute path mode:

<%= render "foo/table" %>

This is part of what we solve with the custom resolver today, and it might be simpler to solve with the inheritance solution above. However, perhaps we could just place partials in the same folder and try and come up with a different solution for column and filter overrides, which are the real problems here.

Another alternative would be to skip the resources_controller entirely and move all of that functionality to the application_controller. That would cater more towards the 80% use case and not be as much "just a rails app" as what we have now. It would solve the inheritance problem, however:

ApplicationController < Godmin::ApplicationController < ActionController::Base

We do have an additional sessions_controller now though, which would be a little tricky...