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

Asset names passed to helpers should not include the "/assets/" prefix. Error #167

Closed
nottewae opened this issue Sep 3, 2014 · 5 comments

Comments

@nottewae
Copy link

nottewae commented Sep 3, 2014

Asset names passed to helpers should not include the "/assets/" prefix. Instead of "/assets/top_fone.png", use "top_fone.png"


slim:
3: = image_tag asset_path("top_fone.png"), class:"col-md-12"


in gem 'sprockets-rails','= 2.1.4' only

@matthewd
Copy link
Member

matthewd commented Sep 3, 2014

Hmm. You can avoid the error by not doing a double-lookup: image_tag already handles asset paths; there's no need to use asset_path as well:

= image_tag "top_fone.png", class: "col-md-12"

(or, of course, turn raise_runtime_errors off.)

But it's true that this is not the sort of thing we're actually trying to warn about... our worry is people incorrectly doing image_tag("/assets/top_fone.png") directly.

@jcoyne
Copy link
Contributor

jcoyne commented Sep 5, 2014

@matthewd Seems like a good change, but pushing it out in a patch level release seems to violate SemVer.

@tagliala
Copy link

tagliala commented Sep 8, 2014

same here, this change breaked:

= javascript_include_tag '//oss.maxcdn.com/html5shiv/3.7.2/html5shiv.min.js', asset_path('respond.js')

and I didn't get the reason at first because the warning message says:

Asset names passed to helpers should not include the "/assets/" prefix. Instead of "/assets/respond.js", use "respond.js"

(like I was doing)

I agree with @jcoyne that it is a breaking change

@rebelwarrior
Copy link

I agree with @jcoyne this is a breaking change.

@rails rails locked and limited conversation to collaborators Sep 11, 2014
@josh
Copy link
Contributor

josh commented Nov 29, 2014

Any new flag like that should have go into a minor release. Though, the flag shouldn't have been enabled by default so it shouldn't have been backwards breaking.

@josh josh closed this as completed Nov 29, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants