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

Turn off JS Minification for Heroku #238

Merged
merged 1 commit into from
Nov 12, 2015
Merged

Turn off JS Minification for Heroku #238

merged 1 commit into from
Nov 12, 2015

Conversation

seanpdoyle
Copy link
Contributor

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.

def config_js_compressor
production_config = "config/environments/production.rb"

inject_into_file production_config, before: "end\n" do <<-RUBY

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block body expression is on the same line as the block start.

@geoffharcourt
Copy link

@seanpdoyle to be clear, this is only disabling the Rails-side minification, correct? JS is still being minified by Ember?

After reading #123 that was my impression, but wanted to confirm my understanding.

@drapergeek
Copy link

What happens if I'm using Ember for part of my app but using jquery and a few other things for another area of my app? Isn't this eliminating the minification there?

@seanpdoyle
Copy link
Contributor Author

@geoffharcourt that's correct.

There are PRs (rondale-sc/ember-cli-rails-addon#12, rondale-sc/ember-cli-rails-addon#16) that disable minification on the addon side of things, but they do it automatically and implicitly, without giving the consumer the chances to make their decision.

There still a big decision to be made here, but I'd like to test it out on Heroku first to see how it goes.

@seanpdoyle
Copy link
Contributor Author

@drapergeek yes, it is.

Fortunately, since this is a generator, there will be diff noise.

That diff noise could be reverted, and at that point you could manually set minifyJS: false in your Ember app, or do nothing and minify both assets twice.

@seanpdoyle
Copy link
Contributor Author

@drapergeek Ideally, on a greenfield app (like the one Geoff and I are working) you'd keep your assets on the frontend.

@drapergeek
Copy link

@drapergeek Ideally, on a greenfield app (like the one Geoff and I are working) you'd keep your assets on the frontend.

That's an ideal situation. There are definitely cases where you want things to be split.

Fortunately, since this is a generator, there will be diff noise.

That's fine for our case but I worry that others won't see what is happening and they lose speed due to an oversight. Not sure how to rectify that issue while still fixing this problem though.

@geoffharcourt
Copy link

@seanpdoyle I think a change to the README to clarify would be helpful, but the code side of this looks good.

@seanpdoyle
Copy link
Contributor Author

@drapergeek would you prefer disabling minification in EmberCLI -- relying on the Asset Pipeline for minification?

@drapergeek
Copy link

@drapergeek would you prefer disabling minification in EmberCLI -- relying on the Asset Pipeline for minification?

Definitely not. Of the two options, I significantly prefer this.

This really doesn't affect me since I deploy with ember-cli-deploy anyway. I'd leave on minification for our other stuff. I'm just concerned about others who don't notice what is happening.

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
@seanpdoyle seanpdoyle merged commit 21f701f into master Nov 12, 2015
@seanpdoyle seanpdoyle deleted the sd-heroku-minify branch November 12, 2015 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants