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 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
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(mapped, dependent_assets)
wlipa marked this conversation as resolved.
Show resolved Hide resolved
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(mapped, dependent_assets)
wlipa marked this conversation as resolved.
Show resolved Hide resolved
# 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(mapped, dependent_assets)
wlipa marked this conversation as resolved.
Show resolved Hide resolved
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(mapped, dependent_assets)
end
end
17 changes: 10 additions & 7 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 @@ -42,6 +43,11 @@ def cache_sweeper
end
end

# needed for testing
def clear_cache
@cached_assets_by_path = nil
end
wlipa marked this conversation as resolved.
Show resolved Hide resolved

private
def assets_by_path
@cached_assets_by_path ||= Hash.new.tap do |mapped|
Expand All @@ -51,6 +57,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 All @@ -62,10 +69,6 @@ def without_dotfiles(files)
files.reject { |file| file.basename.to_s.starts_with?(".") }
end

def clear_cache
@cached_assets_by_path = nil
end

def dedup(paths)
paths = Array(paths).map { |path| Pathname.new(path) }
deduped = [].tap do |deduped|
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');
40 changes: 40 additions & 0 deletions test/propshaft/dependency_tree_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
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

test "modification of a child affects dependent asset digests" do
prev_assets = @assembly.load_path.assets
prev_main_digest = prev_assets.detect{|a| a.logical_path.to_s == 'main.css'}.digest
File.open("#{__dir__}/../fixtures/assets/dependent/child2.css", "w") do |file|
file.puts "p { background: red; }"
end
@assembly.load_path.clear_cache
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)
# restore the original
File.open("#{__dir__}/../fixtures/assets/dependent/child2.css", "w") do |file|
file.puts "p { background: blue; }"
end
@assembly.load_path.clear_cache
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
wlipa marked this conversation as resolved.
Show resolved Hide resolved
end