Use precompiled assets if possible

[gucki]

See #55 (comment)

Works for me with rails 3.2.12 ... :)

[coveralls]

Changes Unknown when pulling ace8dd9 on gucki:master into * on fphilipe:master*.

View Details

[fphilipe]

If the assets are precompiled, then they're stored inside the public folder, right? Then why not just return nil and let the FileSystemLoader load the file? Or in other words, skip the asset pipeline if live asset compilation is disabled.

[fphilipe]

I was thinking, that this could be refactored as follows.

• Remove CacheLoader and let CSSHelper handle the caching. It already is responsible for storing the cached file, thus it seems weird to have this circular dependency.
• Have the following loaders:
  1. AssetPipelineLoader: Loads the asset from the asset pipeline if enabled and live compilation available.
  2. FileSystemLoader: Loads the asset from the filesystem. This should work for precompiled assets from the asset pipeline that are stored in public and for "classical" assets that don't use the asset pipeline.
  3. AssetHostLoader: Loads the asset from the asset host. This should work for precompiled assets that are stored on a CDN and not inside public.

I'd like to include a rails app in the tests to make sure this really works.

Any thoughts on this?

Pinging @sbleon since he was the original author.

[fphilipe]

Copy of @sbleon's comment on #55:

@fphilipe Sorry for the delay, and thanks for your recent thoughts.

Honestly, I've been stalled on this because I can't quite understand why my changes make my app work on Heroku.

I think you're right about using FileSystemLoader for precompiled assets. The outstanding question is: in what order should we try the strategies?

Heroku makes things complicated, because they turn assets.compile and serve_static_assets on. However, they don't make the assets group of gems available at runtime. With premailer using its current strategy order, this leads to crashing when compiling stylesheets that rely on gems in the assets group (compass, sass, etc). So, for my use case at least, it seems like we should try FileSystemLoader first and AssetPipelineLoader second.

Can anyone think of a case where using FileSystemLoader first will not work?

[fphilipe]

@sbleon I think the order I suggested in my previous comment should work. AssetPipelineLoader would return nil if the assets are precompiled, i.e. not available. Don't you think that would work?

[sbleon]

Not on Heroku. Heroku injects a Rails plugin
that
forces asset compilation to be on, so premailer-rails will always try to
compile assets, even if they're precompiled. Hence my wanting to try
FileSystemLoader first.

[fphilipe]

Well, I think it would be possible as it is done in this pull request:

def assets_precompiled?
  !::Rails.configuration.assets.compile rescue false
end

But indeed, it might be wiser to first check the file system and after that check for the asset pipeline and after that perform a request to the asset host.

[sbleon]

That's the setting that Heroku forces to true, so assets_precompiled?
should always return false on Heroku.

I'm glad you're on board with checking the file system first. I was looking
at the ramifications for the test suite, and I realized that
css_helper_spec tests a lot of stuff in css_loader. css_helper_spec
should probably just test that the strategies are used in the correct
order, and we should add a css_loader_spec to test the different
strategies in isolation.

I'll try to take a crack at this soon if I get a chance.

On Wed, Apr 10, 2013 at 12:01 PM, Philipe Fatio notifications@github.comwrote:

Well, I think it would be possible as it is done in this pull request:

def assets_precompiled?
!::Rails.configuration.assets.compile rescue falseend

But indeed, it might be wiser to first check the file system and after
that check for the asset pipeline and after that perform a request to the
asset host.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/64#issuecomment-16183761
.

[kellyfelkins]

Any updates on this? I'm experiencing this problem on Heroku, although only in a staging environment.

[fphilipe]

@kellyfelkins I was thinking that the asset loading could be extracted into its own gem since it might be of interest for others. Unfortunately, I didn't have time yet to do so.

[kellyfelkins]

I decided to go with the gucki fork. https://github.com/gucki/premailer-rails.git

It's working fine for me.

What I did not figure out is why I had a problem in a staging environment, but not in production. Both environments are on Heroku.

-Kelly

[jjb]

looks like this is only a whitespace change -- you should revert it so that the diff is more minimal (and it does indeed conflict with current master)