Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Cosmetic cleanup of generated Gemfile #9955

Merged
merged 2 commits into from Mar 27, 2013

Conversation

Projects
None yet
Contributor

rubys commented Mar 27, 2013

  • Remove obsolete/misleading comment about assets only being used production
  • Remove unnecessary group :assets
  • Eliminate blank lines if options[:skip_javascript] is not specified

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Mar 27, 2013

railties/lib/rails/generators/app_base.rb
GEMFILE
else
<<-GEMFILE.gsub(/^ {12}/, '')
- # Gems used only for assets and not required
- # in production environments by default.
- group :assets do
- gem 'sass-rails', '~> 4.0.0.beta1'
- #{coffee_gemfile_entry if options[:skip_javascript]}
- #{javascript_runtime_gemfile_entry(2) if options[:skip_javascript]}
- gem 'uglifier', '>= 1.0.3'
- end
+ # Gems used for assets
+ gem 'sass-rails', '~> 4.0.0.beta1'
+ gem 'uglifier', '>= 1.0.3'
+ GEMFILE
+ end
+
+ if options[:skip_javascript]
@rafaelfranca

rafaelfranca Mar 27, 2013

Owner

I think this should be unless

@rubys

rubys Mar 27, 2013

Contributor

Just to be clear, the lines it is replacing are:

#{coffee_gemfile_entry if options[:skip_javascript]}
#{javascript_runtime_gemfile_entry(2) if options[:skip_javascript]}

Are you suggesting that those lines are buggy?

rubys added some commits Mar 27, 2013

@rubys rubys Cosmetic cleanup of generated Gemfile
- Remove obsolete/misleading comment about assets only being used production
- Remove unnecessary group :assets
- Eliminate blank lines if options[:skip_javascript] is not specified
49c4af4
@rubys rubys Remove buggy and unnecessary logic
based on a discussion with @rafaelfranca
4e35dc0

@rafaelfranca rafaelfranca added a commit that referenced this pull request Mar 27, 2013

@rafaelfranca rafaelfranca Merge pull request #9955 from rubys/cleanup_app_gemfile
Cosmetic cleanup of generated Gemfile
d684de4

@rafaelfranca rafaelfranca merged commit d684de4 into rails:master Mar 27, 2013

Owner

jeremy commented on 4e35dc0 Mar 28, 2013

Broke AppGeneratorTest#test_javascript_is_skipped_if_required

/cc @rafaelfranca

Owner

rafaelfranca replied Mar 28, 2013

Thanks! It was fixed at 0417bc8

Contributor

alindeman commented on 49c4af4 Apr 13, 2013

If you precompile assets, these gems are not needed in production, right?

Owner

rafaelfranca replied Apr 14, 2013

Some gems can be needed like coffee-rails if you are using coffee templates, but yes, you may not need them in production

Contributor

alindeman replied Apr 14, 2013

Gotcha! Is that the reason for the change?

Owner

rafaelfranca replied Apr 15, 2013

Yes. This and the fact that now assets are not precompiled on demand in production anymore

Member

schneems replied Apr 23, 2013

@rafaelfranca Heroku is still running assets:precompile on Rails 4 during deploys. Should we be changing how we handle assets?

Owner

rafaelfranca replied Apr 23, 2013

not precompiled on demand in production

@rafaelfranca First I thought it was an oxymoron, then that the rake task was removed (refuted in a late comment), if you don't mind me asking, what does that really mean? And why would that motivate a change to make engines available in production?

I'm sorry bothering rails core people, rails look so busy, but the migration guide wasn't particularly enlightening on this "issue" either.

Owner

rafaelfranca replied Apr 30, 2013

Means that if you have that gems in production environment in 3.2.x and forget to precompile, Rails will do exactly what it does in development, precompile the assets that was requested. This is not true anymore in Rails 4, so if you don't precompile the assets using the tasks you will get a 404 when the assets are requests.

What really motivate that change was to make possible to use CoffeeScript templates on production. But, since there is not problem to include that assets related gems in production anymore we choose to remove that assets group.

I'm sorry bothering rails core people, rails look so busy, but the migration guide wasn't particularly enlightening on this "issue" either.

No problem at all. I hope I could explain better. Feel free to ask more questions and feel free to upgrade the migration guide.

Owner

rafaelfranca replied Apr 30, 2013

You keep saying "anymore", was it a problem to include asset related gems in production before because you couldn't know if you forgot to precompile something?

For some people, yes.

How will you know now that forgot to precompile if it you already have an old version?

You will not. But I don't think you had a way to know this in Rails 3.2

Contributor

rubys replied Apr 30, 2013

One thing that may not be obvious in this discussion: while the recommendation remains that you precompile assets, the recommendation as to how you do that has changed. Previously it was:

bundle exec rake assets:precompile

With Rails 4, the recommendation now is:

RAILS_ENV=production bundle exec rake assets:precompile

ches replied Oct 29, 2013

It still strikes me that memory-bloat-by-default with things like therubyracer in production is poor, particularly if precompiling is core's recommendation and a widely-held best practice at this point. Many people likely never stop to consider that that's even happening if they came to Rails after the assets group removal or never gave much thought to it serving that purpose -- at least a suggestive comment in the generated Gemfile might be a good idea.

It's now a yak shave for developers to work around this for gems they know are unneeded in production web or worker processes since loading the group was removed from the precompile task. I'm basically now including this as boilerplate in new apps:

namespace :assets do
  # Override sprockets-rails task to put back assets group require, so as to
  # avoid memory bloat in web processes :-/
  task :environment do
    Bundler.require(:assets)
    Rake::Task['environment'].invoke
  end
end

plus restoring Bundler.require(*Rails.groups(assets: %w[development test])) to config/application.rb. What a mess.

FYI, du reports therubyracer as 17MB on my machine, and it doesn't use autoload. We're not using any CoffeeScript view templates.

I don't fully understand what the advantage of removing the assets group from the rails Gemfile entirely is, because the reality is that many gems that were getting put into that group don't need to be included in production if you are precompiling your assets before deploying. If the concern, as @rafaelfranca says, is allowing CoffeeScript templates in production, then why not just move the cofee-rails gem out of the assets group by default?

True, some gems that provide assets to your asset pipeline may need to be used when in production, but in that case it should be the responsibility of the author of the gem to specify whether the gem can be placed in the assets group on the Gemfile or not? Echoing what @ches said, it doesn't seem to be necessary to include gems in production whose sole purpose is to be leveraged during the precompile process.

Contributor

dnagir replied Nov 8, 2013

I'm totally with the @batter. Don't see the reason why all of the assets now need to be part of production.

Even in Rails 3.2 we could move the ones we needed in production at runtime out of the assets group.

And I really like this comment from SO question which makes so much sense. Killing assets group was a workaround that masked a real problem.

Rails assuming that assets have been precompiled is an argument for keeping the assets group, not getting rid of it (if assets are precompiled, then these gems aren't needed in production and shouldn't be included by bundler). And yes, maybe you would use a gem like coffee-rails in production... but that was the case in Rails 3 too, right? And Rails 3 put coffee-rails in the assets group, by default. So why the change for Rails 4?

To add to this, I do keep the assets group in my Rails 4 apps and precompile in production RAILS_ENV=production RAILS_GROUPS=assets bundle exec rake assets:precompile (which is also what EngineYard does by default, thanks God).

Owner

jeremy replied Nov 8, 2013

Using a separate Bundler group to carefully manage autorequire behavior was an awkward, defensive measure. We appreciate that it had side effects that you prefer. It also had side effects that were undesirable. This can be discussed back and forth perpetually. If you have a strong case, make it in the form of a compelling pull request instead of piling on a dead-end comment thread. PDI ❤️

ches replied Nov 8, 2013

@jeremy I can appreciate the "let some code do the talking" sentiment, but I think the fact that this "dead-end comment thread" has developed at all is evidence that the reasoning for the change wasn't made clear and explicit anywhere else but here (and calling this thread "clear" on the matter is a bit of a stretch even at this point).

When I started using Rails 4, this matter stood out to me and I searched around for information about the history and found a few references that all ultimately lead here, and IMO the message from core is still mixed -- people hastily cite different reasons for the change, and they don't all seem sound (@rafaelfranca's reasoning about precompilation versus the quote @dnagir pointed out above, for instance).

A patch to change this would be fairly trivial, the difficulty would be almost entirely in convincing core why it should be accepted. No one likes to spend time putting a pull request together that's going to be mercilessly rejected without it being packaged with strong reasoning, and without this thread we're in the dark about what we're even reasoning against. Maybe a mailing list would have been a more appropriate forum (and maybe it still is), but apparently the conversation needed to start somewhere.

ches replied Nov 8, 2013

And thank you all for answering us here, and as always for the work that you do ❤️

Owner

jeremy replied Nov 8, 2013

@ches Cheers! Trouble is, the commentary above is expecting "reasoning for the change" to take the form of "why was this feature I like removed?" It won't, because it wasn't seen a feature. The need for a brittle workaround was better satisfied elsewhere so it was removed.

If you're not a fan of loading extra libs, I recommend you remove Bundler.require from config/application.rb entirely. That's the root of these unwanted library autorequires. Require the libraries you need when you need them, where you need them, and you'll meet the aim you're looking to achieve here without without autorequire groups at all.

Sorry to jump on this thread, but I'm looking for a bit more clarity.

Require the libraries you need when you need them, where you need them, and you'll meet the aim you're looking to achieve here without without autorequire groups at all.

Is there new guidance on this topic? I'd expect Rails to encourage autoloading by default (despite the obvious trade-offs). Am I wrong in expecting that? Or is that still what's happening, and it's just being extended to include all of the asset-related gems?

Instead of drastically changing how my team handles our gem dependencies, I'd be interested in solving the problem that the :assets group solved (albeit inadvertently, apparently, since it wasn't seen as a feature). Should we just keep the :assets group in our Gemfile and then precompile with this?:

RAILS_ENV=production RAILS_GROUPS=assets bundle exec rake assets:precompile

ches replied Mar 19, 2014

@smudge FWIW, to continue working in "the Rails 3 way" as far as not loading compilation-only asset libraries into your production app processes goes: yes, keep your :assets group and make the two changes I mentioned above, or be explicit with the RAILS_GROUPS env variable if you prefer. This has been working swimmingly for me for awhile now.

Keep in mind that, as @rafaelfranca noted above, Rails 4 changes behavior to no longer compile a missing asset on-demand and that of course still applies. That's probably irrelevant if you precompile anyway, except maybe in a case where you have some asset files that aren't included in application.js/css and you forget to add them to config, e.g.

# config/application.rb
config.assets.precompile += %w(admin.css admin.js)

There are certainly some advantages to removing Bundler.require as @jeremy suggests and which you've seen Myron Marston discuss in the post DHH responded to. My team so far feels that it will be too tedious though, and since Rails continues to generate Bundler.require(:default, Rails.env) we are not swimming against the current (except to alter that for the :assets group...). I'm not Rails core, so bear in mind I have no insight into "official" guidance beyond what rails new does.

I hope that helps.

Contributor

betesh replied Jul 29, 2015 edited

@ches's workaround in comment 49c4af4 is incomplete in some situations.

When the assets:environment task is run, initializers have already run, so simply requiring the gems in the assets group won't run the initializers defined in them. I found that I had to pull together a list of all Rails Engines & Railties, filter out whichever ones had already run (i.e. from gems not in the assets group), and then run the rest of them. This is what finally worked.

namespace :assets do
  task :environment do
    Bundler.require(:assets)
    already_initialized = Rails.application.railties.collect(&:class)
    railties = Rails::Railtie.subclasses
    engines = Rails::Engine.subclasses
    (railties + engines - already_initialized).each do |engine|
      engine.instance.run_initializers(:default, Rails.application)
    end
  end
end

Also, you need to put this task in your Rakefile ABOVE Rails.application..load_tasks

I know this thread is long dead, but I feel like I've been reading all the same things and finding none of the answers a year later. I've asked a question on StackOverflow here - is the "Rails way" as it stands right now to load all development dependencies in production even though everything has been precompiled?

Did anything ever come of this discussion? Any resources anyone can point me to to address this?

smudge replied Sep 28, 2016 edited

@tobymurray-nanometrics - I responded to your StackOverflow question.

Someone should lock this thread. Future visitors - refer to the comments above and to that SO question, as well as this one.

Member

schneems replied Sep 29, 2016

Done, thanks.

@jrafanie jrafanie added a commit to jrafanie/manageiq that referenced this pull request May 11, 2017

@jrafanie jrafanie Rails assets group was removed with rails 4.0
It was removed here:
rails/rails#9955

The rails 5.1.0 template has just `Bundler.require(*Rails.groups)` here:
https://github.com/rails/rails/blob/v5.1.0/railties/lib/rails/generators/rails/app/templates/config/application.rb#L21
42fb9cd

@jrafanie jrafanie added a commit to jrafanie/manageiq that referenced this pull request May 15, 2017

@jrafanie jrafanie Rails assets group was removed with rails 4.0
It was removed here:
rails/rails#9955

The rails 5.1.0 template has just `Bundler.require(*Rails.groups)` here:
https://github.com/rails/rails/blob/v5.1.0/railties/lib/rails/generators/rails/app/templates/config/application.rb#L21
e297332

@jrafanie jrafanie added a commit to jrafanie/manageiq that referenced this pull request May 15, 2017

@jrafanie jrafanie Rails assets group was removed with rails 4.0
It was removed here:
rails/rails#9955

The rails 5.1.0 template has just `Bundler.require(*Rails.groups)` here:
https://github.com/rails/rails/blob/v5.1.0/railties/lib/rails/generators/rails/app/templates/config/application.rb#L21
c74b957

@jrafanie jrafanie referenced this pull request in ManageIQ/manageiq May 15, 2017

Merged

Rails assets group was removed with rails 4.0 #15095

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