-
Notifications
You must be signed in to change notification settings - Fork 527
-
Notifications
You must be signed in to change notification settings - Fork 527
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
#decorator method doesn't work properly with cache_classes = false #32
Comments
Ignore this. I was loading something incorrectly. -Edit Actually ignore me saying 'ignore this'. This is in fact a problem with |
I see what you're saying here and am observing the same in my sample app. The load-order catch is that the gem is loaded before the application models, so there's no easy moment to tell the app to "go ahead and include this module into various models" unless we setup an initializer. But I'm thinking that will be necessary, anyway. It would help with this issue and with some challenges around the |
Actually, let me nix what I said there. It looks like all other important issues are handled without an initializer. I'll reach out to @paulelliott for ideas. The problem in summary: When Rails is in development mode and However, since the decorator has not yet been loaded, the If you first create some instance of the decorator, causing the class to be loaded, then the above works just fine. |
We have been dealing with this same issue and I haven't decided on an appropriate course of action yet. I am leaning towards having a method on the model class that allows you to specify what it is decorated by. I feel like there is a larger architectural issue at hand though. To get through the day, you can just reference the Decorator class at the bottom of your model file. I know it's gross, but it won't negatively impact performance and dev will work fine. Like so... class MyModel
end
MyModelDecorator |
@paulelliott I thought about doing it that way and yeah, I agree it's kinda gross. What I decided on instead was to just include the Although you could argue you're still doing it specifically for development. :) |
That appears cleaner but you are relying on a non-public class from draper. Either way has its drawbacks. In any event, this needs to be solved by the gem so that none of this is necessary. |
Just throwing a guess out here, but couldn't a change to the System.setup method like this work: module Draper
class System
def self.setup
ActionController::Base.send(:include, Draper::ViewContextFilter) if defined?(ActionController::Base)
Rails::Applications.config.after_initialize Draper::RailsInitializer.load if defined?(Rails)
end
end
end The I haven't tested this theory... and it kinda seems tacky. |
I've had a similar problem with RoleUp, a young gem backed by CanCan that allows you to create and segment ability definitions in a Rails app in a logical way. The solution I came up with is to do certain things on application boot and on each request. Here's the relevant code: https://github.com/quickleft/role_up/blob/master/lib/role_up/railtie.rb # FILE :: lib/role_up/railtie.rb
require "rails/railtie"
module RoleUp
class Railtie < Rails::Railtie
# eager load our app/abilities directory
#
# HAPPENS FIRST
config.before_configuration do |app|
app.config.paths.app.abilities "app/abilities" , :eager_load => true , :glob => RoleUp.glob
end
# make sure our ability files are reloaded on each request in development
# ONLY if we are not caching classes
#
# HAPPENS SECOND
config.to_prepare do
RoleUp::Railtie.force_load_abilities if Rails.env.development? && !Rails.application.config.cache_classes
end
# force load our ability files if we are in a non-development environment and
# if we aren't caching classes
#
# HAPPENS THIRD
config.after_initialize do |app|
force_load_abilities if !Rails.env.development? && !app.config.cache_classes
end
def self.force_load_abilities
Dir[ "#{ Rails.root }/app/abilities/#{ RoleUp.glob }" ].each { |a| load a }
end
end
end I don't like the logic that's being depended on necessarily, but overall it works in development where non-referenced constants are also reloaded upon each request. The above also works in non-development environments where the files are loaded once at boot. Ideally ActiveSupport's code reloading facilities would/could be utilized. I had a short but great discussion with @rkh about this and his RSoC essay on Rails code reloading was invaluable. I hope this is relevant or useful! |
I agree with @cookrn on the possible solution for this. Have you considered using a |
I think the best way would be to include the ModelSupport module in ActiveRecord::Base when it's loaded, using a railtie and defining an initializer, like this:
|
@rubymaverick Would that solve the issue? I thought we had to actually execute the |
@amerine Well, no I guess it wouldn't completely solve the issue. The way I'd imagine it could work would be that all ActiveRecord::Base subclasses would get the #decorator method, but calling #decorator on an model object that didn't have a corresponding decorator class would return nil (or maybe it should raise an error?). The other thing you'd have to account for is the ability to change the decorate class for an AR class, maybe using a AR::Base.decorated_by method. |
+1 to @paulelliot's suggestion for a macro on the model (inverse of the current |
I think it's bound to add some bad effects if the model gets some magic methods by being indirectly referenced from another class. Honestly, having a The kosher, non-magic way to decorate would be to call |
Philosophy of how decorators are related to models aside, loading decorators from
|
This problem still exists, even though it's really old, and Draper has changed a lot. I've created a test repository that you can run I'm still not sure what the right answer is, but test cases are good. And confirming 7 month old issues are good. |
…ther files should be `load`-ed or `require`-ed -- Issue drapergem#32
…paths. Describe the reasoning behind each hook/callback. -- Issue drapergem#32
This was closed by #188 |
EDIT: Haha well it did get closed while I was writing this... Before this issue gets closed, I'd like to +1 the suggestion from @Mange. His strategy could be supported in the future as best practice for users and allows for all of Rails code reloading fu to be used out of the box without the extra code. I also find it to be much more clear and hopefully leading people down the path of avoiding model or database-connected objects in their views at all. |
@cookrn Thanks for the PR, it'll be nice to remove the extra boilerplate code from my app. Much appreciated! |
@steveklabnik Do I think he's right? Yes. Do I think that solution is a good thing for Draper short-term? No. Do I think that solution is a good thing for Draper long-term? Yes. But that's just one opinion on something that is a pretty serious usage consideration for well-used library. My reasoning? It seems a little bit inverted that a model would know about the the thing that decorates it. That might be better as a one-way street. It's a toughie for sure :) -- small code change with a large effect! @jfelchner My pleasure! I hope it works well. |
Right. Well, as Draper approaches 1.0, it's probably going to undergo significant internal change, so I'll consider anything, really. Feel free to submit any patches you have to push it in that direction! Especially this |
Looks like this change causes the decorators and their models to be loaded too early if you use Spork in testing. I'm using this in my spec_helper file to avoid the preloading issue: Spork.trap_class_method(Draper::System, :load_app_local_decorators) |
Ugh, Spork. I hate spork so much... Do you think this is a good general thing? Should this be in draper proper? |
What does it mean that it's "loading too early"? Too early for Spork to properly preload? Or too early in the Rails environment in general? |
Too early in that Spork wont detect changes to the file on test runs until you reload Spork. No issues with Rails at all, just a Spork issue. Given that there's already some debate over whether the current fix for this issue is the right long-term solution, I might hesitate to update Draper proper to support this Spork issue. I just wanted to add a note so when evaluating pros/cons of the current implementation, this was known. Gems preloading files in Spork prematurely is a common problem, Devise suffers a similar issue. In the interim, as Draper approaches 1.0, maybe a simple note with the workarond in the Readme would suffice. |
Cool. I am gonna add it in, but not right this second... let me make another issue to keep track of this, since this is only related to this issue. |
Due to the way the the
#decorator
method is added to the decorated class, whencache_classes
is false, the decorator class isn't loaded at startup and therefore the#decorator
method is never added to the class.This method seems to be in the very beta stages since there's no documentation around it. It's extremely handy though so I hope someone who knows more than me about Rails load order can find a solution.
The text was updated successfully, but these errors were encountered: