Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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
  • Loading branch information
@rubys
rubys committed Mar 27, 2013
1 parent e743850 commit 49c4af4
Showing 1 changed file with 14 additions and 17 deletions.
31 changes: 14 additions & 17 deletions railties/lib/rails/generators/app_base.rb
View file
Expand Up @@ -179,26 +179,23 @@ def assets_gemfile_entry

gemfile = if options.dev? || options.edge?
<<-GEMFILE.gsub(/^ {12}/, '')
# Gems used only for assets and not required
# in production environments by default.
group :assets do
gem 'sprockets-rails', github: 'rails/sprockets-rails'
gem 'sass-rails', github: 'rails/sass-rails'
#{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 'sprockets-rails', github: 'rails/sprockets-rails'
gem 'sass-rails', github: 'rails/sass-rails'
gem 'uglifier', '>= 1.0.3'
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]
gemfile += <<-GEMFILE.gsub(/^ {12}/, '')
#{coffee_gemfile_entry}
#{javascript_runtime_gemfile_entry(2)}
GEMFILE
end

Expand Down

24 comments on commit 49c4af4

@alindeman
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@rafaelfranca
Copy link
Member

Choose a reason for hiding this comment

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

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

@alindeman
Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha! Is that the reason for the change?

@rafaelfranca
Copy link
Member

Choose a reason for hiding this comment

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

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

@schneems
Copy link
Member

Choose a reason for hiding this comment

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

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

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented on 49c4af4 Apr 23, 2013 via email

Choose a reason for hiding this comment

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

@mmacedo
Copy link

Choose a reason for hiding this comment

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

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.

@rafaelfranca
Copy link
Member

Choose a reason for hiding this comment

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

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.

@mmacedo
Copy link

@mmacedo mmacedo commented on 49c4af4 Apr 30, 2013 via email

Choose a reason for hiding this comment

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

@rafaelfranca
Copy link
Member

Choose a reason for hiding this comment

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

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

@rubys
Copy link
Contributor Author

@rubys rubys commented on 49c4af4 Apr 30, 2013

Choose a reason for hiding this comment

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

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
Copy link

@ches ches commented on 49c4af4 Oct 29, 2013

Choose a reason for hiding this comment

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

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.

@batter
Copy link

@batter batter commented on 49c4af4 Oct 29, 2013

Choose a reason for hiding this comment

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

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.

@dnagir
Copy link
Contributor

@dnagir dnagir commented on 49c4af4 Nov 8, 2013

Choose a reason for hiding this comment

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

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).

@jeremy
Copy link
Member

@jeremy jeremy commented on 49c4af4 Nov 8, 2013

Choose a reason for hiding this comment

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

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
Copy link

@ches ches commented on 49c4af4 Nov 8, 2013

Choose a reason for hiding this comment

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

@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
Copy link

@ches ches commented on 49c4af4 Nov 8, 2013

Choose a reason for hiding this comment

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

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

@jeremy
Copy link
Member

@jeremy jeremy commented on 49c4af4 Nov 8, 2013

Choose a reason for hiding this comment

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

@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.

@smudge
Copy link

@smudge smudge commented on 49c4af4 Mar 19, 2014

Choose a reason for hiding this comment

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

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
Copy link

@ches ches commented on 49c4af4 Mar 19, 2014

Choose a reason for hiding this comment

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

@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.

@betesh
Copy link
Contributor

@betesh betesh commented on 49c4af4 Jul 29, 2015 •
edited

Choose a reason for hiding this comment

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

@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

@tobymurray-nanometrics
Copy link

Choose a reason for hiding this comment

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

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
Copy link

@smudge smudge commented on 49c4af4 Sep 28, 2016 •
edited

Choose a reason for hiding this comment

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

@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.

@schneems
Copy link
Member

Choose a reason for hiding this comment

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

Done, thanks.

Please sign in to comment.