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

Only compile non-js/css under app/assets by default #7968

Merged

Conversation

josh
Copy link
Contributor

@josh josh commented Oct 16, 2012

This has been bugging me forever.

I have a few packaged asset gems that have other loose files under their load path like README.mds. Those get copied over and digested to public/assets. This happens more often with bower packages.

The other thing is that these libraries have many optional parts. You don't always want all the images and other stuff copied over unless you say so.

I'm suggesting that we only auto compile "loose" files under app/asset paths. This would include railtie gems. So if you do app/assets in your gem, those are auto included. lib/assets and vendor/assets are by hand. I think that this models Rails' default autoloading strategy of app/ is autoloaded and autorequired, while lib/ and vendor/ have to be explicit.

Depends on #7964 being merged first.

/cc @dhh

@steveklabnik
Copy link
Member

I think that this models Rails' default autoloading strategy of app/ is autoloaded and autorequired, while lib/ and vendor/ have to be explicit.

I like the symmetry here, too. First thing I thought of.

@dhh
Copy link
Member

dhh commented Oct 17, 2012

This makes sense to me 👍

On Oct 16, 2012, at 3:25 PM, Joshua Peek notifications@github.com wrote:

This has been bugging me forever.

I have a few packaged asset gems that have other loose files under their load path like README.mds. Those get copied over and digested to public/assets. This happens more often with bower packages.

The other thing is that these libraries have many optional parts. You don't always want all the images and other stuff copied over unless you say so.

I'm suggesting that we only auto compile "loose" files under app/asset paths. This would include railtie gems. So if you do app/assets in your gem, those are auto included. lib/assets and vendor/assets are by hand. I think that this models Rails' default autoloading strategy of app/ is autoloaded and autorequired, while lib/ and vendor/ have to be explicit.

Depends on #7964 being merged first.

/cc @dhh

You can merge this Pull Request by running:

git pull https://github.com/josh/rails only-precompile-app-assets-by-default
Or view, comment on, or merge it at:

#7968

Commit Summary

• Only compile non-js/css under app/assets by default
File Changes

• M railties/lib/rails/application/configuration.rb (2)
Patch Links

https://github.com/rails/rails/pull/7968.patch
https://github.com/rails/rails/pull/7968.diff

Reply to this email directly or view it on GitHub.

@guilleiguaran
Copy link
Member

:shipit:

/cc @rafaelfranca

rafaelfranca added a commit that referenced this pull request Oct 18, 2012
Only compile non-js/css under app/assets by default
@rafaelfranca rafaelfranca merged commit 0dfebbc into rails:master Oct 18, 2012
@rafaelfranca
Copy link
Member

@josh could you add the CHANGELOG entry?

@stevenharman
Copy link
Contributor

So if you do app/assets in your gem, those are auto included. lib/assets and vendor/assets are by hand.

When you say, "by hand," how does that work? Are you suggesting adding the lib and/or vendor paths to the config.assets.precompile setting? (Currently broken per: rails/sprockets-rails#36) Or is there a better way to limit the scope of things added by hand.

For example, if I need to vendor a jQuery plugin which also has CSS, a font face, and an image sprite, I'd add the .js and .css to vendor/assets/javascripts and vendor/assets/stylesheets. I would also vendor the sprites and fonts in vendor/assets/images and vendor/assets/fonts, respectively. Adding the entire vendor/assets path seems overkill, but manually specifying each asset individually seems tedious (though that might be by design).

@hongliang-goudou
Copy link

This is a REALLY BAD change. Very confusing. Many css libraries have to tell their users to add a special line to their config/application.rb:

config.assets.precompile += %w(*.png *.jpg *.jpeg *.gif)

@shamabbas
Copy link

This worked for me finally after wasting so many hours researching.

config/application.rb
config.assets.precompile += %w(*.png *.jpg *.jpeg *.gif,
"fontawesome-webfont.ttf",
"fontawesome-webfont.eot",
"fontawesome-webfont.svg",
"fontawesome-webfont.woff")

# config/application.rb
config.assets.precompile << Proc.new do |path|
  if path =~ /\.(css|js)\z/
    full_path = Rails.application.assets.resolve(path).to_path
    app_assets_path = Rails.root.join('app', 'assets').to_path
    if full_path.starts_with? app_assets_path
      puts "including asset: " + full_path
      true
    else
      puts "excluding asset: " + full_path
      false
    end
  else
    false
  end
end

environment/production.rb
config.serve_static_assets = true

Then I ran bundle exec rake assets:precompile RAILS_ENV=production and pushed it.

@fny
Copy link
Contributor

fny commented Aug 22, 2013

If we're going for true parallelism, there ought to be a manifest file of sorts for images and fonts. Something just feels awkward about listing umpteen image and font paths in the app configuration.

@jejacks0n
Copy link
Contributor

They load in development, but when you precompile them they aren't generated? The disparity between the different configurations (environments) shouldn't be there IMO. I'm fine with this change, but I think it should be symmetrical and behave in an understandable and consistent way.

It's pretty clear by the issue being referenced on so many other issues that it's caused a considerable amount of confusion and issue churn. I'm happy to jump in and contribute to that if there's some consensus.

@rafaelfranca
Copy link
Member

@jejacks0n we are already on this rails/sprockets-rails#84

sbasir added a commit to sbasir/spree_product_zoom that referenced this pull request Dec 30, 2013
xdite added a commit to xdite/sprockets-rails that referenced this pull request Jan 28, 2014
JDutil pushed a commit to spree-contrib/spree_product_zoom that referenced this pull request Feb 19, 2014
moved thumbnails to use deface to track better with spree_frontend

updated gemspec to work with master branch

tracking to 2-1-stable

fixed single quote typo

version bump

Update _image.html.erb

Update spree_product_zoom.gemspec

use asset_path to point to the zoom image for assets precompile

explicitly set the assets to include in precompile due to rails/rails#7968

start zoom gallery on selected image, display variant images too
@searls
Copy link
Contributor

searls commented Jun 6, 2014

I agree with @steveklabnik's sentiment at the top that the symmetry with lib/vendor in ruby code is nice, but AFAICT in practice this is actually quite dissimilar when you consider that merely "requiring" an asset isn't actually enough to get it copied over during rake assets:precompile.

That is to say, if I have a font in vendor/assets/fonts/ and I "require it" from some css.erb file in vendor by way of the asset_path helper (e.g. url(<%= asset_path('foo.woff') %>)), then what I would expect as a user is that the font will be included during precompilation. Of course, it isn't, and instead I have to reference that particular asset from my application's configuration. As a result, the nice symmetry that seems to have been the motivation for this PR is lost.

(I decided to just give in and start putting all non-CSS, non-JS 3rd party assets into app/assets to save myself the hassle and leaking references)

searls added a commit to INN/impaq-me that referenced this pull request Jun 6, 2014
This worked fine in dev and blew up in prod, thanks to this PR:

rails/rails#7968

Leaving implementors to either list every vendor asset they want

My comments here: rails/rails#7968 (comment)
shopjoy added a commit to shopjoy/spree_product_zoom that referenced this pull request Jun 14, 2014
@jcarlson
Copy link
Contributor

This is an unfortunate opinion Rails has taken on this topic. This change has made it near impossible to use Bower to manage any front-end library that includes images or fonts (Bootstrap, jQuery-UI). The CSS and JavaScript work fine, but no matter how I configure my app, I cannot get the vendored images to end up where the CSS (relatively) expects they will be.

Part of that is my doing, because I'm inlining the Boostrap CSS (into /assets/application.css) instead of serving it from it's "vendored" path of /assets/bootstrap/dist/css/bootstrap.css, but doesn't that defeat the purpose of Sprockets if I have to make all my vendored stylesheets separately precompiled packages?

I don't know guys... this is one of the more maddening problems I've dealt with recently and I lost a lot of time on it because there is a TON of documentation about how to configure Bower to work with Rails... and those instructions just don't work or don't account for images and fonts included with Bower packages.

As of now my options are 1) use gems for assets (which are often out of date) or 2) go back to manually cultivating the vendored assets. Blegh.

@matthewd
Copy link
Member

@jcarlson I'm not sure how closely this PR relates to the problem you're describing.

Anyway, suggestions for improvement are always welcome.

@jcarlson
Copy link
Contributor

Recent versions of Sprockets seem to be aware of bower.json package descriptors. If package publishers are correctly declaring all the resources their package exposes, images and fonts included, is there any reason why Sprockets or Rails couldn't automatically copy over those binary resources, even if they are in vendor/assets or lib/assets? The bower.json file could serve as the "manifest" suggested by @fny above. As an example: https://github.com/twbs/bootstrap/blob/master/bower.json#L16-L24

rubydog pushed a commit to rubydog/flatui-free-rail that referenced this pull request Sep 7, 2014
afeld added a commit to afeld/code_cruise that referenced this pull request Dec 27, 2014
because of rails/rails#7968, all files under
app/assets/ are automatically compiled. this gets around the issue of
files without extensions being included by sprockets.
cappert pushed a commit to cappert/failcascade that referenced this pull request Jan 24, 2016
@jabberfest
Copy link

jabberfest commented Apr 15, 2016

With RAILS_ENV=development:

With this change I get the expected error message of Sprockets::Rails::Helper::AssetNotPrecompiled if trying to do something like

<%= javascript_include_tag 'foo' %>

in my view. This is assuming foo.js is in "vendor/assets/javascripts"

This is also assuming I did NOT set the following:

 Rails.application.config.assets.precompile += ['foo.js']

However, I can still access the foo.js resource if not using a rails view helper.

If I have something like

<script src="assets/foo.js"></script>

in my view helper.

The weird thing is foo.js never appears in the public directory since it's not precompiled, yet it is still available accessing the url without a view helper.

You would expect to not have the resource available. I only bring this up because it could give a developer a false sense of app stability. The app would fail to run in production because the foo.js resource would not be compiled.

I've seen this problem happen more when requireJS is used without a javascript_path helper. The app would work in development, then fail in production when the resource could not be loaded.

Even though this ticket is closed, hopefully I can get some feedback. I'll wait a week or so, and open a new ticket to get some thoughts.

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

Successfully merging this pull request may close these issues.

None yet