Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
3.1 rc5, sprockets 2.0.0.beta.13: Setting config.assets.compress = true causes rails/sprockets to ignore js file include order #2537
Comments
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
@josh ? |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
We're going to need some more info. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
ncri
Aug 15, 2011
Okay, well, what info? ;-)
Well here is my manifest file:
// This is a manifest file that'll be compiled into including all the files listed below.
// Add new JavaScript/Coffee code in separate files in this directory and they'll automatically
// be included in the compiled file accessible from http://example.com/assets/application.js
// It's not advisable to add code directly here, but if you do, it'll appear at the bottom of the
// the compiled file. require_tree .
//
//= require prototype
//= require prototype_ujs
//= require effects
//= require dragdrop
//= require controls
//= require main/lowpro
... and some more js files...
//= require_self
This load order is preserved when asset compression is off. But when on, the files seem to be compiled in alphabetical order, so i get lots of js errors.
ncri
commented
Aug 15, 2011
Okay, well, what info? ;-) Well here is my manifest file: // This is a manifest file that'll be compiled into including all the files listed below. This load order is preserved when asset compression is off. But when on, the files seem to be compiled in alphabetical order, so i get lots of js errors. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
ncri
commented
Aug 16, 2011
Hi there, any updates on this? |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
spastorino
Aug 17, 2011
Owner
Hey @ncri can you push an app to github that shows the issue? that would be helpful, thanks!!
Hey @ncri can you push an app to github that shows the issue? that would be helpful, thanks!! |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
elstudio
Aug 18, 2011
I can confirm this is still an issue for 3.1 RC6 -- it breaks prototype and UJS/RJS for my app.
What can I get you guys to help diagnose this?
Would it help to see application.js source, and then compiled with and without asset compression?
elstudio
commented
Aug 18, 2011
I can confirm this is still an issue for 3.1 RC6 -- it breaks prototype and UJS/RJS for my app. What can I get you guys to help diagnose this? Would it help to see application.js source, and then compiled with and without asset compression? |
#2580 is going to solve it. Please test this new approach. |
josh
closed this
Aug 18, 2011
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
ncri
Aug 18, 2011
Thanks, I don't quite get it though what that ticket #2580 is about. My issue is not about offline or on the fly compiling, it's about the order sprockets appends my js files together when I switch on asset compression, which is preserved without compression. Sprockets is supposed to keep the ordering defined by the requires in the manifest as far as I know, so this is its intended use. So it seems like a bug to me. But maybe it's in sprockets?
ncri
commented
Aug 18, 2011
Thanks, I don't quite get it though what that ticket #2580 is about. My issue is not about offline or on the fly compiling, it's about the order sprockets appends my js files together when I switch on asset compression, which is preserved without compression. Sprockets is supposed to keep the ordering defined by the requires in the manifest as far as I know, so this is its intended use. So it seems like a bug to me. But maybe it's in sprockets? |
NZKoz
reopened this
Aug 18, 2011
ncri
commented
Aug 19, 2011
Could someone please reopen this? I believe it is not fixed by #2580 as that just adresses precompilation. But correct me if I'm wrong. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
ncri
commented
Aug 19, 2011
I can make an example app to illustrate the issue if it helps. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
ncri
commented
Aug 19, 2011
Oops, has been reopen already, a page refresh could help. :) thx |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
guilleiguaran
Aug 20, 2011
Owner
This still failing with Rails 3.1.0.rc6 and Sprockets 2.0.0.beta.14?
This still failing with Rails 3.1.0.rc6 and Sprockets 2.0.0.beta.14? |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
ncri
commented
Aug 20, 2011
Yep. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
elstudio
Aug 25, 2011
Okay, here's that test app. It's a stoplight.
Source: https://github.com/elstudio/stoplight-rjs-test
Production on Heroku: http://blazing-mist-772.herokuapp.com/
When run in Development (without JS asset compression), the stoplight changes colors when you click on a color name. It uses RJS to do this.
In production, though (both locally and on Heroku), the UJS/RJS fails -- you'll see page loads, for one thing. And of course the colors don't change. If I turn off asset compression in production the UJS/RJS works fine.
This is a problem for Sprockets 2.0.0.beta.14 with both Rails 3.1.0.rc6 and today's Rails 3-1-0 branch, 283f5.
Let me know what I can do to help!
elstudio
commented
Aug 25, 2011
Okay, here's that test app. It's a stoplight. Source: https://github.com/elstudio/stoplight-rjs-test Production on Heroku: http://blazing-mist-772.herokuapp.com/ When run in Development (without JS asset compression), the stoplight changes colors when you click on a color name. It uses RJS to do this. In production, though (both locally and on Heroku), the UJS/RJS fails -- you'll see page loads, for one thing. And of course the colors don't change. If I turn off asset compression in production the UJS/RJS works fine. This is a problem for Sprockets 2.0.0.beta.14 with both Rails 3.1.0.rc6 and today's Rails 3-1-0 branch, 283f5. Let me know what I can do to help! |
ncri
commented
Aug 26, 2011
thanks @elstudio ! |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
Is this RJS only related? can I see an example without RJS? |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
@ncri your app was using RJS too? |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
ncri
Aug 27, 2011
@spastorino Yes, but the issue is really about the asset pipeline's file compilation order. And that is completely independent of RJS.
ncri
commented
Aug 27, 2011
@spastorino Yes, but the issue is really about the asset pipeline's file compilation order. And that is completely independent of RJS. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
spastorino
Aug 27, 2011
Owner
@ncri I'd like to take a look to an app without RJS, if you can build one as an example please do it
@ncri I'd like to take a look to an app without RJS, if you can build one as an example please do it |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
guilleiguaran
Aug 28, 2011
Owner
@ncri @elstudio the assets are concatened in alphabetical order when compression is on? I'm trying to reproduce it (without RJS) and I couldn't:
application.js:
//= require b
//= require a
b.js:
var a = 1;
a.js:
a = a + 1;
With config.assets.compress = false
, I'm getting:
var a = 1;
a = a + 1;
and with config.assets.compress = true
I'm getting:
var a=1;a=a+1;
I will check now if the use of external assets (ex. jquery_ujs) is changing this order
@ncri @elstudio the assets are concatened in alphabetical order when compression is on? I'm trying to reproduce it (without RJS) and I couldn't: application.js: //= require b
//= require a b.js: var a = 1; a.js: a = a + 1; With var a = 1;
a = a + 1; and with var a=1;a=a+1; I will check now if the use of external assets (ex. jquery_ujs) is changing this order |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
guilleiguaran
Aug 28, 2011
Owner
Just found the issue, it is caused by Uglifier (UglifyJS?) doing wrong compression with your files.
I modified the app created by @elstudio to use Closure as compressor and now it works fine
Sorry but there aren't nothing that we can do in Rails or Sprockets to solve it. You will need to contact to the authors of Uglifier/UglifyJS to get this fixed and use alternative compressors as temporary workaround
Just found the issue, it is caused by Uglifier (UglifyJS?) doing wrong compression with your files. I modified the app created by @elstudio to use Closure as compressor and now it works fine Sorry but there aren't nothing that we can do in Rails or Sprockets to solve it. You will need to contact to the authors of Uglifier/UglifyJS to get this fixed and use alternative compressors as temporary workaround |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
ncri
Aug 28, 2011
@guilleiguaran Great you found the issue. Just wondering if uglifier is the default compressor? As the issue appears when setting config.assets.compress = true even without specifiying a compressor.
ncri
commented
Aug 28, 2011
@guilleiguaran Great you found the issue. Just wondering if uglifier is the default compressor? As the issue appears when setting config.assets.compress = true even without specifiying a compressor. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
guilleiguaran
Aug 28, 2011
Owner
Yes, uglifier is the default compressor: https://github.com/rails/rails/blob/master/actionpack/lib/sprockets/railtie.rb#L52
Yes, uglifier is the default compressor: https://github.com/rails/rails/blob/master/actionpack/lib/sprockets/railtie.rb#L52 |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
ncri
Aug 28, 2011
Okay, I can confirm my app works fine with closure. Just wonder if it would be better to set rails to a default that works properly? I don't know why uglifier was chosen as a default, I assume there are good reasons for it. However, if it's currently broken it might make sense to replace it by a more robust default for now.
ncri
commented
Aug 28, 2011
Okay, I can confirm my app works fine with closure. Just wonder if it would be better to set rails to a default that works properly? I don't know why uglifier was chosen as a default, I assume there are good reasons for it. However, if it's currently broken it might make sense to replace it by a more robust default for now. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
@lautis, can you check this, please? |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
lautis
Aug 28, 2011
Contributor
This seems to happen also when using raw UglifyJS without any Ruby involved. To be more specific, there seems to be some issues with name mangling feature of UglifyJS as config.assets.js_compressor = Uglifier.new(:mangle => false)
works but results in larger file size.
This seems to happen also when using raw UglifyJS without any Ruby involved. To be more specific, there seems to be some issues with name mangling feature of UglifyJS as |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
elstudio
Aug 28, 2011
I can confirm that using closure-compressor rather than uglifier also fixes Prototype UJS and RJS in a production app (http://tally4.com).
Might closure-compressor be a better default for new Rails 3.1 apps?
Thanks @guilleiguaran and @lautis for the detective work!
elstudio
commented
Aug 28, 2011
I can confirm that using closure-compressor rather than uglifier also fixes Prototype UJS and RJS in a production app (http://tally4.com). Might closure-compressor be a better default for new Rails 3.1 apps? Thanks @guilleiguaran and @lautis for the detective work! |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
lautis
Aug 28, 2011
Contributor
Closure Compiler is definitely better engineered than UglifyJS, but there are caveats:
- it requires Java
- Closure Compiler imposes restrictions on the JS you're allowed to write[1]
- compared to UglifyJS it's slow[2]
[1] http://code.google.com/closure/compiler/docs/limitations.html
[2] http://www.peterbe.com/plog/comparing-google-closure-with-uglifyjs
Closure Compiler is definitely better engineered than UglifyJS, but there are caveats:
[1] http://code.google.com/closure/compiler/docs/limitations.html |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
ncri
Aug 28, 2011
Well, just read around a bit and Uglifier does seem to be a very good choice. See for example:
http://badassjs.com/post/971960912/uglifyjs-a-fast-new-javascript-compressor-for-node-js
http://blog.foxxtrot.net/2010/12/a-comparison-of-javascript-compressors.html
However, if this is issue is not fixed, it's a no go for me.
Has anyone compared file sizes of uglifier generated js with mangle = false vs. mangle = true?
If it's negligible, using config.assets.js_compressor = Uglifier.new(:mangle => false) could be a good option for now (looks hackish though :)
ncri
commented
Aug 28, 2011
Well, just read around a bit and Uglifier does seem to be a very good choice. See for example: http://badassjs.com/post/971960912/uglifyjs-a-fast-new-javascript-compressor-for-node-js However, if this is issue is not fixed, it's a no go for me. Has anyone compared file sizes of uglifier generated js with mangle = false vs. mangle = true? |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
guilleiguaran
Aug 28, 2011
Owner
@elstudio @ncri I'm not sure if is a good idea change the standard compressor now since 3.1.0 will be released very soon and the others supported compressors (YUI and Closure) require Java.
On the next version of Rails we will have plugins (ex uglifier-rails, closure-rails, ...) to set default compressors and we will be able to change the compressor only changing the plugin in the Gemfile. Also we will be able to add custom compressors easily.
@elstudio @ncri I'm not sure if is a good idea change the standard compressor now since 3.1.0 will be released very soon and the others supported compressors (YUI and Closure) require Java. On the next version of Rails we will have plugins (ex uglifier-rails, closure-rails, ...) to set default compressors and we will be able to change the compressor only changing the plugin in the Gemfile. Also we will be able to add custom compressors easily. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
NZKoz
Aug 28, 2011
Member
We could switch our own uglifier settings to turn mangle off. Has anyone been able to reproduce this with 'raw uglifier' and report it up stream?
We could switch our own uglifier settings to turn mangle off. Has anyone been able to reproduce this with 'raw uglifier' and report it up stream? |
added a commit
to guilleiguaran/rails
that referenced
this issue
Aug 28, 2011
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
lautis
Aug 28, 2011
Contributor
The problem exists with raw UglifyJS.
PrototypeJS tries to get argument names for JS functions using toString. Superclasses work by having a special argument name $super
. Unfortunately, the name mangling feature of UglifyJS does break this and replaces $super
with a
to save bytes.
Update: this can be easily given by adding a exception to Uglifier, i.e. config.assets.js_comperssor = Uglifier.new(:except => ["$super"])
. I'll add this as default in Uglifier.
The problem exists with raw UglifyJS. PrototypeJS tries to get argument names for JS functions using toString. Superclasses work by having a special argument name Update: this can be easily given by adding a exception to Uglifier, i.e. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
guilleiguaran
Aug 28, 2011
Owner
@lautis can you release soon a new version of Uglifier? (Rails 3.1 will be released very soon)
Upgrade to a new version of uglifier with the new default looks like the best solution for me.
@lautis can you release soon a new version of Uglifier? (Rails 3.1 will be released very soon) Upgrade to a new version of uglifier with the new default looks like the best solution for me. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
lautis
Aug 28, 2011
Contributor
Uglifier 1.0.2 has been released, it should fix this issue (the test app seems to work)
Uglifier 1.0.2 has been released, it should fix this issue (the test app seems to work) |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
guilleiguaran
Aug 28, 2011
Owner
@lautis thanks for your help, I just updated the app to use last version of uglifier and is working fine: http://blooming-autumn-295.herokuapp.com/
@elstudio @ncri upgrade uglifier, test again and let us know
@josh @spastorino @NZKoz this can be closed
@lautis thanks for your help, I just updated the app to use last version of uglifier and is working fine: http://blooming-autumn-295.herokuapp.com/ @elstudio @ncri upgrade uglifier, test again and let us know @josh @spastorino @NZKoz this can be closed |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
Wicked |
NZKoz
closed this
Aug 28, 2011
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
elstudio
Aug 28, 2011
Wonderful!
Works great for Tally4 -- Uglifier 1.0.2 makes compression work like a charm for PrototypeJS.
Thanks for the fix, @lautis! And the diagnosis outreach, @guilleiguaran!
elstudio
commented
Aug 28, 2011
Wonderful! Works great for Tally4 -- Uglifier 1.0.2 makes compression work like a charm for PrototypeJS. Thanks for the fix, @lautis! And the diagnosis outreach, @guilleiguaran! |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
ncri
commented
Aug 29, 2011
@lautis and @guilleiguaran: Yep, thanks a lot guys! |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
rhulse
Sep 13, 2011
Contributor
In my upgraded app the order of JS files is not as per the manifest.
// Gem jquery-rails
//= require jquery.min
//= require jquery-ui.min
//= require jquery_ujs
//= require admin/jquery-slug
//= require admin/jquery.inline_label.min
//= require admin/chefs
//= require admin/recipe
//= require admin/tag
//= require admin/categories
//= require admin/presenter
//= require admin/status_changes
//= require admin/galleries
//= require admin/date_picker
//= require admin/pictures
//= require admin/pjax
//= require admin/core
//= require admin/jquery.fileupload
//= require admin/jquery.fileupload-ui
//= require admin/presenter_selector
//= require admin/programme_selector
But the order is (as far as I could track through the minified code):
admin/chefs
admin/recipe
admin/categories
admin/status_changes
admin/galleries
admin/date_picker
admin/pictures
admin/presenter
admin/programme
jquery.min
etc
It does not seem to be alphabetical or length-based ordering, but appears to be consistent with each compile operation.
This is Rails 3.1.0 and Uglifier 1.0.2
In my upgraded app the order of JS files is not as per the manifest.
But the order is (as far as I could track through the minified code):
It does not seem to be alphabetical or length-based ordering, but appears to be consistent with each compile operation. This is Rails 3.1.0 and Uglifier 1.0.2 |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
@rhulse can you report this issue in Sprockets issue tracker? |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
Sure. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
rhulse
Sep 13, 2011
Contributor
Response: https://github.com/sstephenson/sprockets/issues/194
This is apparently uglifier, which according to the above thread is fixed.
Trying to narrow this down (by turning compression off to rule out sprockets) didn't work (see #3010)
Response: https://github.com/sstephenson/sprockets/issues/194 This is apparently uglifier, which according to the above thread is fixed. Trying to narrow this down (by turning compression off to rule out sprockets) didn't work (see #3010) |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
@lautis can you check this ^^^ ? please |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
rhulse
Sep 14, 2011
Contributor
FWIW, just tried swapping in Closure, and the order in the compiled file is now correct.
FWIW, just tried swapping in Closure, and the order in the compiled file is now correct. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
lautis
Sep 14, 2011
Contributor
The original bug report here is misleading and you might be slightly confused about the nature of JavaScript. The ordering was never fixed. In fact, it was not even the issue.
The source files are "shuffled" because UglifyJS does what your browser would do anyway: it hoists function definitions and variable declarations on top of the binding scope. As a result, it may seem that some files are moved on the top of the minified file.
function() {
function b() {}
// some code here
function a() {}
}
is equivalent to
function() {
function a() {}
function b() {}
// some code here
}
There are some quite wonky ways to shoot yourself in the foot with these. For a better description, I recommend that you read http://www.adequatelygood.com/2010/2/JavaScript-Scoping-and-Hoisting.
I'm not intimately familiar with the reasoning behind why this is done in UglifyJS. Maybe it allows few bytes to be saved, perhaps there are some performance reasons or it could just be easier to write. Doesn't really matter, it's a safe thing to do.
@rhulse Is there actually any issues with the minified code?
The original bug report here is misleading and you might be slightly confused about the nature of JavaScript. The ordering was never fixed. In fact, it was not even the issue. The source files are "shuffled" because UglifyJS does what your browser would do anyway: it hoists function definitions and variable declarations on top of the binding scope. As a result, it may seem that some files are moved on the top of the minified file.
is equivalent to
There are some quite wonky ways to shoot yourself in the foot with these. For a better description, I recommend that you read http://www.adequatelygood.com/2010/2/JavaScript-Scoping-and-Hoisting. I'm not intimately familiar with the reasoning behind why this is done in UglifyJS. Maybe it allows few bytes to be saved, perhaps there are some performance reasons or it could just be easier to write. Doesn't really matter, it's a safe thing to do. @rhulse Is there actually any issues with the minified code? |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
rhulse
Sep 14, 2011
Contributor
Thanks @lautis for taking the time to explain. It makes perfect sense.
Yes, part of the code stopped working correctly - a jquery UI list sort. It did work, but was adding dozens of extra elements each time anything was moved. Interestingly the code works fine if I use Closure instead. I didn't write most of the code, but if you think it might be an actual bug, I can do some digging around.
Thanks @lautis for taking the time to explain. It makes perfect sense. Yes, part of the code stopped working correctly - a jquery UI list sort. It did work, but was adding dozens of extra elements each time anything was moved. Interestingly the code works fine if I use Closure instead. I didn't write most of the code, but if you think it might be an actual bug, I can do some digging around. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
lautis
Sep 14, 2011
Contributor
It does sound like a bug in UglifyJS/Uglifier. If you can debug further and perhaps generate a minimal failing example, it'd help. Issues should probably be reported to UglifyJS.
Some pointers:
- using
eval
andfunction.toString()
likely lead to unexpected behavior when minified (though Closure is usually more anal about these) - try less aggressive options, e.g.
config.assets.js_compressor = Uglifier.new(:mangle => false, :squeeze => false)
It does sound like a bug in UglifyJS/Uglifier. If you can debug further and perhaps generate a minimal failing example, it'd help. Issues should probably be reported to UglifyJS. Some pointers:
|
ncri commentedAug 15, 2011
Without asset compression sprockets keeps the load order of my js files as specified in the manifest. However, when setting config.assets.compress = true the load order is ignored.