Skip to content

Engine resource path helpers #5120

Closed
wants to merge 1 commit into from

5 participants

@bcardarella

Please see my write up on http://reefpoints.dockyard.com/ruby/2012/02/20/rails-engines-and-monkey-patching.html that goes into the advantage of this

@josevalim
Ruby on Rails member

Thanks for the pull request, I like the general idea. But we have some topics to discuss before we get to the final implementation. A few things:

1) We should not hardcode to app/models. In fact, all engines have a config.paths["app/models"] that returns an array with model related paths. We should probably rely on this;

2) This is not related to your patch but your blog post. I believe it would be better for you to use require_dependency instead of require. Just in case to allow it to play well with development;

That said, what do you think about having a method called require_dependency on the engine?

MyEngine::Engine.require_dependency "invitation"

This would loop through all engine load paths (they are available here), find the file that matches and require it. I believe there is already methods in AS that can do the looping for us, but if not, it should be trivial to add it.

@bcardarella

@josevalim sounds good, let me take a stab at your suggestions and submit an update

@josevalim
Ruby on Rails member

@bcardarella please submit a pull request then? I am just trying to expand our options, I am not sure require_dependency is THE way to go. :)

@bcardarella

@josevalim Actually a few questions:

  1. Are you suggesting using Rails::Engine.config['app'] as the base path or instead iterating through each of the keys and providing dependency helpers for the paths in each value?

  2. I am OK with the require_dependency idiom, I'm just wondering if there is a use case to get the absolute path of the different app resources outside of requiring? In that case wouldn't it make sense to just provide the paths and rely upon the user to explicitly require instead?

@bcardarella

I like the require_dependency idea and I think it is the most clean. I'm going to head down that route.

@josevalim
Ruby on Rails member

1) It depends. If you are considering this pull request, all I am saying is that to get the real models path, we need to get the directory from config.paths["app/models"].first, get config.paths["app/controllers"].first for controllers and so on.

If you are considering the require_dependency pull request, we don't need to loop config.paths, as all load paths are already in the method I linked above.

2) Yes, someone may want to get the absolute path for other reason, we can expose a method for that. But I prefer require_dependency exactly because it reminds developers to use require_dependency and not require. Using require may eventually lead to reloading headaches in development.

@josevalim
Ruby on Rails member

Btw, I was wrong above. MyEngine.require_dependency should loop all autoload paths:

https://github.com/rails/rails/blob/master/railties/lib/rails/engine.rb#L649

What is in the load path only (which is usually lib files) should be required via a normal require.

@bcardarella

@josevalim updated with require_dependency implemented. Please let me know if there are any issues

@jeremy
Ruby on Rails member
jeremy commented Feb 21, 2012

I do like having easy access to paths like this, but this subverts the ruby load path big time.

If you namespace your directories and classes, a plain old require 'invitable/invitation' would do the job here.

Feels like this is the pattern we should be encouraging instead of absolute paths to work around load path collisions.

@bcardarella

@jeremy did you check out the use case that I quoted in my blog post above?

@jeremy
Ruby on Rails member
jeremy commented Feb 22, 2012

@bcardarella that's the use case that I take issue with. Here we have two 'invitable/invitation' in the load path. One is the "app" model, a specialization of the engine model. We want autoload to load the app model first, yet still be able to load the underlying engine model.

Coming up with a way around the load path is one answer here. But - now you have two problems.

Some other ways to do this:
1. Mix behavior in to engine modules using an initializer that requires your extensions, probably from lib/
2. Subclass engine models in your app and use these instead.
3. Start with app models, always, and rely on engines for behavior mix-ins instead of fully-baked classes.

In the Invitable::Invitation case you cite, it feels like #3 would be a good candidate: keep Invitation in your app and mix in the Invitable concern. Then customize your app's model as you like.

@bcardarella

How about Rails::Engine.load_dependency instead of require_dependency ? It will of course use Kernel#load instead of Kernel#require

@josevalim
Ruby on Rails member

require_dependency doesn't actually use require. It uses require or load depending on your environment.

@bcardarella

Ok, then I should update my pull request to act accordingly.

@bcardarella

@josevalim ok, updated to conditionally use load or require depending upon the state of Rails.configuration.cache_classes

@bcardarella bcardarella Rails::Engine.require_dependency
Allows the require of depdenecies in a Rails engine

Will load or require depending if the Rails configuration
is caching classes or not
b844276
@bcardarella

@josevalim is this still up for consideration?

@dpickett

I agree with @jeremy on this one. Having implemented an engine in a way similar to what this PR is advocating, I really wish I had namespaced everything and went with option 3 as @jeremy suggested

@bcardarella

@dpickett the downside for that method is if you want to extend any models you need to copy them entirely into your app. Which becomes a slippery slope as you continue to want to modify engine code. Eventually you end up with the almost the entire app in your engine and relying upon an external gem is pointless. (no benefit from API/bug fix updates)

@bcardarella

@dpickett the real issue here is when Rails does not cache the classes, but I have accommodated for that by conditionally using load instead of require when necessary.

@woodhull

Your original blog post link returns 404.

@bcardarella

@woodhull Github is being with gh-pages, here is the raw file: https://github.com/dockyard/reefpoints/blob/gh-pages/_posts/2012-02-20-rails-engines-and-monkey-patching.md

The usecase is the same in the blog post, but the method is different from what I have in this PR.

@dpickett

yeah maybe it's more I felt like I was "doing it wrong" when I didn't have namespaced classes in my engines. I totally agree with the problem you're describing, in that you end up having to rely on generators, which is contrary to the whole idea of "mountable apps"

I feel like there needs to be a way or option to toggle engine specific concerns in the root namespace or in it's own namespace. When I need to extend, I wish they were namespaced, when I need to just plug-in, I don't want to generate all of the concerns.

@josevalim
Ruby on Rails member

I don't consider this pull request to be the way to build engines. Yes, you should namespace and build engines, and use configuration, inheritance, whatever you fancy.

But then there is real life and sometimes you want to monkey patch an engine or you need to develop against an engine that does not provide the proper abstraction, that's when this pull request comes in hand. It is simply a way to monkey patch engines that will also work with development.

@jeremy
Ruby on Rails member
jeremy commented Mar 25, 2012

@josevalim And these cases deserve a heavy dose of vinegar, not sugar. After all, it's still possible to circumvent the load path by using MyEngine.paths directly.

@josevalim
Ruby on Rails member

@jeremy I will bring DHH here to do his freedom patch 🇺🇸 speech :trollface: Anyway, using MyEngine.paths is fine. A bit verbose, but fine.

@josevalim josevalim closed this Mar 25, 2012
@bcardarella

While MyEngine.path gets you the paths, it does not address the issue of development mode, which my PR does.

@josevalim
Ruby on Rails member

@bcardarella you could handle this in development as:

require_dependency File.join(MyLib::Engine.paths["app/models"].first, "user")

I like the sugar introduced by this patch but as I believe we won't reach a consensus, I have closed this issue.

@bcardarella

Fair enough, thank you for the feedback :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.