-
Notifications
You must be signed in to change notification settings - Fork 97
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
@import url('child2.css'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
p { background: blue; } |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
@import url('cyclic2.css'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
@import url('cyclic1.css'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
@import url('child1.css'); |
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 |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.