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

Use uglifier-rails to set default js compressor for Rails #2335

Closed
wants to merge 1 commit into from

Conversation

guilleiguaran
Copy link
Member

uglifier-rails: https://github.com/guilleiguaran/uglifier-rails

It can be moved to Rails Github account if you want

@guilleiguaran
Copy link
Member Author

This can be included before 3.1 release?

cc @spastorino @josevalim

@guilleiguaran
Copy link
Member Author

@jeremy This is updated for master and can be cherry-picked to 3-2-stable branch

@josevalim
Copy link
Contributor

Please, if we are going to ship with uglifier-rails let's not make it depend on specific Rails versions. We don't want to have to release yet another gem on every rails release. We should simply depend on "uglifier-rails", "~> 4.0" and uglifier-rails should just accept any Rails version after 3.2.

@guilleiguaran
Copy link
Member Author

@josevalim updated, uglifier-rails now depends on railties >= 3.2.0

@spastorino
Copy link
Contributor

I was saying to Guillermo that seems excessive to have another gem for just this. All that uglifier-rails does is https://github.com/guilleiguaran/uglifier-rails/blob/master/lib/uglifier/rails/railtie.rb#L8.
Why not trying to push this to the uglifier gem itself or letting the user config this option in his own config files?.

@josevalim
Copy link
Contributor

I agree with @spastorino but @jeremy was running into some issues iirc. There's a tradeoff here. It is up to you guys to decide.

@guilleiguaran
Copy link
Member Author

@josevalim @spastorino this isn't going in Rails anymore, I will handle this in sprockets-rails and without creating a separated plugin (an initializer should be enough)

Closing.

@spastorino
Copy link
Contributor

cool

@arunagw
Copy link
Member

arunagw commented Apr 3, 2012

👍 @guilleiguaran

@josevalim
Copy link
Contributor

@guilleiguaran :+:1 agreed, thanks!

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