Skip to content
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

Set asset finder from callable #154

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions lib/inline_svg.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,15 @@

require "inline_svg/railtie" if defined?(Rails)
require 'active_support'
require 'active_support/core_ext/object/blank'
xymbol marked this conversation as resolved.
Show resolved Hide resolved
require 'active_support/core_ext/string'
require 'nokogiri'

module InlineSvg
class Configuration
class Invalid < ArgumentError; end

attr_reader :asset_file, :asset_finder, :custom_transformations, :svg_not_found_css_class
attr_reader :asset_file, :custom_transformations, :svg_not_found_css_class

def initialize
@custom_transformations = {}
Expand All @@ -40,8 +41,15 @@ def asset_file=(custom_asset_file)
end
end

def asset_finder
set_asset_finder_from_callable
@asset_finder
end

def asset_finder=(finder)
@asset_finder = if finder.respond_to?(:find_asset)
@asset_finder = if finder.respond_to?(:call)
finder
elsif finder.respond_to?(:find_asset)
finder
xymbol marked this conversation as resolved.
Show resolved Hide resolved
elsif finder.class.name == "Propshaft::Assembly"
InlineSvg::PropshaftAssetFinder
Expand All @@ -51,7 +59,6 @@ def asset_finder=(finder)
# See: https://github.com/jamesmartin/inline_svg/issues/25
InlineSvg::StaticAssetFinder
end
asset_finder
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return values are ignored in setter methods.

end

def svg_not_found_css_class=(css_class)
Expand Down Expand Up @@ -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)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the significance of the while here? 🤔

Copy link
Contributor Author

@xymbol xymbol Oct 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an example.

self.asset_finder = @asset_finder.call
end
end
end

@configuration = InlineSvg::Configuration.new
Expand Down
14 changes: 6 additions & 8 deletions lib/inline_svg/railtie.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,12 @@ class Railtie < ::Rails::Railtie

config.after_initialize do |app|
InlineSvg.configure do |config|
# Configure the asset_finder:
# Only set this when a user-configured asset finder has not been
# configured already.
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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason not to call the #assets method here?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replaced it with a method call.

end
# Configure an asset finder for Rails. This will be evaluated when the
# first SVG is rendered, giving time to the asset pipeline to be done
# loading.
config.asset_finder = proc {
app.instance_variable_get(:@assets)
}
end
end
end
Expand Down
16 changes: 15 additions & 1 deletion spec/inline_svg_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ def self.named(filename); end

describe InlineSvg do
describe "configuration" do
before do
InlineSvg.reset_configuration!
end

context "when a block is not given" do
it "complains" do
expect do
Expand All @@ -29,14 +33,24 @@ def self.named(filename); end

context "asset finder" do
it "allows an asset finder to be assigned" do
sprockets = double('SomethingLikeSprockets', find_asset: 'some asset')
sprockets = double("Something like sprockets", find_asset: "some asset")
InlineSvg.configure do |config|
config.asset_finder = sprockets
end

expect(InlineSvg.configuration.asset_finder).to eq sprockets
end

it "allows to give a callable object that returns an asset finder" do
propshaft = double("Something like propshaft", class: double(name: "Propshaft::Assembly"))
callable = -> { propshaft }
InlineSvg.configure do |config|
config.asset_finder = callable
end

expect(InlineSvg.configuration.asset_finder).to eq InlineSvg::PropshaftAssetFinder
end

it "falls back to StaticAssetFinder when the provided asset finder does not implement #find_asset" do
InlineSvg.configure do |config|
config.asset_finder = 'Not a real asset finder'
Expand Down