jwhitley / requirejs-rails

RequireJS support for your Rails 3 or 4 application
MIT License
592 stars 201 forks source link

Assets not minified after upgrade to 0.9.3 #237

Closed agis closed 9 years ago

agis commented 9 years ago

Hello!

After upgrading from 0.9.1 to 0.9.3 on my Rails 3.2 app, I've noticed that assets are not minified anymore. I'm posting the snippet from a specific file, application_desktop.js (lines 1-40)

In 0.9.1 (expected output):

define('vendor/modernizr_transitions',[], function() {
function b(e) {
s.cssText = e;
}
function w(e, t) {
return b(prefixes.join(e + ";") + (t || ""));
}
function E(e, t) {
return typeof e === t;
}
function S(e, t) {
return !!~("" + e).indexOf(t);
}
function x(e, t) {
for (var n in e) {
var r = e[n];
if (!S(r, "-") && s[r] !== undefined) return t == "pfx" ? r : !0;
}
return !1;
}
function T(e, t, n) {
for (var r in e) {
var i = t[e[r]];
if (i !== undefined) return n === !1 ? e[r] : E(i, "function") ? i.bind(n || t) : i;
}
return !1;
}
function N(e, t, n) {
var r = e.charAt(0).toUpperCase() + e.slice(1), i = (e + " " + f.join(r + " ") + r).split(" ");
return E(t, "string") || E(t, "undefined") ? x(i, t) : (i = (e + " " + l.join(r + " ") + r).split(" "), T(i, t, n));
}
var e = "2.6.1", t = {}, n = document.documentElement, r = "modernizr", i = document.createElement(r), s = i.style, o, u = {}.toString, a = "Webkit Moz O ms", f = a.split(" "), l = a.toLowerCase().split(" "), c = {}, h = {}, p = {}, d = [], v = d.slice, m, g = {}.hasOwnProperty, y;
!E(g, "undefined") && !E(g.call, "undefined") ? y = function(e, t) {
return g.call(e, t);
} : y = function(e, t) {

In 0.9.3 (incorrect output):

define('vendor/modernizr_transitions',[],function(){

  var version = '2.6.1',

  Modernizr = {},

  docElement = document.documentElement,

  mod = 'modernizr',
  modElem = document.createElement(mod),
  mStyle = modElem.style,

  inputElem  ,

  toString = {}.toString,    omPrefixes = 'Webkit Moz O ms',

  cssomPrefixes = omPrefixes.split(' '),

  domPrefixes = omPrefixes.toLowerCase().split(' '),

  tests = {},
  inputs = {},
  attrs = {},

  classes = [],

  slice = classes.slice,

  featureName,

  _hasOwnProperty = ({}).hasOwnProperty, hasOwnProp;

  if ( !is(_hasOwnProperty, 'undefined') && !is(_hasOwnProperty.call, 'undefined') ) {
    hasOwnProp = function (object, property) {
      return _hasOwnProperty.call(object, property);
    };
  }
  else {
    hasOwnProp = function (object, property) {
      return ((property in object) && is(object.constructor.prototype[property], 'undefined'));
    };
  }

Any ideas?

carsomyr commented 9 years ago

@agis- Have you tried making the jump to 0.95?

agis commented 9 years ago

Sorry, forgot to mention that this happens in 0.9.5 and 0.9.8 too. It's just that 0.9.3 is the first "bad" version.

carsomyr commented 9 years ago

@agis- Is that a top-level module specified in requirejs.yml?

agis commented 9 years ago

The setup is the one described in #236, so yes, it's a top-level module:

# config/requirejs.yml
locale : 'en'
optimize : 'none'
paths:
  jquery.datepicker:  'vendor/jquery/plugins/jquery.datepicker'
  jquery.jcrop:       'vendor/jquery/plugins/jquery.jcrop'
modules :
  - name: 'application_desktop'

  - name: 'application_tablet'

  - name: 'application_mobile'

# ...
agis commented 9 years ago

Sorry, the working version is 0.9.1, not 0.9.2 (updated the initial post accordingly).

I've been thinking to try upgrading only require.js inside the working requirejs-rails (0.9.1) to version to 2.1.11 and see what happens. If the problem pops up, then it's an issue on the requirejs side.

Thing is, I cannot really test with 0.9.2 because the compilation never ends in that version, seems like it's entering and infinite loop (actually the run_rjs task is processing the same files again and again).

carsomyr commented 9 years ago

Have you double checked your use of the optimizer?

agis commented 9 years ago

I've always had the r.js optimizer disabled (optimize: 'none' in requirejs.yml) in order to have the rjs files (which, as mentioned before, live under app/assets/javascripts/rjs/) also optimized by my global asset pipeline options that I use:

  # config/environments/production.rb

  if defined? Uglifier
    config.assets.js_compressor = Uglifier.new(
      :mangle    => true,
      :toplevel  => false,
      :copyright => false,
      :beautify  => true,
      :beautify_options => {
        :indent_level => 0
      }
    )
  end

This setup was working fine with 0.9.0 and 0.9.1 but not with 0.9.3 apparently.

agis commented 9 years ago

@carsomyr I've confirmed that https://github.com/jwhitley/requirejs-rails/commit/fbc1e5c1f550d9a2be816e94f73626c53039c404 is the culprit. I've tried with 0.9.3 and without that line and it works fine: all assets are minified properly.

The thing is that by disabling the js_compressor in the initialization process, settings that were defined before it are overriden and they seem to persist all the way through assets:precompile:all and assets:precompile:nondigest which runs after requirejs:precompile:all.

This prevents certain setups like mine, which want to use a unified compressor in their asset pipeline (as described in https://github.com/jwhitley/requirejs-rails/issues/237#issuecomment-114812629)and not the one that comes with r.js.

This override wasn't happening prior to 0.9.3 that the compressor was disabled by a rake task.

Also I've noticed that with the problematic version, only assets that are defined as modules in my requirejs.yml are not minified.

Not sure this is the actual issue though and can't really tell without some background on the reason of that change.

This may also be related to #155.

agis commented 9 years ago

Hey @carsomyr, any feedback on this? It's really a blocker for upgrading to Rails 4.

Thanks!

carsomyr commented 9 years ago

@agis- Where does the compressor in your workflow come into play? Care to explain in depth?

agis commented 9 years ago

Sure.

I'm on Rails 3.2.22 and requirejs-rails 0.9.0. The RequireJS optimizer is totally disabled:

# config/requirejs.yml

optimize: 'none'
# ...

Instead, I have Uglifier set globally for the whole asset pipeline:

# config/environments/production.rb

# Ensure this only runs in the :assets bundler group
if defined? Uglifier
  config.assets.js_compressor = Uglifier.new(
    :mangle    => true,
    :toplevel  => false,
    :copyright => false,
    :beautify  => true,
    :beautify_options => {
      :indent_level => 0
    }
  )
end

The flow is this:

  1. I run $ bundle exec rake assets:precompile which executes...
  2. requirejs:precompile:all and then
  3. assets:precompile:all

(this is what happens when I deploy also, except step (1) which is automatically executed by capistrano).

So the compression/minification of assets happens (as far as I understand) during step (3), once and for all the assets I have.

My assets are located in the usual locations and any RequireJS-specific files are located in app/assets/javascripts/rjs/, therefore I also set the baseUrl accordingly:

# config/initializers/requirejs_path_override.rb

# Override the baseUrl value used in the build_config parameter to
# locate the temporary folder where compiled scripts put (from coffeescript compiler)
# This workaround is only needed for usage with r.js driver of requirejs-rails gem
if ENV['RAILS_GROUPS'] == "assets"
  requirejs = Rails.application.config.requirejs
  requirejs.build_config['baseUrl'] = "#{Rails.root}/tmp/assets/rjs"
end

This setup works fine in requirejs-rails 0.9.0 and 0.9.1, but not in 0.9.3 or later, because of https://github.com/jwhitley/requirejs-rails/commit/fbc1e5c1f550d9a2be816e94f73626c53039c404: assets are not minified, as explained in https://github.com/jwhitley/requirejs-rails/issues/237#issue-90605095.

Update: I've noticed that in the working setup (0.9.0) when requirejs:precompile:all runs (and before rake:assets:precompile), the generated assets are in fact minified. For example, the original source file:

# app/assets/javascripts/rjs/application_desktop.js is the original, uncompressed file

define [
  "common"
  "utils/fb_api"
  ], (common, infobox, uservoice, fb) ->
    return {
      init: ->
        $(document).ready ->
          ## INIT COMMON
          common.init()

          ## FOCUS THE SEARCH_BAR
          $('body#index #search_bar_input').focus()

          ## INIT USER VOICE
          uservoice.init()

          ## INIT FB BUTTON
          fb.load_api()

          ## INIT INFOBOX
          infobox.init()

          return
        return
    }

The temporarily generated file, after requirejs:precompile:all runs and before rake:assets:precompile:

# tmp/assets/rjs/application_desktop.js minified even though the compressor is turned to off in requirejs.yml

(function() {
define([ "common", "utils/fb_api" ], function(e, t, n, r) {
return {
init: function() {
$(document).ready(function() {
e.init(), $("body#index #search_bar_input").focus(), n.init(), r.load_api(), t.init();
});
}
};
});
}).call(this);;

Not sure if this is expected, since I've turned the "optimizer" of requirejs to none.

agis commented 9 years ago

@carsomyr Hey, any insights? Thanks!

carsomyr commented 9 years ago

@agis- Hang in there. Looking to devote 3 consecutive nights this week to looking into requirejs-rails issues more deeply.

agis commented 9 years ago

@carsomyr I'm grateful and willing to help any way I can! I'm currently upgrading from Rails 3.2.x to 4.0.x so I can provide insight while in the process of doing so.

Thanks :bow:

carsomyr commented 9 years ago

@agis- Upgrading to Rails 4 may very well fix your issue. I'd recommend stripping down your app to the point where you can demonstrate the problem is occurring in Rails 4. This exercise in teasing out essentials may even help you along in your overall migration path.

agis commented 9 years ago

@carsomyr I see. Upgrading requirejs-rails after upgrading to Rails 4.0 maybe sounds a better idea given that, apparently, there isn't a single requirejs-rails version that supports both 3.2 and 4. It's just that I was hoping to upgrade before, so that the upgrade to 4.0 would be transparent, as far as requirejs is concerned.

I have a long way for Rails 4 yet (maybe even a month), so it'll be a while before I get to the asset pipeline.

carsomyr commented 9 years ago

@agis- Here's your problem: A while back users were complaining that requirejs-rails was double-compressing their assets. One of the compressors had to give; in this case, it was the Rails compressor. If you'd like, you're welcome to file a pull request that:

  1. Ignores the RequireJS compressor.
  2. Uses whatever compressor Rails has registered to compress inlined modules.

I believe that this is a better solution because it's more Rails-y.

agis commented 9 years ago

One of the compressors had to give; in this case, it was the Rails compressor.

@carsomyr Can you point me to the relevant commit that made requirejs-rails to disable the Rails compressor? Was it https://github.com/jwhitley/requirejs-rails/commit/fbc1e5c1f550d9a2be816e94f73626c53039c404?

If not, it'd be great if we could have the reasoning behind that commit.

carsomyr commented 9 years ago

@agis- Take a look at this PR. The reasoning for the commit 4ea9141 is to prevent double compression: Obfuscated assets should not be processed in a way that obfuscates them further. Also, RequireJS's purpose is to logically glue a bunch of JavaScripts together. Compressing JS before it even hits RequireJS's dependency solver is just bad. That's why I proposed ignoring the r.js compressor and instead using the asset pipeline's compressor after JavaScripts have been inlined.

agis commented 9 years ago

@carsomyr Thanks for explaining. I'd be interested on preparing a PR that will switch to the asset pipeline compressor instead of the r.js compressor, but there are certain issues blocking me to do so.

This means I'm locked at 0.9.5 and cannot proceed to 0.9.6 cause of incompatibilities with Sprockers 2 (and I assume that will be more troubles on the way with 0.9.7 and later).

When I resolve these issues and upgrade to 0.9.9, I'd be able to work on this.