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

Match Dev Asset Experience to Production #84

Merged
Merged
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
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
### Master

* Assets not in the precompile list can be checked for errors by setting
`config.assets.raise_runtime_errors = true` in any environment

*Richard Schneeman*


### 2.0.1

* Allow keep value to be specified for `assets:clean` run with args
Expand Down
50 changes: 47 additions & 3 deletions lib/sprockets/rails/helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ module Helper
# support for Ruby 1.9.3 && Rails 3.0.x
@_config = ActiveSupport::InheritableOptions.new({}) unless defined?(ActiveSupport::Configurable::Configuration)
include ActiveSupport::Configurable
config_accessor :raise_runtime_errors
config_accessor :precompile, :assets, :raise_runtime_errors

class DependencyError < StandardError
def initialize(path, dep)
Expand All @@ -18,6 +18,15 @@ def initialize(path, dep)
end
end

class AssetFilteredError < StandardError
def initialize(source)
msg = "Asset filtered out and will not be served: " <<
"add `config.assets.precompile += %w( #{source} )` " <<
"to `config/application.rb` and restart your server"
super(msg)
end
end

if defined? ActionView::Helpers::AssetUrlHelper
include ActionView::Helpers::AssetUrlHelper
include ActionView::Helpers::AssetTagHelper
Expand Down Expand Up @@ -62,6 +71,21 @@ def compute_asset_path(path, options = {})
end
end

# Computes the full URL to a asset in the public directory. This
# method checks for errors before returning path.
def asset_path(source, options = {})
check_errors_for(source)
path_to_asset(source, options)
end
alias :path_to_asset_with_errors :asset_path

# Computes the full URL to a asset in the public directory. This
# will use +asset_path+ internally, so most of their behaviors
# will be the same.
def asset_url(source, options = {})
path_to_asset_with_errors(source, options.merge(:protocol => :request))
end

# Get digest for asset path.
#
# path - String path
Expand Down Expand Up @@ -104,6 +128,7 @@ def javascript_include_tag(*sources)

if options["debug"] != false && request_debug_assets?
sources.map { |source|
check_errors_for(source)
if asset = lookup_asset_for_path(source, :type => :javascript)
asset.to_a.map do |a|
super(path_to_javascript(a.logical_path, :debug => true), options)
Expand All @@ -123,9 +148,9 @@ def javascript_include_tag(*sources)
# Eventually will be deprecated and replaced by source maps.
def stylesheet_link_tag(*sources)
options = sources.extract_options!.stringify_keys

if options["debug"] != false && request_debug_assets?
sources.map { |source|
check_errors_for(source)
if asset = lookup_asset_for_path(source, :type => :stylesheet)
asset.to_a.map do |a|
super(path_to_stylesheet(a.logical_path, :debug => true), options)
Expand All @@ -141,14 +166,33 @@ def stylesheet_link_tag(*sources)
end

protected

# Checks if 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
end

# Raise errors when source does not exist or is not in the precompiled list
def check_errors_for(source)
Copy link
Member

Choose a reason for hiding this comment

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

WDYT?

def check_errors_for(source)
  source = source.to_s

  if !self.raise_runtime_errors || source.blank? || source =~ URI_REGEXP
    return source
  else
    asset = lookup_asset_for_path(source)

    if asset && asset_needs_precompile?(source, asset.pathname.to_s)
      raise AssetFilteredError.new(source)
    end
  end
end

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I would prefer if we didn't have to nest that if

def check_errors_for(source)
  source = source.to_s
  return source if !self.raise_runtime_errors || source.blank? || source =~ URI_REGEXP
  asset = lookup_asset_for_path(source)

  if asset && asset_needs_precompile?(source, asset.pathname.to_s)
    raise AssetFilteredError.new(source)
  end
end

Copy link
Member

Choose a reason for hiding this comment

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

no problem with this too.

source = source.to_s
return source if !self.raise_runtime_errors || source.blank? || source =~ URI_REGEXP
asset = lookup_asset_for_path(source)

if asset && asset_needs_precompile?(source, asset.pathname.to_s)
raise AssetFilteredError.new(source)
end
end

# Returns true when an asset will not be available after precompile is run
def asset_needs_precompile?(source, filename)
Copy link
Member

Choose a reason for hiding this comment

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

Not a fan of sort-circuits ifs. I think this would work:

if assets && assets.send(:matches_filter, precompile || [], source, filename)
  false
else
  true
end

Copy link
Member Author

Choose a reason for hiding this comment

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

i ususally like them if they let me get rid of a nested if/else or cut down on lines. In this case your suggestion is good. I will do that.

if assets_environment && assets_environment.send(:matches_filter, precompile || [], source, filename)
false
else
true
end
end

# Enable split asset debugging. Eventually will be deprecated
# and replaced by source maps in Sprockets 3.x.
def request_debug_assets?
Expand Down
3 changes: 3 additions & 0 deletions lib/sprockets/railtie.rb
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,9 @@ def configure(&block)
app.assets = app.assets.index
end


Sprockets::Rails::Helper.precompile ||= app.config.assets.precompile
Sprockets::Rails::Helper.assets ||= app.assets
Sprockets::Rails::Helper.raise_runtime_errors = app.config.assets.raise_runtime_errors

if config.assets.compile
Expand Down
2 changes: 1 addition & 1 deletion test/fixtures/error/dependency.js.erb
Original file line number Diff line number Diff line change
@@ -1 +1 @@
<%= asset_path("bar.js") %>
<%= asset_path("bar.js") %>
60 changes: 57 additions & 3 deletions test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,56 @@ def test_stylesheet_path
assert_equal "/assets/foo-#{@foo_css_digest}.css", @view.stylesheet_path("foo")
end

def test_public_folder_fallback_works_correctly
@view.raise_runtime_errors = true
@view.debug_assets = true

@view.asset_path("asset-does-not-exist-foo.js")
@view.asset_url("asset-does-not-exist-foo.js")
@view.stylesheet_link_tag("asset-does-not-exist-foo.js")
@view.javascript_include_tag("asset-does-not-exist-foo.js")
end

def test_asset_not_precompiled_error
@view.raise_runtime_errors = true
@view.precompile = [ lambda {|logical_path| false } ]
@view.assets_environment = @assets
@view.debug_assets = true

assert_raise(Sprockets::Rails::Helper::AssetFilteredError) do
@view.asset_path("foo.js")
end

assert_raise(Sprockets::Rails::Helper::AssetFilteredError) do
@view.asset_url("foo.js")
end

assert_raise(Sprockets::Rails::Helper::AssetFilteredError) do
@view.javascript_include_tag("foo.js")
end

assert_raise(Sprockets::Rails::Helper::AssetFilteredError) do
@view.javascript_include_tag("foo")
end

error = assert_raise(Sprockets::Rails::Helper::AssetFilteredError) do
@view.javascript_include_tag(:foo)
end

assert_raise(Sprockets::Rails::Helper::AssetFilteredError) do
@view.stylesheet_link_tag("foo.js")
end

@view.precompile = [ lambda {|logical_path| true } ]

@view.asset_path("foo.js")
@view.asset_url("foo.js")
@view.javascript_include_tag("foo.js")
@view.javascript_include_tag("foo")
@view.javascript_include_tag(:foo)
@view.stylesheet_link_tag("foo.js")
end

def test_asset_digest_path
assert_equal "foo-#{@foo_js_digest}.js", @view.asset_digest_path("foo.js")
assert_equal "foo-#{@foo_css_digest}.css", @view.asset_digest_path("foo.css")
Expand All @@ -384,13 +434,17 @@ def test_asset_digest
end

class ErrorsInHelpersTest < HelperTest

def test_dependency_error
@view.raise_runtime_errors = true
@view.precompile = [ lambda {|logical_path| true } ]
@view.assets_environment = @assets

assert_raise Sprockets::Rails::Helper::DependencyError do
@assets['error/dependency.js'].to_s
@view.asset_path("error/dependency.js")
end

@view.raise_runtime_errors = false
@assets['error/dependency.js'].to_s
@view.asset_path("error/dependency.js")
end
end
end