Set asset finder from callable #154
Conversation
| @@ -51,7 +59,6 @@ def asset_finder=(finder) | |||
| # See: https://github.com/jamesmartin/inline_svg/issues/25 | |||
| InlineSvg::StaticAssetFinder | |||
| end | |||
| asset_finder | |||
There was a problem hiding this comment.
Return values are ignored in setter methods.
| if config.asset_finder.nil? | ||
| # In default Rails apps, this will be a fully operational | ||
| # Sprockets::Environment instance | ||
| config.asset_finder = app.instance_variable_get(:@assets) |
There was a problem hiding this comment.
Is there a reason not to call the #assets method here?
There was a problem hiding this comment.
This was to deal with older versions of Rails (probably back to 3.x) when these things did not all quack like ducks. May not be necessary with the supported versions of Rails.
There was a problem hiding this comment.
I replaced it with a method call.
It fixes jamesmartin#151 by allowing the asset finder to be a callable object. This is useful when the asset pipeline is not ready when the initializer is run, for example, when using Propshaft. I don't know if this is the best way to solve this problem. On the one hand, Propshaft could be done when its initializer is run, but on the other hand, this gem's railtie assumes the asset pipeline is ready.
There was a problem hiding this comment.
Thanks for the PR. ✨
I don't know if this is the best way to solve this problem. On the one hand, Propshaft could be done when its initializer is run, but on the other hand, this gem's railtie assumes the asset pipeline is ready
I'm not sure either. Assuming the asset pipeline is ready has been reliable until now. I don't have a better suggestion to solve this problem right now. 🤷
| if config.asset_finder.nil? | ||
| # In default Rails apps, this will be a fully operational | ||
| # Sprockets::Environment instance | ||
| config.asset_finder = app.instance_variable_get(:@assets) |
There was a problem hiding this comment.
This was to deal with older versions of Rails (probably back to 3.x) when these things did not all quack like ducks. May not be necessary with the supported versions of Rails.
| @@ -81,6 +88,11 @@ def incompatible_transformation?(klass) | |||
| !klass.is_a?(Class) || !klass.respond_to?(:create_with_value) || !klass.instance_methods.include?(:transform) | |||
| end | |||
|
|
|||
| def set_asset_finder_from_callable | |||
| while @asset_finder&.respond_to?(:call) | |||
There was a problem hiding this comment.
What's the significance of the while here? 🤔
There was a problem hiding this comment.
To handle a callable returning a callable. I can replace it with a conditional only to call the object once or add a spec to describe the intention.
Co-authored-by: James Martin <jamesmartin@users.noreply.github.com>
|
Hi guys! # config/initializers/inline_svg.rb
InlineSvg.configure do |config|
config.asset_finder = InlineSvg::PropshaftAssetFinder
end |
@yshmarov So, this branch does not work with your app? Did you change the order in your Gemfile, as mentioned in #151? I have a Propshaft app working with this change. Still, I'm unconvinced by the initializer approach and would rather have a set of strategies to detect the asset pipeline lazily. I'd be happy to explore that. |
|
@jamesmartin I drafted a more flexible approach in #155, which still needs to be completed, where we don't rely on the initializer and lazily detect which asset finder adapter matches the environment unless the user sets one explicitly. I can clean up and mark either one as ready for review, depending on your preference. |
|
Thanks for continuing to push this forward. I haven't had the chance to properly think about the implications of this change. I'll try to get back to you soon. |
It fixes #151 by allowing the asset finder to be a callable object. This is useful when the asset pipeline is not ready when the initializer is run, for example, when using Propshaft.
I don't know if this is the best way to solve this problem. On the one hand, Propshaft could be done when its initializer is run, but on the other hand, this gem's railtie assumes the asset pipeline is ready.