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

Deprecate asset fallback #375

Merged
merged 8 commits into from
Sep 1, 2016
Merged
Show file tree
Hide file tree
Changes from 5 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
5 changes: 4 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,12 @@ Each asset task will invoke `assets:environment` first. By default this loads th

Also see [Sprockets::Rails::Task](https://github.com/rails/sprockets-rails/blob/master/lib/sprockets/rails/task.rb) and [Rake::SprocketsTask](https://github.com/rails/sprockets/blob/master/lib/rake/sprocketstask.rb).


### Initializer options

**`config.assets.unknown_asset_fallback`**

When set to a truthy value, the a result will be returned even if the requested asset is not found in the asset pipeline. When set to `false` it will raise an error.
Copy link

Choose a reason for hiding this comment

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

You mention truthy and yet also use an explicit false — shouldn't a falsy value work?

the a result -> the result.

Copy link
Member

Choose a reason for hiding this comment

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

Does Sprockets have precedent for using 'truthy'? Rails uses 'true' (as opposed to 'true') for that.


**`config.assets.precompile`**

Add additional assets to compile on deploy. Defaults to `application.js`, `application.css` and any other non-js/css file under `app/assets`.
Expand Down
29 changes: 28 additions & 1 deletion lib/sprockets/rails/helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
module Sprockets
module Rails
module Helper
class AssetNotFound < StandardError; end
Copy link

Choose a reason for hiding this comment

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

Shouldn't we add a newline after this class definition?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

class AssetNotPrecompiled < StandardError
include Sprockets::Rails::Utils
def initialize(source)
Expand Down Expand Up @@ -33,7 +34,8 @@ def initialize(source)
:assets_environment, :assets_manifest,
:assets_precompile, :precompiled_asset_checker,
:assets_prefix, :digest_assets, :debug_assets,
:resolve_assets_with, :check_precompiled_asset
:resolve_assets_with, :check_precompiled_asset,
:unknown_asset_fallback
]

def self.included(klass)
Expand Down Expand Up @@ -68,12 +70,24 @@ def assets_environment
end
end

# Writes over the built in ActionView::Helpers::AssetUrlHelper#compute_asset_path
# to use the asset pipeline.
def compute_asset_path(path, options = {})
debug = options[:debug]

if asset_path = resolve_asset_path(path, debug)
File.join(assets_prefix || "/", legacy_debug_path(asset_path, debug))
else
message = "The asset #{ path.inspect } you are looking for is not present in the asset pipeline."
Copy link
Member

Choose a reason for hiding this comment

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

Don't use "you" in an exception message (or a deprecation).

I think this should have separate phrasings for the exception vs the deprecation, actually, even though the intent is similar. The deprecation wants a softer "we didn't find it in the place [but we're doing this other thing]" ('not present in'); for the exception, that feels too gentle... a more definitive "not found" / "unknown asset" seems called for.

raise AssetNotFound, message unless unknown_asset_fallback

if respond_to?(:public_compute_asset_path)
message << "The public fallback behavior is being deprecaed and will be removed.\n"
Copy link

Choose a reason for hiding this comment

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

If users see this message it's because we have deprecated this type of lookup. So I'd go with is deprecated instead (there's also a typo in deprecated).

message << "pass in `skip_pipeline: true` instead.\n"
Copy link

Choose a reason for hiding this comment

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

Pass in...?

Copy link
Member Author

Choose a reason for hiding this comment

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

How is this?

            message << "Falling back to an asset that may be in the public folder.\n"
            message << "This behavior is deprecated and will be removed.\n"
            message << "To bypass the asset pipeline and preserve this behavior,\n"
            message << "use the `skip_pipeline: true` option.\n"

Copy link
Member

Choose a reason for hiding this comment

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

I'd consider something more terse, e.g.:

"Asset fallback is deprecated. To bypass the asset pipeline, use `skip_pipeline: true`"

Because of where/how they show up (i.e., often many times in a row), overly-helpful deprecation messages can just be overwhelming. IMO to-the-point + Google-able are the most valuable attributes here.


call_stack = respond_to?(:caller_locations) ? caller_locations : caller
Copy link
Member

Choose a reason for hiding this comment

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

If we're on a Rails new enough to have public_compute_asset_path, that implies we're on a Ruby new enough to have caller_locations, doesn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

The tests don't actually use Rails 5.1, we define the method, and the tests get run on all Ruby versions. The alternative to this would be detecting ruby versions and skipping the tests, which is less clean. I also want to be able to make a gem so that older versions of Rails can use this flag to get the error/deprecation behavior.

ActiveSupport::Deprecation.warn(message, call_stack)
end
super
end
end
Expand Down Expand Up @@ -244,6 +258,19 @@ def legacy_debug_path(path, debug)
path
end
end

private
# Emits a deprecation warning when asset pipeline
# is used with an asset that is not part of the pipeline.
#
# Attempts to determine proper method name based on caller.
def deprecate_invalid_asset_lookup(name, call_stack)
message = "The asset #{ name.inspect } you are looking for is not present in the asset pipeline.\n"
message << "The public fallback behavior is being deprecated and will be removed.\n"
message << "pass in `skip_pipeline: true` instead.\n"
Copy link

Choose a reason for hiding this comment

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

Pass in...?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why this comment shows twice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh, i need to remove this method, we're not using it anymore. Added this in when we tried to get fancy computing exactly where in the call stack you were when trying to look up the asset.


ActiveSupport::Deprecation.warn(message, call_stack)
end
end

# Use a separate module since Helper is mixed in and we needn't pollute
Expand Down
3 changes: 2 additions & 1 deletion lib/sprockets/railtie.rb
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ def configure(&block)
config.assets.cache_limit = 50.megabytes
config.assets.gzip = true
config.assets.check_precompiled_asset = true
config.assets.unknown_asset_fallback = true

config.assets.configure do |env|
config.assets.paths.each { |path| env.append_path(path) }
Expand Down Expand Up @@ -245,7 +246,7 @@ def self.build_manifest(app)
self.resolve_assets_with = config.assets.resolve_with

self.check_precompiled_asset = config.assets.check_precompiled_asset

self.unknown_asset_fallback = config.assets.unknown_asset_fallback
# Expose the app precompiled asset check to the view
self.precompiled_asset_checker = -> logical_path { app.asset_precompiled? logical_path }
end
Expand Down
32 changes: 32 additions & 0 deletions test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ def setup
@view.assets_precompile = %w( manifest.js )
precompiled_assets = @manifest.find(@view.assets_precompile).map(&:logical_path)
@view.check_precompiled_asset = true
@view.unknown_asset_fallback = true
@view.precompiled_asset_checker = -> logical_path { precompiled_assets.include? logical_path }
@view.request = ActionDispatch::Request.new({
"rack.url_scheme" => "https"
Expand Down Expand Up @@ -827,6 +828,37 @@ def test_index_files
end
end

class DeprecationTest < HelperTest
def test_deprecations_for_asset_path
@view.send(:define_singleton_method, :public_compute_asset_path, -> {})
assert_deprecated do
@view.asset_path("does_not_exist.noextension")
end
ensure
@view.instance_eval('undef :public_compute_asset_path')
end

def test_deprecations_for_asset_url
@view.send(:define_singleton_method, :public_compute_asset_path, -> {})

assert_deprecated do
@view.asset_url("does_not_exist.noextension")
end
ensure
@view.instance_eval('undef :public_compute_asset_path')
end

def test_deprecations_for_image_tag
@view.send(:define_singleton_method, :public_compute_asset_path, -> {})

assert_deprecated do
@view.image_tag("does_not_exist.noextension")
end
ensure
@view.instance_eval('undef :public_compute_asset_path')
end
end

class RaiseUnlessPrecompiledAssetDisabledTest < HelperTest
def test_check_precompiled_asset_enabled
@view.check_precompiled_asset = true
Expand Down