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

Fix css loading and add logging #140

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 35 additions & 8 deletions lib/premailer/rails/css_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,21 @@ module Rails
module CSSHelper
extend self

@cache = {}
attr :cache

STRATEGIES = [
CSSLoaders::CacheLoader,
CSSLoaders::FileSystemLoader,
CSSLoaders::AssetPipelineLoader,
CSSLoaders::NetworkLoader
CSSLoaders::NetworkLoader,
Copy link

Choose a reason for hiding this comment

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

comma left.

]

def reset_cache!
@cache = {}
end

def cache
reset_cache! if @cache.nil?
@cache
end

Copy link
Author

Choose a reason for hiding this comment

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

Cache was spread through two different places, the old CacheLoader and this module, which kinda breaks encapsulation. Now cache is fully implemented here and a little bit more robust.

# Returns all linked CSS files concatenated as string.
def css_for_doc(doc)
urls = css_urls_in_doc(doc)
Expand All @@ -21,18 +26,40 @@ def css_for_doc(doc)

private

def lookup_cached(url)
return yield if rails_dev_env?

if !cache.has_key?(url)
cache[url] = yield(url)
end

cache[url]
end

def rails_dev_env?
return false
defined?(::Rails) && ::Rails.env.development?
end

def css_urls_in_doc(doc)
doc.search('link[@rel="stylesheet"]').map do |link|
link.attributes['href'].to_s
end
end

def load_css(url)
@cache[url] =
lookup_cached(url) do |url|
css = nil

STRATEGIES.each do |strategy|
css = strategy.load(url)
break css if css
if css = strategy.load(url)
::Rails.logger.debug "premailer-rails: loaded asset #{url} using #{strategy}" if defined?(::Rails)
break
end
end

css
end
end
end
end
Expand Down
1 change: 0 additions & 1 deletion lib/premailer/rails/css_loaders.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
require 'uri'

require 'premailer/rails/css_loaders/cache_loader'
require 'premailer/rails/css_loaders/file_system_loader'
require 'premailer/rails/css_loaders/asset_pipeline_loader'
require 'premailer/rails/css_loaders/network_loader'
6 changes: 4 additions & 2 deletions lib/premailer/rails/css_loaders/asset_pipeline_loader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@ module AssetPipelineLoader
def load(url)
if asset_pipeline_present?
file = file_name(url)
asset = ::Rails.application.assets.find_asset(file)
asset.to_s if asset

if asset = ::Rails.application.assets.find_asset(file)
asset.to_s
end
end
end

Expand Down
19 changes: 0 additions & 19 deletions lib/premailer/rails/css_loaders/cache_loader.rb

This file was deleted.

11 changes: 9 additions & 2 deletions lib/premailer/rails/css_loaders/file_system_loader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,15 @@ module FileSystemLoader

def load(url)
path = URI(url).path
file_path = "public#{path}"
File.read(file_path) if File.exist?(file_path)

# Remove leading slash if it exists
path = path[1..-1] if path[0,1] == "/"

file_path = ::Rails.root.join("public/#{path}").to_s
Copy link
Author

Choose a reason for hiding this comment

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

The path needs to be relative to Rails root and not the current working directory. This wasn't working on some environments where the process is started outside of the app folder.


if File.exist?(file_path)
File.read(file_path)
end
end
end
end
Expand Down
5 changes: 3 additions & 2 deletions lib/premailer/rails/css_loaders/network_loader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@ module NetworkLoader
extend self

def load(url)
uri = uri_for_url(url)
Net::HTTP.get(uri) if uri
if uri = uri_for_url(url)
Net::HTTP.get(uri)
end
end

def uri_for_url(url)
Expand Down
2 changes: 1 addition & 1 deletion lib/premailer/rails/customized_premailer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ def initialize(html)
Premailer.send(:include, Adapter.find(Adapter.use))
doc = load_html(html)

options = @options.merge(css_string: CSSHelper.css_for_doc(doc))
options = @options.merge(include_link_tags: false, css_string: CSSHelper.css_for_doc(doc))
Copy link
Author

Choose a reason for hiding this comment

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

Since we are manually loading and embedding the css, we need to make sure premailer skips the link tags, otherwise it will load the assets through the network redundantly.

super(html, options)
end
end
Expand Down
8 changes: 3 additions & 5 deletions spec/integration/css_helper_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
describe Premailer::Rails::CSSHelper do
# Reset the CSS cache:
after do
Premailer::Rails::CSSHelper.send(:instance_variable_set, '@cache', {})
Premailer::Rails::CSSHelper.reset_cache!
end

def load_css(path)
Expand Down Expand Up @@ -59,8 +59,7 @@ def expect_file(path, content='file content')

context 'when file is cached' do
it 'returns the cached value' do
cache =
Premailer::Rails::CSSHelper.send(:instance_variable_get, '@cache')
cache = Premailer::Rails::CSSHelper.cache
cache['http://example.com/stylesheets/base.css'] = 'content of base.css'

expect(load_css('http://example.com/stylesheets/base.css')).to \
Expand All @@ -70,8 +69,7 @@ def expect_file(path, content='file content')

context 'when in development mode' do
it 'does not return cached values' do
cache =
Premailer::Rails::CSSHelper.send(:instance_variable_get, '@cache')
cache = Premailer::Rails::CSSHelper.cache
cache['http://example.com/stylesheets/base.css'] =
'cached content of base.css'
content = 'new content of base.css'
Expand Down
8 changes: 8 additions & 0 deletions spec/support/stubs/rails.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
module Rails
extend self

def root
@root ||= Pathname.new(".")
end

def logger
@logger = Logger.new(STDOUT)
end

module Configuration
extend self
end
Expand Down