Sprockets should ignore :cache and :concat options #6834

Closed
wants to merge 2 commits into
from

4 participants

@0x000000

By some reasons, if someone will put these options in javascript_include_tag or stylesheet_link_tag it will
cause strange errors (when sprockets is enable for app). For me it took about work day to detect what exactly happened.

You can easy reproduce the error: just create new rails 3.2 app, generate simple controller with one action, specify root action in routes and add :cache => true, :cache => "foo" or :concat => "foo" to the end of any javascript_include_tag or stylesheet_link_tag helpers.

I am in the middle of migrating 2.3.x app to 3.2.x for my current project, so I have a lot of legacy :cache options. Unfortunately current exception is too hard to undersand situation for developers, without spending time to debugging Rails.

I suggest to ignore :cache and :concat options for js and css sprockets tag helpers (like I have done). Also I think that sprockets may detect these options and say something (for example it can show warning).

If something wrong in my commit let me know and I will fix it asap. Hope it will help someone.

@steveklabnik
Ruby on Rails member

I think this is a bug that I fixed in #6752, actually.

@0x000000

If you take a look at https://github.com/rails/rails/pull/6752/files#L0R40 you will see the line, which produces wrong path when cache or concat options are enabled. In this case paths will looks like "/Absolute/path/to/public/" + "/Absolute/path/to/asset/file.ext" which is wrong.

So to avoid this behaviour I suggest to ignore :cache and :concat options when sprockets is enabled, because sprockets already does things like these options.

@guilleiguaran guilleiguaran and 1 other commented on an outdated diff Sep 11, 2012
actionpack/lib/sprockets/helpers/rails_helper.rb
@@ -23,6 +23,8 @@ def javascript_include_tag(*sources)
body = options.key?(:body) ? options.delete(:body) : false
digest = options.key?(:digest) ? options.delete(:digest) : digest_assets?
+ ignore_legacy_options(options)
@guilleiguaran
Ruby on Rails member
guilleiguaran added a line comment Sep 11, 2012

This can be just:

options.except!(:cache, :concat)
@0x000000
0x000000 added a line comment Sep 12, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilleiguaran
Ruby on Rails member

If this is going to be applied please only in 3-2-stable, I think in 4.0 everyone should delete legacy options 😄

@guilleiguaran
Ruby on Rails member
@jeremy
Ruby on Rails member

How about raising a better exception that makes it clear that you should stop using these options?

@steveklabnik
Ruby on Rails member

Right. Deprecate in 4.0, remove in 4.1 being the rule.

@guilleiguaran
Ruby on Rails member

@0x000000 what's the exception that you are getting?

@0x000000

@guilleiguaran here is sample of error which I am getting by way described in pull-request message (for Rails 3.2.8 as you may see): https://gist.github.com/edd5d9433b7c44754473

Also I have attached tests for this case. Not sure there is a big problem, but I had some troubles when migrating 2.x application to 3.x.

@guilleiguaran
Ruby on Rails member

@0x000000 I think the best place to do this is on docs that the people read when they're upgrading their apps:

rails/docrails@53568e0
rails/docrails@276e7b0
rails/docrails@6250873

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment