Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Move controllers and concerns under app/controllers and app/concerns #2906

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

andreychernih commented Jan 28, 2014

In order to get files automatically reloaded in Rails, these files should be under ActiveSupport::Dependencies.autoload_paths directory. Rails::Engine adds app/controllers path automatically to autoload_paths.

Reference: rails/rails#12195 (comment)

Closes #697

@andreychernih andreychernih Move controllers and concerns under app/controllers and app/concerns
In order to get files automatically reloaded in Rails, these files should be under `ActiveSupport::Dependencies.autoload_paths` directory. `Rails::Engine` adds `app/controllers` path automatically to `autoload_paths`.

Reference: rails/rails#12195 (comment)

Closes #697
9c4634b
Contributor

andreychernih commented Mar 16, 2014

@seanlinsley any issues with this PR?

Contributor

andreychernih commented May 29, 2014

@seanlinsley sorry for being pushy, but I really would love to hear back 😄 This caching problem still bothers me...

I'm not a user of active_admin and I see @andreychernih has put a lot of work into this.

The amount of code moved might be scary, so I'm thinking what if lib was added to autoload_paths? Then you might be reduce the amount of code that you have to shuffle around?

With lib in autoload_paths you would just need to remove any requires or autoloads for the controller classes and use require_dependency for some "inner" modules (such as in lib/active_admin/base_controller.rb you'd replace require 'active_admin/base_controller/authorization' with require_dependency 'active_admin/base_controller/authorization', require_dependency is require that works with auto-loadable constants). Removing require's and autoload's is necessary because we need Rails to magically load the files for us.

I've done some poking and prodding and here's what I have: thedarkone/active_admin@2086cae.

Unfortunately I don't really have the time to work further on this, so @andreychernih if you want pick up on the autoload_paths += 'lib' approach, feel free to do so! I haven't run the tests or did anything but the really basic testing, maybe investigate why your approach doesn't require changes to register_page_controller and register_resource_controller, they are after-all creating new controllers that inherit from ActiveAdmin::ResourceController and if they are cached in Controllers#controller then they will not be picking up changes from ApplicationController etc.

Owner

timoschilling commented Nov 5, 2014

The idea behind this PR is good, but at the moment we have to many other problems in active admin with the autoloading / code reloading.

And there are some problems explicit with this PR:

  • (activeadmin)/app/controllers is not in the autoload_paths
  • renaming and moving of the controllers will brake things on the app side

This is way I'm going to close this, but I will create a issue which address this problem and add them to a 2.0 millstone.

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