-
Notifications
You must be signed in to change notification settings - Fork 247
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
Import append_assets_path initializer #215
Conversation
@@ -36,6 +36,17 @@ def assets | |||
# Returns Sprockets::Manifest for app config. | |||
attr_accessor :assets_manifest | |||
end | |||
|
|||
class Engine < Railtie |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is extending initializers on Engine
allowed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is, although it is not the best way of doing it.
Since we want to detect if the initializer was already defined I think we can't do better.
When we drop support to Rails 4 we can change it to instead of reopening Rails::Engine
just define a Sprockets::Rails::Engine
instead of a Railtie
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we drop support to Rails 4 we can change it to instead of reopening Rails::Engine just define a Sprockets::Rails::Engine instead of a Railtie.
Ah, cool, I didn't realize that would work.
# Skip defining append_assets_path on Rails <= 4.2 | ||
unless initializers.find { |init| init.name == :append_assets_path } | ||
initializer :append_assets_path, :group => :all do |app| | ||
if paths["app/assets"].respond_to?(:existent_directories) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why bother with versions < 4 if the initializer is defined and we will not enter this code? It is just because Rails 3.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:/ yeah, I just wanted to make the tests pass. This is merging into the old 2.x. I'll drop this condition when pulling this patch into master.
Import append_assets_path initializer
Alright, @rafaelfranca
Heres the idea for moving the
append_assets_path
into this plugin.Then in rails/master we could completely remove
config.assets
and this guy. This code would still cover people running Rails 4.2. And eventually we can remove the guard when we only need to support Rails 5.Related discussion