add note on minification #124

egh · 2015-03-21T18:53:19Z

rwz · 2015-03-21T18:55:06Z

README.md

+
+in your `config/environments/production.rb` file. Disabling in Ember
+is the better route if it works for you, but it may be more fragile in
+the case of javascript errors.


I don't get this part. How can it be more fragile? Care to elaborate?

Your guess is as good as mine. :) I get this error in an app:

rake aborted! ExecJS::ProgramError: Unexpected token: punc (:) (line: 1, col: 10, pos: 10) Error at new JS_Parse_Error (/tmp/execjs20150321-24731-1hyu7atjs:2359:10623) at js_error (/tmp/execjs20150321-24731-1hyu7atjs:2359:10842) at croak (/tmp/execjs20150321-24731-1hyu7atjs:2359:19086) at token_error (/tmp/execjs20150321-24731-1hyu7atjs:2359:19223) at unexpected (/tmp/execjs20150321-24731-1hyu7atjs:2359:19311) at semicolon (/tmp/execjs20150321-24731-1hyu7atjs:2359:19784) at simple_statement (/tmp/execjs20150321-24731-1hyu7atjs:2359:22580) at /tmp/execjs20150321-24731-1hyu7atjs:2359:20274 at /tmp/execjs20150321-24731-1hyu7atjs:2359:19957 at block_ (/tmp/execjs20150321-24731-1hyu7atjs:2359:24599) (in /home/egh/c/tahi/tmp/ember-cli-cd98e71c-ad5a-4b81-85de-a24baaf26e75/assets/tahi/ember-data.js.map)new JS_Parse_Error ((execjs):2359:10623)

when I disable minifyJS in ember and use uglify in rails. I'll try to track it down, but it's pretty unclear to me.

Interesting. Will investigate this before merging the PR.

rwz · 2015-03-21T18:55:35Z

Other than my comment above LGTM.

egh · 2015-03-21T19:16:26Z

Thanks!

rondale-sc · 2015-03-21T22:23:06Z

Disabling in ember-cli can be done automatically by the addon.

[Edit]: @egh I think the readme should still be updated to show that you can disable this in Rails land. And I'd be interested in working with you on a PR for ember-cli-rails-addon also, if you wanna.

Should be very similar to this line:

app.options.minifyJS = false;

egh · 2015-03-22T20:58:56Z

@rondale-sc That might be useful. I'm not sure if there is a one size fits all solution, though. Whether minification is better done in ember or rails might depend on the nature of your app.

Another issues is source maps. I'm still trying to get source maps to work in production with an ember-cli-rails app.

[edit] Perhaps a good start would be to add a warning if you are minifying your code twice.

rzane · 2015-04-23T16:22:16Z

@egh I was attempting to use the minifyJS option, but I was getting the same issue when running assets:precompile. I definitely didn't want to disable Rails asset compression. The compile was choking on ember-data.js.map.

I found this issue: emberjs/ember-rails#428
It links to this comment: jimsynz/faye-rails#15 (comment)

It seems that tossing this in an initializer fixes the issue:

Rack::Mime::MIME_TYPES.merge!(".map" => "text/plain")

It should probably be application/json, though. See http://stackoverflow.com/questions/18809567/what-is-the-correct-mime-type-for-min-map-javascript-source-files#18809594.

However, there's also this issue: sstephenson/sprockets#400. It seems to suggest that map files shouldn't be asset path at all.

stevehanson · 2015-07-27T21:32:39Z

@rzane adding the MIME_TYPES line in to my initializers fixed the issue for me. Thanks for sharing your solution. I'm now able to have:

minifyJS: {
  enabled: false
}

and keep the Rails JS compressor turned on. Builds are taking me about 20-30 seconds.

seanpdoyle · 2015-10-23T19:34:47Z

We're sorry that this issue has sat for so long.

The project is now maintained by thoughtbot, and we're declaring an issue bankruptcy of sorts.

We're closing this issue. If what you've described is still a problem, please comment on this issue and we'll reopen.

@rondale-sc were we able to modify https://github.com/rondale-sc/ember-cli-rails-addon to handle this automatically?

Thanks for your patience.

egh · 2015-10-23T21:43:52Z

It is still an issue: javascript assets are being minified by ember and by rails asset pipeline in the default setup. This is a problem because a) it breaks sourcemaps and b) it is slow.

How to replicate: do the following (as recommended by the README):

rails new foo
cd foo
echo "gem 'ember-cli-rails', github: 'thoughtbot/ember-cli-rails'" >> Gemfile
bundle install
rails generate ember-cli:init
mkdir frontend
cd frontend
ember init
npm install --save-dev [email protected]
cd ..

Now, running: RAILS_ENV=production rake assets:precompile will minify the javascript twice: once when building with ember, and then again in the rails asset pipeline.

This behavior can be tested by suspending (^z) the assets:precompile after ember prints the temp directory where it has generated assets: (e.g., home/egh/foo/tmp/ember-cli-e9c79eb4-c0e9-44f2-8219-66516b413c13/apps/frontend) and copying those temp files somewhere and then comparing against the final output of the rails asset pipeline.

seanpdoyle · 2015-10-23T21:45:43Z

@egh thanks for following up.

Hopefully we'd be able to set this automatically through https://github.com/rondale-sc/ember-cli-rails-addon. @rondale-sc is that something that's in the works?

If not, @egh would you mind opening a PR against that project?

egh · 2015-10-23T22:09:38Z

@seanpdoyle A decision would need to made about whether it is better for ember or the rails asset pipeline to handle this. I'm not really sure if there is a universally better choice, or if there is, which it is. But I'm happy to do what I can.

seanpdoyle · 2015-10-23T22:14:09Z

My opinion is that it would be better for the ember addon to handle turning uglification / minification off.

Leaving it enabled Rails-side lets developers continue to minify/uglify their server-side assets.

Would you agree?

When precompiling assets in production, we ensure that we are not minifying the JavaScript assets twice. Duplicating minification can slow down assets precompilation considerably. Closes [tricknotes/ember-cli-rails#124][124] and [tricknotes/ember-cli-rails#123][123]. [123]: tricknotes/ember-cli-rails#123 [124]: tricknotes/ember-cli-rails#124

egh · 2015-11-02T23:56:31Z

That sounds reasonable. I'll file a pull request.

egh · 2015-11-02T23:57:29Z

It seems you were way ahead of me. :) I guess we can close this pull request, then?

Thank you for your work on this!

seanpdoyle · 2015-11-03T02:55:59Z

@egh I've gone back on forth on where (and whether or not) minification should happen.

In the meantime, I think your PR make a lot of sense.

Would you mind updating the reference to Brocfile.js to ember-cli-build.js?

Also, I think it makes sense to omit

Disabling in Ember is the better route if it works for you, but it may be more fragile in the case of javascript errors.

since we really don't know which side of the pipeline is better suited for minifcation.

What do you think?

egh · 2015-11-03T04:00:57Z

Oh, sorry, I thought rondale-sc/ember-cli-rails-addon#16 was merged. I will update this PR.

egh · 2015-11-03T04:05:13Z

Updated.

seanpdoyle · 2015-11-03T12:50:14Z

Merged in ef0fc67

seanpdoyle · 2015-11-03T12:50:18Z

Thanks!

Given the outcome of [#123] and [#124], we don't take a stand on where minification should happen. However, to make sure that Heroku deployments don't take too long, the generator can disable Rails-side minification. The benefit of doing this in the generator (as opposed to automatically in [the addon]) is that the change will introduce diff noise, which is explicit and easy to back out of. [#123]: #123 [#124]: #124 [the addon]: https://github.com/rondale-sc/ember-cli-rails-addon/blob/2f2a47efe6e81a2bcd43ec9ff0f89fc18bed5a03/index.js#L25-L32

When precompiling assets in production, we ensure that we are not minifying the JavaScript assets twice. Duplicating minification can slow down assets precompilation considerably. Closes [tricknotes/ember-cli-rails#124][124] and [tricknotes/ember-cli-rails#123][123]. [123]: tricknotes/ember-cli-rails#123 [124]: tricknotes/ember-cli-rails#124

Given the outcome of [#123] and [#124], we don't take a stand on where minification should happen. However, to make sure that Heroku deployments don't take too long, the generator can disable Rails-side minification. The benefit of doing this in the generator (as opposed to automatically in [the addon]) is that the change will introduce diff noise, which is explicit and easy to back out of. [#123]: #123 [#124]: #124 [the addon]: https://github.com/rondale-sc/ember-cli-rails-addon/blob/2f2a47efe6e81a2bcd43ec9ff0f89fc18bed5a03/index.js#L25-L32

rwz reviewed Mar 21, 2015
View reviewed changes

egh mentioned this pull request Mar 23, 2015

Chore/speed up heroku compile Aperta-project/Aperta#924

Merged

egh mentioned this pull request Aug 31, 2015

Don't build the Ember app twice Aperta-project/Aperta#1603

Merged

seanpdoyle closed this Oct 23, 2015

seanpdoyle reopened this Oct 23, 2015

add note on minification

84807e6

seanpdoyle closed this Nov 3, 2015

seanpdoyle mentioned this pull request Nov 3, 2015

Turn off JS Minification for Heroku #238

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add note on minification #124

add note on minification #124

egh commented Mar 21, 2015

rwz Mar 21, 2015

egh Mar 21, 2015

rwz Mar 21, 2015

rwz commented Mar 21, 2015

egh commented Mar 21, 2015

rondale-sc commented Mar 21, 2015

egh commented Mar 22, 2015

rzane commented Apr 23, 2015

stevehanson commented Jul 27, 2015

seanpdoyle commented Oct 23, 2015

egh commented Oct 23, 2015

seanpdoyle commented Oct 23, 2015

egh commented Oct 23, 2015

seanpdoyle commented Oct 23, 2015

egh commented Nov 2, 2015

egh commented Nov 2, 2015

seanpdoyle commented Nov 3, 2015

egh commented Nov 3, 2015

egh commented Nov 3, 2015

seanpdoyle commented Nov 3, 2015

seanpdoyle commented Nov 3, 2015

add note on minification #124

add note on minification #124

Conversation

egh commented Mar 21, 2015

rwz Mar 21, 2015

Choose a reason for hiding this comment

egh Mar 21, 2015

Choose a reason for hiding this comment

rwz Mar 21, 2015

Choose a reason for hiding this comment

rwz commented Mar 21, 2015

egh commented Mar 21, 2015

rondale-sc commented Mar 21, 2015

egh commented Mar 22, 2015

rzane commented Apr 23, 2015

stevehanson commented Jul 27, 2015

seanpdoyle commented Oct 23, 2015

egh commented Oct 23, 2015

seanpdoyle commented Oct 23, 2015

egh commented Oct 23, 2015

seanpdoyle commented Oct 23, 2015

egh commented Nov 2, 2015

egh commented Nov 2, 2015

seanpdoyle commented Nov 3, 2015

egh commented Nov 3, 2015

egh commented Nov 3, 2015

seanpdoyle commented Nov 3, 2015

seanpdoyle commented Nov 3, 2015