Remove legacy conditions #510
Conversation
lib/sprockets/railtie.rb
Outdated
| class Engine < Railtie | ||
| # 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| | ||
| app.config.assets.paths.unshift(*paths["vendor/assets"].existent_directories) | ||
| app.config.assets.paths.unshift(*paths["lib/assets"].existent_directories) | ||
| app.config.assets.paths.unshift(*paths["app/assets"].existent_directories) | ||
| end | ||
| initializer :append_assets_path, :group => :all do |app| | ||
| app.config.assets.paths.unshift(*paths["vendor/assets"].existent_directories) | ||
| app.config.assets.paths.unshift(*paths["lib/assets"].existent_directories) | ||
| app.config.assets.paths.unshift(*paths["app/assets"].existent_directories) | ||
| end |
There was a problem hiding this comment.
Could Sprockets::Railtie be upgraded to a Rails::Engine so that we don't have to reopen the Rails::Engine class? (Rails::Railtie doesn't have access to the paths used here)
There was a problem hiding this comment.
I think we can:
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.
We can't do that. This needs to be an initializer defined in all Rails::Engine subclasses, not only in the sprockets Engine. But we can do the same as rails/propshaft#43.
There was a problem hiding this comment.
Hmm, can you elaborate on the difference? I thought rails/propshaft@66ce0f8 didn't work because Propshaft::Railtie isn't a full engine.
There was a problem hiding this comment.
The difference is that this code is changing all engines (every single one of them) and defining an initializer insider then. If we just move that initializer to the Sprockets-Rails Engine, only it would be affected.
There was a problem hiding this comment.
Ah, it just clicked for me: the initializer is appending the Rails::Engine subclasses' paths to config.assets.paths. That makes complete sense now.
| end | ||
| alias_method_chain :internal?, :sprockets | ||
| end | ||
| end |
There was a problem hiding this comment.
So this can be removed because we no longer call include Sprockets::Rails::RouteWrapper ?
It was removed from Rails in rails/rails@5172d93, and sprockets-rails only supports Rails 5.2+ The initializer is also changed to be a simpler class method call instead of reopening Rails::Engine, similarly to how it was changed in rails/propshaft@c7d5542
Module#prepend was added in ruby 2.0, and the required ruby version is 2.5+ The Rails > 4 conditional will also always be true since the required Rails version is 5.2+
Kernel#caller_locations was added in ruby 2, and the required ruby version is 2.5 The Rails version check will also always be true since the minimum Rails version is 5.2
skipkayhil
force-pushed
the
rm-legacy-conditions
branch
from
83d858f
to
05e40aa
Compare
Summary
These are no longer necessary to due higher Ruby and Rails requirements