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

Make public asset use explicit #26226

Merged
merged 29 commits into from Aug 31, 2016
Merged

Conversation

@schneems
Copy link
Member

@schneems schneems commented Aug 19, 2016

Currently 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"

This is bad. No exceptions are raised, the error goes undetected.

This PR makes adds an explicit api for when you want to use an asset that is not in the asset pipeline. If you want an asset from the "public" directory you can now explicitly say so by using public_folder: true

asset_path("app1icati0n.js", public_folder: true)
# => "app1icati0n.js"

Now there is no confusion by sprockets-rails about the intent of the method call. Likewise in rails/sprockets-rails#375 we are adding a flag unknown_asset_fallback. When it is set to true then the current fallback behavior will happen, but a deprecation will be emitted. When it is set to false an exception will be raised so you know immediately that the asset was not found.

We are defaulting it to false for all new apps.

# Unknown asset fallback will return the path passed in when the given
# asset is not present in the asset pipeline.
Rails.application.config.assets.unknown_asset_fallback = false

While this patch is intended to go along with the sprockets-rails patch, it should still function with older sprockets-rails versions, however no deprecations or exceptions will be raised.

@schneems
Copy link
Member Author

@schneems schneems commented Aug 19, 2016

The sprockets-rails PR is rails/sprockets-rails#375

@schneems schneems force-pushed the schneems:schneems/explicit-public-urls branch 2 times, most recently Aug 19, 2016
@alexcameron89
alexcameron89 reviewed Aug 19, 2016
View changes
actionview/lib/action_view/helpers/asset_tag_helper.rb Outdated
@@ -304,19 +344,27 @@ def audio_tag(*sources)
multiple_sources_tag("audio", sources)
end

# Returns an HTML audio tag for the source asset in the public
# folder. This uses +audio_tag+ and skips any asset

This comment has been minimized.

@alexcameron89

alexcameron89 Aug 19, 2016
Member

You skipped this line in the documentation here: # lookups by assuming any assets are in thepublicfolder.

@alexcameron89
alexcameron89 reviewed Aug 19, 2016
View changes
actionview/lib/action_view/helpers/asset_url_helper.rb Outdated
@@ -158,6 +167,14 @@ def asset_path(source, options = {})
end
alias_method :path_to_asset, :asset_path # aliased to avoid conflicts with an asset_path named route

# Computes the path to an asset in the public folder.
# This uses +asset_path+ and skips any asset lookups by assuming the asset is in the

This comment has been minimized.

@alexcameron89

alexcameron89 Aug 19, 2016
Member

On these docs, should it be +path_to_asset+? Same question for the ones below.

@matthewd
Copy link
Member

@matthewd matthewd commented Aug 20, 2016

This feels too onerous to me. Particularly, having the asset pipeline hijack image_tag seems out of step with how other similar tag helpers behave.

@schneems
Copy link
Member Author

@schneems schneems commented Aug 20, 2016

@md not sure I follow. Asset pipeline already hijacks these helpers. This patch provides an un-hijack.

@robin850
Copy link
Member

@robin850 robin850 commented Aug 20, 2016

👍 on deprecating but can't we just add the :public_folder option to the existing helpers and output the warning if none of :public_folder or :raw is passed ? Thus we don't have to maintain more methods and more documentation for two things that are mostly the same.

@matthewd
Copy link
Member

@matthewd matthewd commented Aug 21, 2016

@schneems the distinction I'm making is that it currently enhances the helper, but doesn't interfere with non-asset use.

This also creates a cascade problem, where because image_path becomes public_image_path, image_tag must become public_image_tag, and so must some 3rd party helper that calls image_tag, etc.

I propose that we add public_image_path, and deprecate calling image_path with a relative filename that is not a known asset... and that's all.

So, anyone using image_tag with an asset, or a full image URL, or /some/thing.png, is unaffected -- all of those strings mean exactly one thing, and there's no potential for misinterpretation. Anyone currently using image_tag('my/image.png') to refer to something in public/images/my/image.png will be warned to change that to: image_tag(public_image_path('my/image.png')) -- I feel no obligation to offer a public_image_tag, because that formulation is relatively rare. And so the 3rd party helper will also be unaffected: it takes a string in, and the application-level caller introduces the public_image_path helper if they're doing the unusual thing.

@schneems
Copy link
Member Author

@schneems schneems commented Aug 21, 2016

I never liked the idea of introducing so many new helpers, method lookup time isn't free and more methods can mean slower performance over time. I do like the idea of just exposing the public_folder: true API. It's basically almost already implemented by this patch, and makes intent clear.

The code in rails needs to enable people to run the same app they do today without deprecations. It also needs to be written in such a way that we can detect correctly when we should be deprecating (and eventually raising). I'm not in love with the idea of something like image_tag(public_image_path('my/image.png')), it looks a bit gnarly and the only way I can see to detect when it's not being used correctly is to have public_image_path return a special object instead of a string.

I'm going to explore modifying this patch to get rid of the public_* helpers, which I think everyone agrees are not the best idea.

Side Notes

I wanted to note that I don't like the API of passing in an asset that starts with a slash as indicating a full path. I deprecated that. I think it's too easy to accidentally start an asset with a slash and not realize that you're bypassing the whole asset pipeline, silently. At the end of the day I want an error to show up in development if someone uses one of these helpers with anything that won't resolve to an asset that exists and render in production.

I also wanted to note a special case in asset_path for the full url option: When you call image_tag with something like image_tag("https://www.schneems.com/smile.png") the call will eventually call asset_path before it calls compute_asset_path which is the main entry point to the asset pipeline. Before that can happen

def asset_path(source, options = {})
  raise ArgumentError, "nil is not a valid asset source" if source.nil?

  source = source.to_s
  return "" if source.blank?
  return source if URI_REGEXP.match?(source) # <====== HERE

This last line will return true since we're giving it a full URI so the asset lookup never happens, no deprecation is thrown. So we don't need to handle that case for skipping deprecations.

Thanks for all then input. Let me know if you need more clarification on any of this, or want to respond to any of the sub-points.

@matthewd
Copy link
Member

@matthewd matthewd commented Aug 21, 2016

I wanted to note that I don't like the API of passing in an asset that starts with a slash as indicating a full path. I deprecated that.

IMO it's no more acceptable for image_tag('/a/url') to start insisting on assets than it would be for link_to('/a/url') to start complaining about the argument not matching a route: they're both direct convenience wrappers for their corresponding HTML tags, and it's only a secondary / additional convenience that they can intuit a "better" bonus behaviour for some arguments.

I think it's too easy to accidentally start an asset with a slash and not realize that you're bypassing the whole asset pipeline, silently.

It's not supposed to be; that's why we turned on digests in development. If you use /assets/my/image.png, that's already supposed to complain & fail. And if you use /my/image.png, it won't complain, but it also won't ever work -- and I could likely be convinced that should raise if my/image.png is a valid asset.

The only other possibility is that someone manually determines and then uses /assets/my/image-012345etc.png. That's going to go pretty poorly, but feels like a reasonable limit-point to me... the gun is braced to point horizontally and down-range: if you still manage to get your foot in front of it, good luck to you.

(Which of those three is the one that worries you? The first certainly used to be a big problem, but that's why we fixed it... or at least I thought we did.)


I'm not in love with the idea of something like image_tag(public_image_path('my/image.png')), it looks a bit gnarly

My first thought was to just kill the /images magic completely... given that the asset pipeline has been the "right" path for a long time now, I'm dubious that it sees much use. And people that are using it could just switch to /images/my/image.png -- the magic of the asset pipeline seems worthwhile for what it gives you... inserting 8 characters feels a bit more opaque-for-the-sake-of-it.

With that context, the above doesn't trouble me overmuch... the two methods are at very different conceptual levels, just like link_to(new_post_comment_path(@post)). Nice API is nice, but we're not here to prevent people from ever having to call more than one method.

and the only way I can see to detect when it's not being used correctly is to have public_image_path return a special object instead of a string.

It'd be returning an absolute URL /images/my/image.png, whereas I only want to complain about a bare my/image.png.

This last line will return true since we're giving it a full URI so the asset lookup never happens, no deprecation is thrown.

👍

@schneems
Copy link
Member Author

@schneems schneems commented Aug 22, 2016

I updated to use public_folder: true as the API and got rid of the public_* methods. Need to update sprockets-rails patch.

@schneems
Copy link
Member Author

@schneems schneems commented Aug 22, 2016

Updated sprockets-rails patch, was pretty minimal.

@schneems schneems force-pushed the schneems:schneems/explicit-public-urls branch Aug 23, 2016
@matthewd
matthewd reviewed Aug 23, 2016
View changes
railties/lib/rails/generators/rails/app/templates/config/initializers/assets.rb.tt Outdated
@@ -6,6 +6,10 @@ Rails.application.config.assets.version = '1.0'
# Add additional assets to the asset load path
# Rails.application.config.assets.paths << Emoji.images_path

# Unknown asset fallback will return the path passed in when the given
# asset is not present in the asset pipeline.
Rails.application.config.assets.unknown_asset_fallback = false

This comment has been minimized.

@matthewd

matthewd Aug 23, 2016
Member

I think this has to go elsewhere so the upgrade task can skip it 😕

This comment has been minimized.

@schneems

schneems Aug 25, 2016
Author Member

Moved to the new initializer

@prathamesh-sonpatki
prathamesh-sonpatki reviewed Aug 25, 2016
View changes
guides/source/asset_pipeline.md Outdated
@@ -586,6 +586,19 @@ in your application are included in the `config.assets.precompile` list.
If `config.assets.digest` is also true, the asset pipeline will require that
all requests for assets include digests.

### Raise an Error When an Asset is Not Found

If you are using a recent version of sprockets-rails you can configure what happens

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Aug 25, 2016
Member

may be specify exactly which version of sprockets-rails, like greater than > 3.something ?

@schneems
Copy link
Member Author

@schneems schneems commented Aug 26, 2016

Updated to Rails.application.config.assets.unknown_asset_fallback = <%= options[:update] ? true : false %> and mentioned the specific version of sprockets-rails in the assets guide,

@robin850
robin850 reviewed Aug 26, 2016
View changes
actionview/lib/action_view/helpers/asset_url_helper.rb Outdated
# This is the entry point for all assets.
# When using the asset pipeline (i.e. sprockets and sprockets-rails), the
# behavior is "enhanced". You can bypass the asset pipeline by passing in
# `public_folder: true` to the options.

This comment has been minimized.

@robin850

robin850 Aug 26, 2016
Member

The RDoc parser doesn't understand back-ticks, you should rather use <tt>public_folder: true</tt>.

@robin850
robin850 reviewed Aug 26, 2016
View changes
actionview/lib/action_view/helpers/asset_url_helper.rb Outdated
#
# asset_path("application.js") # => "/assets/application-60aa4fdc5cea14baf5400fba1abf4f2a46a5166bad4772b1effe341570f07de9.js"
#
# === Without the asset pipeline (`public_folder: true`)

This comment has been minimized.

@robin850

robin850 Aug 26, 2016
Member

Ditto, use <tt> here.

@robin850
robin850 reviewed Aug 26, 2016
View changes
actionview/lib/action_view/helpers/asset_url_helper.rb Outdated
# asset_path "http://www.example.com/js/xmlhr.js" # => http://www.example.com/js/xmlhr.js
# === With the asset pipeline
#
# All options passed to asset_path will be passed to `compute_asset_path

This comment has been minimized.

@robin850

robin850 Aug 26, 2016
Member

Here you could write +asset_path+ and +compute_asset_path+; if you don't wrap asset_path around pluses, RDoc will generate a link back to this method, which is pretty useless.

@robin850
robin850 reviewed Aug 26, 2016
View changes
actionview/lib/action_view/helpers/asset_url_helper.rb Outdated
# === Without the asset pipeline (`public_folder: true`)
#
# Accepts a `type` option that can specify the asset's extension. No error
# checking is done to verify the source passed into `asset_path` is valid

This comment has been minimized.

@robin850

robin850 Aug 26, 2016
Member

Ditto, you can use pluses here.

@robin850
robin850 reviewed Aug 26, 2016
View changes
actionview/lib/action_view/helpers/asset_url_helper.rb Outdated
#
# === Without the asset pipeline (`public_folder: true`)
#
# Accepts a `type` option that can specify the asset's extension. No error

This comment has been minimized.

@robin850

robin850 Aug 26, 2016
Member

Ditto.

@robin850
robin850 reviewed Aug 26, 2016
View changes
actionview/lib/action_view/helpers/asset_url_helper.rb Outdated
#
# === Options applying to all assets
#
# Below lists scenarios that apply to `asset_path` whether or not you're

This comment has been minimized.

@robin850

robin850 Aug 26, 2016
Member

Ditto.

@robin850
robin850 reviewed Aug 26, 2016
View changes
actionview/lib/action_view/helpers/asset_url_helper.rb Outdated
#
# asset_path("") # => ""
#
# - If `config.relative_url_root` is specified, all assets will have that

This comment has been minimized.

@robin850

robin850 Aug 26, 2016
Member

Ditto.

@robin850
robin850 reviewed Aug 26, 2016
View changes
actionview/lib/action_view/helpers/asset_url_helper.rb Outdated
# Rails.application.config.relative_url_root = "bar"
# asset_path("foo.js", public_folder: true) # => "bar/foo.js"
#
# - A different asset host can be specified via `config.action_controller.asset_host`

This comment has been minimized.

@robin850

robin850 Aug 26, 2016
Member

Ditto.

@robin850
robin850 reviewed Aug 26, 2016
View changes
actionview/lib/action_view/helpers/asset_url_helper.rb Outdated
# Rails.application.config.action_controller.asset_host = "assets.example.com"
# asset_path("foo.js", public_folder: true) # => "http://assets.example.com/foo.js"
#
# - An extension name can be specified manually with `extname`.

This comment has been minimized.

@robin850

robin850 Aug 26, 2016
Member

Ditto.

@robin850
robin850 reviewed Aug 26, 2016
View changes
guides/source/asset_pipeline.md Outdated
@@ -724,7 +737,7 @@ If you have other manifests or individual stylesheets and JavaScript files to
include, you can add them to the `precompile` array in `config/initializers/assets.rb`:

```ruby
Rails.application.config.assets.precompile += %w( admin.js admin.css )
Rails.application.config.assets.precompile += ['admin.js', 'admin.css', 'swfObject.js']

This comment has been minimized.

@robin850

robin850 Aug 26, 2016
Member

Really nit-picky but why did you change the style here ? This is not consistent with other code snippets and the code generated by Rails by default. 😊

@robin850
robin850 reviewed Aug 26, 2016
View changes
guides/source/configuring.md Outdated
@@ -169,6 +169,8 @@ pipeline is enabled. It is set to `true` by default.

* `config.assets.precompile` allows you to specify additional assets (other than `application.css` and `application.js`) which are to be precompiled when `rake assets:precompile` is run.

* `config.assets.unknown_asset_fallback` allows you to modify the behavior of the asset pipeline when an asset is not in the pipeline with recent verisons of sprockets-rails.

This comment has been minimized.

@robin850

robin850 Aug 26, 2016
Member

Also nit-picky but to be consistent, you could say like above, "in the pipeline, if you use sprockets-rails 3.2.0 or newer".

@robin850
Copy link
Member

@robin850 robin850 commented Aug 26, 2016

This is looking very good ! 👍

Just a nit-pick again regarding documentation though 😄 it looks like you didn't document the :public_poster_folder option for the video_tag method, this may be useful.

@schneems schneems force-pushed the schneems:schneems/explicit-public-urls branch to e1791d1 Aug 29, 2016
#
# Accepts a <tt>type</tt> option that can specify the asset's extension. No error
# checking is done to verify the source passed into +asset_path+ is valid
# and that the file exists on disk.

This comment has been minimized.

@dhh

dhh Aug 29, 2016
Member

Not the biggest fan of public_folder. It implies that we're doing something special with this, when all we do is pass this directly through. I'd think something negative like skip_pipeline: true would be clearer for that. Would match this documentation as well.

@@ -269,6 +270,8 @@ def image_alt(src)
# # => <video preload="none" controls="controls" src="/videos/trailer.ogg" ></video>
# video_tag("trailer.m4v", size: "16x10", poster: "screenshot.png")
# # => <video src="/videos/trailer.m4v" width="16" height="10" poster="/assets/screenshot.png"></video>
# video_tag("trailer.m4v", size: "16x10", poster: "screenshot.png", public_poster_folder: true)

This comment has been minimized.

@kaspth

kaspth Aug 30, 2016
Member

Shouldn't this just be skip_pipeline to match the other options? Presumably the same reasons renaming public_folder to skip_pipeline works here, so what do we gain making this stand out?

This comment has been minimized.

@schneems

schneems Aug 30, 2016
Author Member

There need to be 2 options here as the video may be in the pipeline but the poster might not be. Good catch though on the naming. I'll fix this.

This comment has been minimized.

@kaspth

kaspth Aug 30, 2016
Member

Ah gotcha!

# All options passed to +asset_path+ will be passed to +compute_asset_path+
# which is implemented by sprockets-rails.
#
# asset_path("application.js") # => "/assets/application-60aa4fdc5cea14baf5400fba1abf4f2a46a5166bad4772b1effe341570f07de9.js"

This comment has been minimized.

@kaspth

kaspth Aug 30, 2016
Member

I think our style is to nest code examples by 3 spaces, not 6.

This comment has been minimized.

@schneems

schneems Aug 30, 2016
Author Member

When you do that the example lines up with the bullet points, is that correct?

      # - An extension name can be specified manually with <tt>extname</tt>.
      #
      #   asset_path("foo", skip_pipeline: true, extname: ".js")     # => "/foo.js"
      #   asset_path("foo.css", skip_pipeline: true, extname: ".js") # => "/foo.css.js"

This comment has been minimized.

@kaspth

kaspth Aug 30, 2016
Member

Ah, overlooked the bullet points! I guess the style then is 2 spaces from the starting indentation of a sentence. So here it would be 5 spaces.

This comment has been minimized.

@kaspth

kaspth Aug 30, 2016
Member

In fact the examples seem to mix 6 and 5 spaces, so let's just have them all be 5 😁

This comment has been minimized.

@schneems

schneems Aug 30, 2016
Author Member

Some examples are bullet points and some aren't which is why they are different. Thanks for the reviews btw.

This comment has been minimized.

@kaspth

kaspth Aug 31, 2016
Member

Then they should only have had 3 space indentation 😉 — and no problem on the reviews, we were bit by the past behavior in our app at one point, so the thanks is all on me 😄

This comment has been minimized.

@kaspth

kaspth Aug 31, 2016
Member

Ah sorry, they did get shortened to 3 spaces! GitHub somehow showed me an older diff.

# - An extension name can be specified manually with <tt>extname</tt>.
#
# asset_path("foo", skip_pipeline: true, extname: ".js") # => "/foo.js"
# asset_path("foo.css", skip_pipeline: true, extname: ".js") # => "/foo.css.js"

This comment has been minimized.

@kaspth

kaspth Aug 30, 2016
Member

Should these last 3 examples use skip_pipeline?

This comment has been minimized.

@schneems

schneems Aug 30, 2016
Author Member

Makes the example more clear IMHO

This comment has been minimized.

@kaspth

kaspth Aug 30, 2016
Member

Yet I was confused by the usage 😄

I'm not objecting to using the option necessarily, but why does using it make the examples more clear?

This comment has been minimized.

@schneems

schneems Aug 30, 2016
Author Member

The assetpipeline does a bunch of things and I don't want asset pipeline magic being confused with this option. Adding a long fingerprint to the example makes it less clear that an extension was added to the output. What it doesn't do is replace an extension name, so foo.css does not become foo.js. Also technically this option doesn't change the extension output, it changes the input that goes into the asset pipeline. It's harder to show.

This comment has been minimized.

@kaspth

kaspth Aug 30, 2016
Member

Okay, sounds good 👍

}

cases.each do |(view_method, tag_match)|
app_file "app/views/posts/index.html.erb", "<%= #{ view_method } '#{contents}', skip_pipeline: true %>"

This comment has been minimized.

@kaspth

kaspth Aug 30, 2016
Member

Mixed spaces around interpolation braces abound in this test file. I think our preferred style is to not have any around interpolation braces, e.g.: #{view_method}.

@@ -282,8 +285,11 @@ def image_alt(src)
# video_tag(["trailer.ogg", "trailer.flv"], size: "160x120")
# # => <video height="120" width="160"><source src="/videos/trailer.ogg" /><source src="/videos/trailer.flv" /></video>
def video_tag(*sources)

This comment has been minimized.

@kaspth

kaspth Aug 30, 2016
Member

We could remove the options wrangling with a keyword argument:

def video_tag(*sources, skip_pipeline_on_poster: nil)
  multiple_sources_tag_builder("video", sources) do |options|
    options[:poster] = path_to_image(options[:poster], skip_pipeline: skip_pipeline_on_poster) if options[:poster]
    options[:width], options[:height] = extract_dimensions(options.delete(:size)) if options[:size]
  end
end

I think that would also save us a block argument shadowing local variable warning from options. 😁

This comment has been minimized.

@schneems

schneems Aug 30, 2016
Author Member

It's turtles all the way down, other method signatures that this gets passed to use extract_options.

Better to do that in a separate PR.

This comment has been minimized.

@schneems

schneems Aug 30, 2016
Author Member

Oh, i see what you're saying. I still want to do that in a different PR.

This comment has been minimized.

@kaspth

kaspth Aug 30, 2016
Member

I'm not generally objecting to extract_options!, I agree that's out of scope. I'm objecting to using it in video_tag because the diff says it didn't use it before. But messing around in IRB showed me that we can't mix keyword arguments with a symbol keyed options hash without using **options, so that doesn't save us much here anyway. 👍 to keeping it like you had it 😁

@schneems schneems merged commit 4ba40bc into rails:master Aug 31, 2016
2 checks passed
2 checks passed
codeclimate Code Climate has skipped analysis of this commit.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
claudiob added a commit to claudiob/rails that referenced this pull request Mar 14, 2017
schneems added a commit that referenced this pull request Mar 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.