rails / sprockets-rails

Sprockets Rails integration
MIT License
574 stars 244 forks source link

Inability to compile nondigest and digest assets breaks compatibility with bad gems #49

Closed gwcoffey closed 11 years ago

gwcoffey commented 11 years ago

For instance, the jquery_mobile-rails gem does not use image-url in its CSS. This is a bug really, but it "worked fine" in Rails 3. I assume they will fix the gem eventually. But in the meantime, in Rails 4, it is broken and as far as I can tell there's no way to work around it. The simple short term fix is to compile both digest and nondigest assets for this gem, but I don't see any way to do that. I can apparently only turn digest off or on globally for the application.

A rake assets:precompile:nondigest task or something similar would provide a workaround. But it seems to be gone now.

Is there a good way to deal with this, or do I just have to drop the gem and manage jquery mobile manually outside the asset pipeline, which is a pain?

pelargir commented 11 years ago

+1

Removing generation of non-digest files also makes is impossible to have static 404 error pages that use the same images, JS, and CSS as the remainder of the site (without making duplicate non-digest copies of those files in /public). But then I end up with two copies of the same file in my app.

It's really unfortunate that this feature would be yanked without an explanation of how to deal with these situations.

rafaelfranca commented 11 years ago

From the README: Only compiles digest filenames. Static non-digest assets should simply live in public/.

pelargir commented 11 years ago

I saw that. So please explain how I'm supposed to reference my precompiled application.css file from my static 404 error page? Am I supposed to copy the precompiled file into /public and remove the fingerprint from the filename? So then, if I change the file in the future, I have to precompile, copy, and rename all over again? That's silly.

guilleiguaran commented 11 years ago

You can use a rake task like this for your case:

task non_digested: :environment do
  assets = Dir.glob(File.join(Rails.root, 'public/assets/**/*'))
  regex = /(-{1}[a-z0-9]{32}*\.{1}){1}/
  assets.each do |file|
    next if File.directory?(file) || file !~ regex

    source = file.split('/')
    source.push(source.pop.gsub(regex, '.'))

    non_digested = File.join(source)
    FileUtils.cp(file, non_digested)
  end
end
ryana commented 11 years ago

FWIW, I disagree with the resolution of this issue. I'm all for software being opinionated, but I shouldn't have to create a rake task like this in order to compile assets without the digest. It is my application after all ;) The bigger issue from my perspective is that this breaks applications on Heroku that are doing things like serving JS widgets that can't have digests in the name.

ryana commented 11 years ago

https://gist.github.com/ryana/6049833

A monkeypatch for current version of Sprockets cc: @pelargir

tikaro commented 11 years ago

I also disagree with the resolution of this issue: "So please explain how I'm supposed to reference my precompiled application.css file from my static 404 error page?" is more or less the use case I have - a centralized Rails single-sign-on utility provides precompiled CSS via absolute URLS to other client applications.

rafaelfranca commented 11 years ago

If you need to generate assets without digest so you have to do this on your own.

tikaro commented 11 years ago

After talking this over, my big-chair developer decided to handle this by having Rails return a 301 redirect FROM the old, non-digested CSS location, TO whatever the latest digested CSS location is. That way, the client sites get to use a non-changing location and the rails app doesn't have to bake everything twice.

rafaelfranca commented 11 years ago

This was exactly because we remove the undigest version. The Rails app had to bake the compilation twice.

guilleiguaran commented 11 years ago

@tikaro like https://github.com/sikachu/sprockets-redirect (it's for Rails 3.x but should be easy to port to Rails 4)

ryana commented 11 years ago

@tikaro clever, but you'll want to 302 instead of 301. 301s are permanent and some browsers will cache them.

tikaro commented 11 years ago

@ryana good point, re: 302 and 301; I think I over-described what we've done, and may have gotten it wrong. Cloudfront is involved, too. Perhaps I can coax him in here so he can describe what he did.

Intrepidd commented 11 years ago

Hey guys,

Just wanted to say that I liked the middleware solution so I updated @sikachu's code for rails 4 : https://github.com/Intrepidd/sprockets-redirect

I'll issue a PR in no time.

Cheers

will-r commented 11 years ago

This seems like a real problem to me. It is now impossible to refer to rails-generated assets from outside the rails application. @Intrepidd's middleware is an elegant patch but sadly no use if you're delivering the assets through a CDN.

Would you accept a pull request that adds a config.assets.nondigest setting? There's no need to bake twice: we can just write duplicate files during Manifest#compile and remove them again during Manifest#remove. That's what I'm doing in a monkeypatch now and it seems to work well.

ilyakatz commented 11 years ago

I concur, no having non-digest files without an alternatives is pretty annoying. Maybe I missed something, but what problem is it solving that non-digest files are no longer generated?

ilyakatz commented 11 years ago

For those of you looking for a way to make it work with error pages, I adopted a post on how to make error pages come from asset pipeline (added the bit about non-digest assets) https://gist.github.com/ilyakatz/6346020

scottillogical commented 11 years ago

I also have a use case for the ability to precompile assets without digests - we need to reference a javascript file from a 3rd party app, and it would be nice to use the asset pipeline - just without the digests!

alexspeller commented 11 years ago

Agreed, @will-r it seems easy to not compile assets twice and still have this built in.

tilsammans commented 11 years ago

I know it's unproductive to pile on but I really agree with all the commenters here. A decision was made to remove this and by gosh we're gunna stick to that decision no matter what it seems. While the use case for non-digest assets is completely valid IMO:

  1. download almost any javascript library, like fancybox
  2. put jquery.fancybox.min.js in vendor/assets/javascripts
  3. put related images in vendor/assets/images
  4. include the main javascript in your application.js
  5. watch all your images not loading.

What are we supposed to do in this case? Put fancybox in public? So by-pass the asset pipeline altogether. This does not seem like an improvement to me, in fact it's a regression. We had something that worked just fine, now we are back to loading a bunch of javascripts separately. We can ditch the asset pipeline altogether?

If I am missing something, I sure would like to be educated. But putting potentially a dozen scripts in public is no kind of solution, frankly I am shocked it is even being suggested.

alexspeller commented 11 years ago

Yeah I haven't yet seen a single rails 4 project that doesn't need to work around this. This decision should be reconsidered IMO.

turlockmike commented 10 years ago

This was a bad decision and is negatively affecting multiple apps i'm working on for no reason. Rails should not be this opinionated.

ryana commented 10 years ago

@rafaelfranca you ever gonna come comment on this issue again? I've moved on by using the monkeypatch above, but I find it funny that I get emails from this thread every few days and you guys are just ignoring it. I understand you have jobs and probably busy, but you're stewards of this project. If you can't handle it, responding with a simple "ok cool submit a pull request and we'll merge it in" would suffice. I'm sure the community would pick up the slack.

will-r commented 10 years ago

This is what I'm using at the moment. It's a little crude and doesn't tackle the slightly harder problem of what files to remove, but as you can see the added computation is very slight. All we have to do is build another filename and write that file too.

module Sprockets
  class Manifest

    def compile(*args)
      unless environment
        raise Error, "manifest requires environment for compilation"
      end

      paths = environment.each_logical_path(*args).to_a +
        args.flatten.select { |fn| Pathname.new(fn).absolute? if fn.is_a?(String)}

      paths.each do |path|
        if asset = find_asset(path)
          files[asset.digest_path] = {
            'logical_path' => asset.logical_path,
            'mtime'        => asset.mtime.iso8601,
            'size'         => asset.bytesize,
            'digest'       => asset.digest
          }
          assets[asset.logical_path] = asset.digest_path

          target = File.join(dir, asset.digest_path)
          undigested_target = File.join(dir, asset.logical_path)

          if File.exist?(target)
            logger.debug "Skipping #{target}, already exists"
          else
            logger.info "Writing #{target}"
            asset.write_to target
            asset.write_to undigested_target
            asset.write_to "#{target}.gz" if asset.is_a?(BundledAsset)
            asset.write_to "#{undigested_target}.gz" if asset.is_a?(BundledAsset)
          end

          save
          asset
        end
      end

    end
  end
end
alexspeller commented 10 years ago

Hot from the mines, there's now a gem for this:

https://github.com/alexspeller/non-stupid-digest-assets

gem 'non-stupid-digest-assets'

Problem solved.

ryana commented 10 years ago

lol

jeremy commented 10 years ago

We've had no trouble using digested assets throughout, and we've also updated our Rails 3 apps to do the same.

When an external app needs to reference a specific static asset, you can use a separate rake task that symlinks yourasset-<digest>.css to yourasset.css after precompile.

When you want to pull in an external lib that has its own scripts, images, and css, you can vendor the assets in public/somelib-v1/* an include the javascript in the asset pipeline. Then you're still free to put far-future expires on all static assets. When you upgrade to somelib v2, just add a public/somelib-v2/ directory.

Stable assets that can be publicly cached—and CDNed—forever. Now that's non-stupid :grin:

turlockmike commented 10 years ago

I don't have a problem using digested assets, but I do have a problem with rails being so opinionated about it and removing the choice. The biggest problem being with 3rd party javascript libraries or using my own app as a javascript source. Not every app is the same and that's why these choices need to exist.

rubytastic commented 10 years ago
mtylty commented 10 years ago

@turlockmike I have the exact same usecase you presented.

tilsammans commented 10 years ago

Hi @jeremy, I appreciate your explanation on how we are supposed to use external libs now. We've been forced to not use public for anything besides a favicon, so this is I think why you're seeing the pushback. Also, front ends web servers will not have been configured to serve from /public with optimal headers. And with the version numbers hardcoded in paths, some libs will require changes to the JS/CSS file every time you upgrade them. It's going to be a painful experience.

It would be incredibly helpful to have a writeup in the sprockets README or some kind of documentation how to use third-party libs. This issue is going to sting a lot more developers. It would be even better if we could come up with a formal, optimized approach on how to include the wide variety of libs we have in the wild.

Is anyone in this thread aware of a better solution? Personally I'm intrigued by the middleware that redirects to the correct asset. I just fear that doing this in rack middleware is a performance problem.

alexspeller commented 10 years ago

@tilsammans https://github.com/rails/sprockets-rails/issues/49#issuecomment-24835988

tilsammans commented 10 years ago

@alexspeller that works as long as you don't upgrade your lib or can live with possible breakage from cached assets.

Which is cool by me but arguably not the best solution we can think of.

rubytastic commented 10 years ago

If one would only need certain files to be non digested which seems the case many times ( you don't need all the files to be non digested and copied over an extra time ) how about an paramater added to image_tag like

= image_tag('/assets/logo2.png', :digested => false)

I tried this initialiser code hacked together below from several sources but it fails on rails 4, this would be ideal solution for those of us who only want to make some files available

module ActionView
  module Helpers
    module AssetTagHelper

      def image_tag_with_digested(source, options={})
        digested = options.delete(:digested)
        digested = true if digested.nil?

        unless digested
          source = image_tag_without_digested(source, options)
          source.sub(/(\-[a-f0-9]+)\.(png|gif|jpg)"/, '.\2"').html_safe
        else
          image_tag_without_digested(source, options)
        end
      end
      alias_method_chain :image_tag, :digested
    end
  end
end
rubytastic commented 10 years ago

@alexspeller Tried you gem for production but it failed to create any non digested files.

alexspeller commented 10 years ago

@rubytastic I have tested it and it does create non-digested files. Please file an issue on the project and give more details if you can't get this to work.

shioyama commented 10 years ago

+1 I would also really like to see this reversed, as an option. for now using @alexspeller's gem, does the trick in a jiffy!

rubytastic commented 10 years ago

Confirm @alexspeller gem is working correctly. I had other local issues earlier on.

alexey commented 10 years ago

gem 'non-stupid-digest-assets' convert only one css file, js files are not. rails 4 + ruby 2.cap deploy

alexspeller commented 10 years ago

@alexey please open an issue on the gem and give details https://github.com/alexspeller/non-stupid-digest-assets/issues

alexey commented 10 years ago

@alexspeller After investigation i found that its do create non digest files, but another gem (assets_sync) is not uploading them all.

thanks for nice gem

sarmiena commented 10 years ago

@rafaelfranca seems to me like there are sensible and logically sound arguments against the change to sprockets. I believe a lot of people here are upset because you haven't addressed any of the comments objectively, but rather with a heavy-handed "just do what we say".

To recap arguments against non-digest to be included:

  1. Static pages (404s) will require you to create duplicate images (e.g. logos) from assets/images and put them into public/assets - that's a headache to remember later on down the road if you want to update images.
  2. Many people don't have control over css since they're using jQuery libraries that know nothing about the asset pipeline. Going into a plugin's css and changing url(..) to image-url("..") each time a library gets updated goes against Rails' philosophy of "web development that doesn't hurt".
  3. Some js libraries (like ckeditor) do tricky stuff like inject assets on the client side at runtime (ugly, but it's what it does)
  4. There hasn't been an explanation of why something so seemingly benign has been removed. So... why was this change so strongly supported?
radar commented 10 years ago

To address your points, @sarmiena:

  1. Reference static images from these pages. I do not know why you would precompiling images for these things. Stick them in public/images and be done with it.
  2. There are plenty of "bridging" gems out there for the major plugins and libraries. I would suggest you use those if you want to save yourself the hassle of find+replace operations.
  3. I don't know enough about how ckeditor works to fairly answer this point, but I think it's probably in a similar boat to point no. 2. There's probably a gem for it.
  4. That's one for the maintainers to answer.
sarmiena commented 10 years ago

@radar I can understand where you're coming from, but: 1) Having duplicate images in public/assets goes against DRY, which is at a higher level than rails opinions 2) In the event that plugins don't have gems, people shouldn't feel compelled to commit to maintaining a gem. That development hurts. 3) There are gems that do handle ckeditor. 2 in fact. However there is a huge community around it, and why should we mess with their tests to modify it for a gem? 4) Ditto

turlockmike commented 10 years ago

@radar For a lot of applications, this change makes absolute sense. Referencing a file which is cached by the browser/http does cause issues where a user might have an expired version of the asset but the cache header was not set appropriately.

For other applications, rails serves as a host for a set of javascript files which are used by third parties. In this case, given the impossibility of constantly asking users to update their script tags to reflect the new file hash, the best solution has been to have two copies of the file after compilation. One with the md5 hash and one without. If sprockets intends to support these types of applications, then I believe there should be an option to precompile an undigested version of the assets if desired.

It really comes down to the scope of the library and defining what it is for. If this gem is not meant to support certain types of applications out of the box, then it should probably be documented and rails should probably consider a replacement since i'm fairly certain rails would like to support those types of applications.

As far as third party javascript files like ckeditor or other javascript libraries which expect relative files to exist, perhaps a solution which allows you to pick files to be undigested using a whitelist would be the best solution. It would make it difficult enough that you would only whitelist files that you explicitly need to whitelist and asset pipeline can generally assume everything else is digested.

alexspeller commented 10 years ago

@radar

  1. Reference static images from these pages. I do not know why you would precompiling images for these things. Stick them in public/images and be done with it.

Because static 404/500 pages almost always need to look the same as the rest of the site, i.e. sharing images, css and potentially javascript. Having to duplicate all of your sites assets for these pages is not a solution.

  1. There are plenty of "bridging" gems out there for the major plugins and libraries. I would suggest you use those if you want to save yourself the hassle of find+replace operations.

Forcing the use of bridging gems is a not a good long term solution to this issue. You should be able to use arbitrary javascript libraries with the rails asset pipeline.

  1. I don't know enough about how ckeditor works to fairly answer this point, but I think it's probably in a similar boat to point no. 2. There's probably a gem for it.

Same answer as (2)

The asset pipeline clearly works for some people and also clearly does not work for other people. You seem to be someone in the latter group. Don't use the asset pipeline if it doesn't work for you

It seems like it would be possible to greatly increase the number of people the asset pipeline works for by reverting this change, whilst at the same time not causing any negative consequences to others.

I know rubygems counts are not entirely accurate but my gem has received nearly 800 downloads in less than 3 weeks. It certainly seems like a lot of people have this problem.

sarmiena commented 10 years ago

@alexspeller I can't speak for the other 799, but you got one from me! :)

rubytastic commented 10 years ago

Yes please listen to us developer, stepping back a little bit from your ( well deserved celebrity superman status )

rafaelfranca commented 10 years ago

TL;DR yes, we choose what we think is best for the direction of the project. And the reasons is here in @radar's comment and @jeremy's comment.

We never said this gem have to solve every problem in the world, and we don't think it should. If you have a different problem that is not solved by this gem, it is your job to solve your problem. And we already give you a lot of suggestion in this thread about how to solve some problems you may have.

The majority of Rails applications don't need non-digest assets and we don't want to keep this code in the gem since it will not used by the majority of our users. It is all about maintenance cost.

rafaelfranca commented 10 years ago

TL;DR 2, this rake task would solve almost all your problems: https://github.com/rails/sprockets-rails/issues/49#issuecomment-20535134