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

Mixing Asset Pipeline Minification and Webpack 2 tree shaking breaks #699

Closed
justin808 opened this issue Jan 30, 2017 · 10 comments
Closed

Comments

@justin808
Copy link
Member

justin808 commented Jan 30, 2017

We need a custom sprockets exporter to optionally avoid minification of files created by webpack.

This PR contains the details on how to do this:
rails/sprockets#386

This is the code that might cause the double minification problem on the webpack side:

    new webpack.optimize.UglifyJsPlugin({
      compress: {
        screw_ie8: true,
        warnings: true,
      },
    }),

While we can turn off the minification on the webpack side, we'll probably lose tree shaking which reduces the bundle sizes.

Another approach would be to bypass sprockets entirely. However, that would not be suitable for a legacy app.

@justin808
Copy link
Member Author

justin808 commented Jan 31, 2017

Any Sprockets experts out there? I just ran into a problem with using Webpack v2 ulglification/minification and then passing the result in to the asset pipeline which also minifies. Double minification is breaking on lodash! Ouch!

So I've got, so I can't just bypass the asset pipeline. I'd like to have regular asset pipeline functionality, but have a list of files to skip minification. So this could go into assets.rb:

    Rails.application.config.assets.skip_minification += %w(foo.js bar.js)

What's the best way to do this that's super easy? Should I extend React on Rails? or create a new gem? Or file a PR on Sprockets?

While you can use your own compressor, you only get the string to compress. Maybe we could pass the second arg as a file name or file path?

http://guides.rubyonrails.org/asset_pipeline.html#using-your-own-compressor

class Transformer  
  def compress(string)    
    do_something_returning_a_string(string)  end
end

Another approach is to wrap the webpack files in a JS/CSS manifest file from Rails.
Here's an article on doing this: Selectively Disabling Rails Asset Compression.

However, this requires that I add a comment in a rails manifest file to then include the webpack minified file and the selective compressor would skip the compressed files.

vendor-webpack.js
(some name to not confuse with the vendor-webpack-bundle which is minified by Webpack)

/**
 * DO NOT REMOVE THIS COMMENT
 * It turns off compression of during the asset precompilation phase,
 * to avoid double minification from Rails after Webpack does it's thing.
 * no_asset_compression
 *
 **/
//= require vendor-webpack-bundle

and in assets.rb

class SelectiveAssetsCompressor < Uglifier
 def initialize(options = { })
   options.merge(comments: :all)
   super(options)
 end

 def compress(string)
   if string =~ /no_asset_compression/
     string
   else
     super(string)
   end
 end
end

Thoughts on this approach? Should I include the SelectiveAssetsCompressor inside of React on Rails, or just recommend pasting it into assets.rb, or even make a tiny gem?

@rupurt
Copy link
Contributor

rupurt commented Feb 3, 2017

@justin808 this would be solved by the approaches outlined in #704. Unfortunately it would only work for a decoupled Rails/Webpack setup where you're bypassing the asset pipeline. It wouldn't work if you needed to do a gradual transition where you're still compiling Webpack builds in sprockets.

@justin808
Copy link
Member Author

@rupurt So are you agreeing with me that an approach that lists files to exclude from minification is the way to go?

@rupurt
Copy link
Contributor

rupurt commented Feb 5, 2017

@justin808 that's one option, but if you use either of the approaches outline in #704 by building and serving assets out of public/assets you'll completely bypass this problem as it never needs to get compiled again by sprockets.

@chimame
Copy link

chimame commented Feb 27, 2017

Thanks for the wonderful gem.

Is it due to this problem that webpack does not upgrade to v2?
Since I am building with webpack in the first place, I think that it is not necessary to pass it through the Asset Pipeline again.
The webpacker has already responded as such.

However, because it is necessary for Server Side Rendering, we put it under assets and make selection not included in application.js.
I am making it like this.

@justin808
Copy link
Member Author

@chimame Yes -- no need for double inclusion in the asset pipeline. If you can help with a PR, code or doc, that describes what's needed, we'll all be very grateful!

@justin808
Copy link
Member Author

See rails/webpacker#139. Very soon, we won't be putting Webpack assets through the rails asset pipeline!

@chimame
Copy link

chimame commented Feb 28, 2017

👍

@justin808
Copy link
Member Author

Fixed with 8.0.0! 🎉

@justin808
Copy link
Member Author

As far as I know, the minification steps might break React on Rails 7.x apps, and avoiding double minification in v8 fixes the issue:

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

No branches or pull requests

3 participants