Skip to content

Commit

Permalink
Instead of complaining, add the dependency automatically
Browse files Browse the repository at this point in the history
We need to use *both* `depend_on` and `depend_on_asset`, because the
latter (bugfully?) considers changes to the referenced asset's
dependencies, but not the file itself (making it entirely useless for an
image).

In the corresponding test, we call a protected Sprockets method... by
design, this isn't something we're supposed to care about / access. But
actually manipulating assets to ensure the dependency has taken hold
seems rather like overkill.
  • Loading branch information
matthewd committed Apr 9, 2014
1 parent ff1e483 commit 5723c7e
Show file tree
Hide file tree
Showing 4 changed files with 9 additions and 25 deletions.
17 changes: 4 additions & 13 deletions lib/sprockets/rails/helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,6 @@ def raise_runtime_errors
Sprockets::Rails::Helper.raise_runtime_errors
end

class DependencyError < StandardError
def initialize(path, dep)
msg = "Asset depends on '#{dep}' to generate properly but has not declared the dependency\n"
msg << "Please add: `//= depend_on_asset \"#{dep}\"` to '#{path}'"
super msg
end
end

class AssetFilteredError < StandardError
def initialize(source)
msg = "Asset filtered out and will not be served: " <<
Expand Down Expand Up @@ -71,7 +63,7 @@ def self.extended(obj)

def compute_asset_path(path, options = {})
# Check if we are inside Sprockets context before calling check_dependencies!.
check_dependencies!(path) if defined?(_dependency_assets)
check_dependencies!(path) if defined?(depend_on)

if digest_path = asset_digest_path(path)
path = digest_path if digest_assets
Expand Down Expand Up @@ -177,11 +169,10 @@ def stylesheet_link_tag(*sources)
end

protected
# Checks if the asset is included in the dependencies list.
# Ensures the asset is included in the dependencies list.
def check_dependencies!(dep)
if raise_runtime_errors && !_dependency_assets.detect { |asset| asset.include?(dep) }
raise DependencyError.new(self.pathname, dep)
end
depend_on(dep)

This comment has been minimized.

Copy link
@robin850

robin850 Apr 21, 2014

Member

Unfortunately this line makes the sass-rails build red. Would you mind taking a look please ?

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Apr 22, 2014

Member

I'll, but I'm pretty sure it is something that needs to be fixed on sass-rails test suite

This comment has been minimized.

Copy link
@matthewd

matthewd Apr 22, 2014

Author Member

@rafaelfranca it's a "feature" that it silently "works" when the file doesn't exist 😞

Addressed in #144.

depend_on_asset(dep)
end

# Raise errors when source does not exist or is not in the precompiled list
Expand Down
1 change: 0 additions & 1 deletion test/fixtures/url.css.erb
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
//= depend_on_asset "logo.png"
p { background: url(<%= image_path "logo.png" %>); }
1 change: 0 additions & 1 deletion test/fixtures/url.js.erb
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
//= depend_on_asset "foo.js"
var url = '<%= javascript_path :foo %>';
15 changes: 5 additions & 10 deletions test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -435,18 +435,13 @@ def test_asset_digest
end
end

class ErrorsInHelpersTest < HelperTest
class AutomaticDependenciesFromHelpersTest < HelperTest

def test_dependency_error
Sprockets::Rails::Helper.raise_runtime_errors = true
Sprockets::Rails::Helper.precompile = [ lambda {|logical_path| true } ]
@view.assets_environment = @assets
def test_dependency_added
@view.assets_environment = @assets

assert_raises Sprockets::Rails::Helper::DependencyError do
@view.asset_path("error/dependency.js")
end
@view.asset_path("url.css")

Sprockets::Rails::Helper.raise_runtime_errors = false
@view.asset_path("error/dependency.js")
assert_equal ["logo.png", "url.css.erb"], @assets['url.css'].send(:dependency_paths).map {|d| File.basename(d.pathname) }.sort
end
end

0 comments on commit 5723c7e

Please sign in to comment.