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

Dependency aware asset digests #175

Closed
wants to merge 3 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion lib/propshaft/assembly.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
require "propshaft/compilers"
require "propshaft/compiler/css_asset_urls"
require "propshaft/compiler/source_mapping_urls"
require "propshaft/dependency_tree"

class Propshaft::Assembly
attr_reader :config
Expand All @@ -15,7 +16,7 @@ def initialize(config)
end

def load_path
@load_path ||= Propshaft::LoadPath.new(config.paths, version: config.version)
@load_path ||= Propshaft::LoadPath.new(config.paths, version: config.version, dependency_tree: dependency_tree)
end

def resolver
Expand All @@ -26,6 +27,10 @@ def resolver
end
end

def dependency_tree
Propshaft::DependencyTree.new(compilers: compilers)
end
Comment on lines +30 to +32
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any specific reason to have this exposed as a public method? I understand it makes the load_path method more legible, but I suggest this is made private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No specific reason; that was done to copy the style of the rest of the methods.


def server
Propshaft::Server.new(self)
end
Expand Down
20 changes: 19 additions & 1 deletion lib/propshaft/asset.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,12 @@

class Propshaft::Asset
attr_reader :path, :logical_path, :version
attr_accessor :dependencies, :dependency_fingerprint

def initialize(path, logical_path:, version: nil)
@path, @logical_path, @version = path, Pathname.new(logical_path), version
@dependencies = nil
@dependency_fingerprint = nil
end

def content
Expand All @@ -16,12 +19,27 @@ def content_type
Mime::Type.lookup_by_extension(logical_path.extname.from(1))
end

def may_depend?
content_type&.symbol == :css
end

def length
content.size
end

# A dependency aware digest can be calculated for any asset that has no dependencies or
# that has had its dependency fingerprint set by DependencyTree.
# If it's too soon, we can still calculate the digest based on file contents,
# but there won't be any dependency cache busting.
def digest_too_soon?
may_depend? && dependencies&.any? && !dependency_fingerprint
end

def digest
@digest ||= Digest::SHA1.hexdigest("#{content}#{version}").first(8)
@digest ||= begin
Propshaft.logger.warn("digest dependencies not ready for #{logical_path}") if digest_too_soon?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe I'm missing something obvious, but in what scenario would an asset have dependencies but no dependency fingerprint? It's just not clear to me what should be done by users in response to this warning, so either failing more loudly (and clearly) or not warning at all seems like better options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That should only happen if there was a bug in the dependency tree logic. Since digest bugs are insidious, I decided a warning was necessary, but going to the extent of failing a build for inadequate cache busting seemed a little much.

Digest::SHA1.hexdigest("#{content}#{version}#{dependency_fingerprint}").first(8)
end
end

def digested_path
Expand Down
4 changes: 4 additions & 0 deletions lib/propshaft/compiler.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ def compile(logical_path, input)
raise NotImplementedError
end

def find_dependencies(logical_path, input)
Set.new
end

private
def url_prefix
@url_prefix ||= File.join(assembly.config.relative_url_root.to_s, assembly.config.prefix.to_s).chomp("/")
Expand Down
8 changes: 8 additions & 0 deletions lib/propshaft/compiler/css_asset_urls.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,14 @@ def compile(logical_path, input)
input.gsub(ASSET_URL_PATTERN) { asset_url resolve_path(logical_path.dirname, $1), logical_path, $2, $1 }
end

def find_dependencies(logical_path, input)
Set.new.tap do |deps|
input.scan(ASSET_URL_PATTERN).each do |fn, _|
deps << resolve_path(logical_path.dirname, fn)
end
end
end

private
def resolve_path(directory, filename)
if filename.start_with?("../")
Expand Down
10 changes: 10 additions & 0 deletions lib/propshaft/compilers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,14 @@ def compile(asset)
asset.content
end
end

def find_dependencies(asset)
Set.new.tap do |deps|
if relevant_registrations = registrations[asset.content_type.to_s]
relevant_registrations.each do |compiler|
deps.merge compiler.new(assembly).find_dependencies(asset.logical_path, asset.content)
end
end
end
end
end
61 changes: 61 additions & 0 deletions lib/propshaft/dependency_tree.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
class Propshaft::DependencyTree

def initialize(compilers:)
@compilers = compilers
end

# a single pass through the dependent assets, calculating all the dependency
# fingerprints we can.
def dependency_fingerprint_pass(dependent_assets)
dependent_assets.each do |asset|
# the fingerprint can be calculated unless a dependency of this asset
# is not ready yet.
next if asset.dependencies.detect{|da| da.digest_too_soon?}
# ok, ready to set the fingerprint, which is the concatenation of the
# digests of each dependent asset.
# the fingerprints need to maintain a stable order, done via the sort.
asset.dependency_fingerprint = asset.dependencies.map(&:digest).sort.join
end
# clear out any ones we are done with.
dependent_assets.delete_if{|asset| asset.dependency_fingerprint.present?}
end

# After we know the assets that depend on other propshaft assets, we can iterate
# through the list, calculating the dependency fingerprint for any asset with
# children with known digests. Once we run out of dependent assets without
# digests, we are done. But if we notice that we aren't making any progress on
# an iteration, it means there is an asset dependency cycle. In that case we
# bail out and warn.
def set_dependency_fingerprints(dependent_assets)
# There will be N iterations where N is the depth of the longest dependency chain.
loop do
initial_count = dependent_assets.size
dependency_fingerprint_pass(dependent_assets)
break if dependent_assets.empty? # success, all done
if dependent_assets.size == initial_count
# failing to make progress
cyclic_assets = dependent_assets.map{|a| a.logical_path.to_s}.join(', ')
Propshaft.logger.warn "Dependency cycle between #{cyclic_assets}"
break
end
end
end

def traverse(mapped)
dependent_assets = Set.new
mapped.each_pair do |path, asset|
next unless asset.may_depend? # skip static asset types
# get logical paths of dependencies
deps = @compilers.find_dependencies(asset)
# convert logical paths to asset objects
# asset references that aren't managed by propshaft will not be in the mapping
# and can be ignored since they won't have digests
asset.dependencies = deps.map{|d| mapped[d]}.compact
# If no dependencies, the normal digest will be fine for this asset.
# Otherwise we will need to perform dependency tree traversal to
# create dependency-aware digests. Keep a list of such assets to visit.
dependent_assets << asset if asset.dependencies.any?
end
set_dependency_fingerprints(dependent_assets)
end
end
8 changes: 5 additions & 3 deletions lib/propshaft/load_path.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@
class Propshaft::LoadPath
attr_reader :paths, :version

def initialize(paths = [], version: nil)
@paths = dedup(paths)
@version = version
def initialize(paths = [], version: nil, dependency_tree: nil)
@paths = dedup(paths)
@version = version
@dependency_tree = dependency_tree
end

def find(asset_name)
Expand Down Expand Up @@ -51,6 +52,7 @@ def assets_by_path
mapped[logical_path.to_s] ||= Propshaft::Asset.new(file, logical_path: logical_path, version: version)
end if path.exist?
end
@dependency_tree&.traverse(mapped)
end
end

Expand Down
1 change: 1 addition & 0 deletions test/fixtures/assets/dependent/child1.css
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
@import url('child2.css');
1 change: 1 addition & 0 deletions test/fixtures/assets/dependent/child2.css
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
p { background: blue; }
1 change: 1 addition & 0 deletions test/fixtures/assets/dependent/cyclic1.css
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
@import url('cyclic2.css');
1 change: 1 addition & 0 deletions test/fixtures/assets/dependent/cyclic2.css
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
@import url('cyclic1.css');
1 change: 1 addition & 0 deletions test/fixtures/assets/dependent/main.css
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
@import url('child1.css');
42 changes: 42 additions & 0 deletions test/propshaft/dependency_tree_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
require "test_helper"
require "minitest/mock"
require "propshaft/asset"
require "propshaft/assembly"
require "propshaft/compilers"

class Propshaft::DependencyTreeTest < ActiveSupport::TestCase
setup do
@assembly = Propshaft::Assembly.new(ActiveSupport::OrderedOptions.new.tap { |config|
config.paths = [ Pathname.new("#{__dir__}/../fixtures/assets/dependent") ]
config.output_path = Pathname.new("#{__dir__}/../fixtures/output")
config.prefix = "/assets"
})
@assembly.compilers.register "text/css", Propshaft::Compiler::CssAssetUrls
end

test "cyclic assets do not cause a loop" do
@assembly.load_path.assets
end

def write_child2(col)
File.open("#{__dir__}/../fixtures/assets/dependent/child2.css", "w") do |file|
file.puts "p { background: #{col}; }"
end
end

test "modification of a child affects dependent asset digests" do
write_child2('blue') # ensure no leftovers from previous runs
prev_assets = @assembly.load_path.assets
prev_main_digest = prev_assets.detect{|a| a.logical_path.to_s == 'main.css'}.digest
write_child2('red')
@assembly.load_path.cache_sweeper.execute
changed_assets = @assembly.load_path.assets
changed_main_digest = changed_assets.detect{|a| a.logical_path.to_s == 'main.css'}.digest
assert_not_equal(prev_main_digest, changed_main_digest)
write_child2('blue') # restore the original
@assembly.load_path.cache_sweeper.execute
final_assets = @assembly.load_path.assets
final_main_digest = final_assets.detect{|a| a.logical_path.to_s == 'main.css'}.digest
assert_equal(prev_main_digest, final_main_digest)
end
end