Deprecate asset fallback #375

Merged
merged 8 commits into from Sep 1, 2016

Projects

None yet

3 participants

@schneems
Member

When an asset isn't found the behavior is to pass the string through. For example a valid asset will return a url from pipeline

asset_path("application.js")
# => assets/application-123098udasvi0mnafd.js

While if you make a typo, you won't get an error or anything, it just falls through:

asset_path("app1icati0n.js")
# => "app1icati0n.js"

Hopefully I don't have to elaborate on why this is bad.

This PR is a child PR to one in Rails that will introduce a public_asset_path API.

There are valid reasons for not using the asset pipeline, if you have a purely static asset in the public folder or if you want to link to a static URL in your assets somewhere it makes sense to declare that intention. Eventually we will replace the behavior of the deprecation with an exception so people don't lose hours of their to typos.

We only emit the deprecation when a public_ api is available in Rails. This means that sprockets-rails can still be used in a backwards compatible manner.

@schneems schneems Deprecate asset fallback
When an asset isn't found the behavior is to pass the string through. For example a valid asset will return a url from pipeline

```
asset_path("application.js")
# => assets/application-123098udasvi0mnafd.js
```

While if you make a typo, you won't get an error or anything, it just falls through:

```
asset_path("app1icati0n.js")
# => "app1icati0n.js"
```

Hopefully I don't have to elaborate on why this is bad.

This PR is a child PR to one in Rails that will introduce a `public_asset_path` API.

There are valid reasons for not using the asset pipeline, if you have a purely static asset in the public folder or if you want to link to a static URL in your assets somewhere it makes sense to declare that intention. Eventually we will replace the behavior of the deprecation with an exception so people don't lose hours of their to typos.

We only emit the deprecation when a `public_` api is available in Rails. This means that sprockets-rails can still be used in a backwards compatible manner.
03533fe
@schneems schneems referenced this pull request in rails/rails Aug 19, 2016
Merged

Make public asset use explicit #26226

@schneems
Member

The Rails PR is rails/rails#26226

schneems added some commits Aug 20, 2016
@schneems schneems Better way to find deprecated method location
Instead of relying on hard coded indexes, we can look through the called method stack backwards (i.e. the first method called will be the first in the list). If it has a suffix for one of our APIs, then we check to see if there exists a `public_` prefix on the view, if it does it's the first method called that can be deprecated so we point to that.
5902abe
@schneems schneems public_* API is not desired
Use `public_folder: true` instead.
e2f615d
@schneems schneems public_* API is not desired
- Use `public_folder: true` instead.
- Allow configurable deprecation/error behavior via **`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.
d00b8ed
@schneems schneems Change option name to `skip_pipeline`
1af59d2
@kaspth kaspth commented on the diff Aug 30, 2016
lib/sprockets/rails/helper.rb
@@ -6,6 +6,7 @@
module Sprockets
module Rails
module Helper
+ class AssetNotFound < StandardError; end
@kaspth
kaspth Aug 30, 2016 Member

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

@kaspth kaspth and 2 others commented on an outdated diff Aug 30, 2016
lib/sprockets/rails/helper.rb
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."
+ 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"
+ message << "pass in `skip_pipeline: true` instead.\n"
@kaspth
kaspth Aug 30, 2016 Member

Pass in...?

@schneems
schneems Aug 31, 2016 Member

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"
@matthewd
matthewd Aug 31, 2016 Member

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.

@kaspth kaspth commented on an outdated diff Aug 30, 2016
lib/sprockets/rails/helper.rb
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."
+ 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"
@kaspth
kaspth Aug 30, 2016 Member

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).

@kaspth kaspth and 1 other commented on an outdated diff Aug 30, 2016
lib/sprockets/rails/helper.rb
@@ -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"
@kaspth
kaspth Aug 30, 2016 Member

Pass in...?

@schneems
schneems Aug 31, 2016 Member

Not sure why this comment shows twice.

@schneems
schneems Aug 31, 2016 Member

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.

@kaspth kaspth and 1 other commented on an outdated diff Aug 30, 2016
### 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.
@kaspth
kaspth Aug 30, 2016 Member

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

the a result -> the result.

@matthewd
matthewd Aug 31, 2016 Member

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

schneems added some commits Aug 31, 2016
@schneems schneems Better copy and deprecation message
Address comments by @kaspth
230b7e1
@schneems schneems Remove unused method
dfae4c5
@matthewd matthewd and 1 other commented on an outdated diff Aug 31, 2016
lib/sprockets/rails/helper.rb
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."
+ 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"
+ message << "pass in `skip_pipeline: true` instead.\n"
+
+ call_stack = respond_to?(:caller_locations) ? caller_locations : caller
@matthewd
matthewd Aug 31, 2016 Member

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?

@schneems
schneems Sep 1, 2016 Member

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.

@matthewd matthewd commented on an outdated diff Aug 31, 2016
lib/sprockets/rails/helper.rb
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."
@matthewd
matthewd Aug 31, 2016 Member

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.

@schneems schneems Remove "you" from deprecation.
09730f9
@kaspth
Member
kaspth commented Aug 31, 2016

Does @matthewd's comment about caller_locations stand? Otherwise :shipit: 👏

@schneems schneems merged commit e3dc15c into master Sep 1, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment