Asset Pipeline support refs #54 #63
Conversation
* check for asset digest and store in TFR config * If asset pipeline, use theme_assets_dir config and PREPEND to config.asset.paths. This requires theme assets to be in a folder under the theme_name so as not to clobber other themes. * When gathering digested assets, fallback to the original asset if no digested one is available
* Load and override action_view when asset digests are enabled. * don't load asset_controller routes when asset pipeline is in use.
…gemfile with this the themes assets aren't prepended to the assets.path
|
ha i just happened to run that and was coming back here to let you know :) thanks man, that was it. hope this gets merged in soon! |
|
awesome! thanks for the feedback! |
|
have you noticed issues with calling theme_image_path directly? example: <%= theme_image_path 'test.png' %> == ":theme/images/test-{digest}.png" (incorrect) i would expect these to render the same paths, right? debugging now.. |
|
hmmm... interesting... I'll have a look at my end too. I assume you aren't referring to sass/css files here? |
|
so far i've just been testing with theme_image_path. with config.assets.digest = false, it works as expected (path being /assets/:theme/images/test.png). if i set config.assets.digest = true, and precompile assets, that's when it breaks (path being :theme/images/test-{digest}.png). have to run out for a few hours but will be back online later to continue debugging. thanks again! |
|
I've written this code so that if assets.digest if false it continues to use the original TFR code, the asset pipeline code is only enabled if assets.digest is true. (So you aren't' looking in the wrong place :) ) |
|
I've just tested this locally, and I'm getting the equivalent of "test-{digest}.png" returned. Could you tell me the dir structure of your themes folders? and also any config in your initializer? |
|
weird it seems to be working ok now. i was juggling a few things earlier, so maybe something else was off. for reference regardless my config/dir structure is below: initializers/theme_for_rails.rb -> gist.github.com/3506433 directory structure is: |
|
actually sorry, the issue still exists. if config.assets.digest = false everything is ok w/ theme_image_path, theme_stylesheet_path, and theme_javascript_path. with config.assets.digest = true all the theme tag functions work properly but the following is returned for theme path functions: <%= theme_javascript_path 'home' %> --> 'home.js' (expected: /assets/:theme/javascripts/home-{digest}.js) <%= theme_stylesheet_path 'home' %> --> 'home.css' (expected: /assets/:theme/stylesheets/home-{digest}.css) <%= theme_image_path 'test.png' %> --> ':theme/images/test-{digest}.png' (expected: /assets/:theme/images/test-{digest}.png) will continue debugging, but my config is above. thanks! |
|
hey guys i came up with this and it seems to be working well for us: i started from lucasefe/master and my approach was to stop using routes and send_file for assets and instead use the asset pipeline through rails. i'm guessing the previous reason for using routes was to support assets when the folder structure was not within app/assets/ (ex: app/themes/)? i still have to get tests working etc but let me know what you think. i'm newer to rails so hopefully this isn't too off. also fyi - having the gem within :assets group seemed to break in production for us. moving it out fixed. |
|
I had gone much the same route, you cannot take advantage of the asset pipeline and still use an assets_controller. What I had done however overrode the asset_controller methods when the asset pipeline is activated but still enabled the original functionality when not. Regarding the gem in the assets group, do you "initialize_on_precompile" ? |
| # will add the view path for a given theme name | ||
| def add_theme_view_path_for(name) | ||
| self.view_paths.insert 0, ::ActionView::FileSystemResolver.new(theme_view_path_for(name)) | ||
| end | ||
|
|
||
| def digest_for_image(asset, theme_context) | ||
| if ThemesForRails.config.asset_digests_enabled? | ||
| Rails.application.config.assets.digests["#{theme_context}/images/#{asset}"] || asset |
There was a problem hiding this comment.
by using Rails.application.config.assets.digests here this code doesn't handle the case where an asset isn't precompiled. If asset_path or asset_paths.digest_for is used instead, then that case is handled.
There was a problem hiding this comment.
Line 48 is looking for the precompiled, digested version OR returns the asset as it was. That line is also only run if asset digests is enabled.
Or I'm misunderstanding your concern. :)
There was a problem hiding this comment.
The issue is when when an asset is not precompiled. I should have added the comment to the digest_for_stylesheet (since images should all be compiled by default). Using the example from my other comment, say there is a file xproject/stylesheets/style.css. And this file has not been precompiled. If the assets settings are:
config.assets.compile = true
config.assets.digest = true
Now an asset reference is made like this:
theme_stylesheet_link_tag 'style'
The code above will fail to find xproject/stylesheets/style.css because it isn't precompiled. It will then fall back to style.css even though xproject/stylesheets/style.css actually exists.
With the settings above, I'd expect the code to figure out the asset exists, then compile it and return the digest. By using asset_path or asset_paths.digest_for instead of Rails.application.config.assets.digests, this is what will happen.
There was a problem hiding this comment.
now that is an interesting thought. I will have another look at those methods and try and adjust the code. Thanks for the pointer. :)
I believe the idea of checking the assets hash directly was that in our original usecase the precompilation is handled by deployment so we didn't worry about the case where the asset might be compiled during runtime. You have made a brilliant point and it would definitely improve the code!
I'll try and get an update to this in the next day or so.
|
@ksheurs can you open a pull request so it is easier to discuss your approach? |
| available_theme_names.each do |theme_name| | ||
| theme_asset_path = ThemesForRails.config.assets_dir.gsub(":root", ThemesForRails.config.base_dir).gsub(":name", theme_name.to_s) | ||
| Rails.logger.info "== Adding theme [#{theme_name}] asset dir [#{theme_asset_path}] to asset pipeline" | ||
| Rails.application.config.assets.paths.prepend(theme_asset_path) |
There was a problem hiding this comment.
Why is adding the themes to the config.assets.paths useful? (note: @ksheurs does this same thing in his fork)
Say there are two themes each with the same file in them:
assets/themes/xproject/stylesheets/styles.css
assets/themes/genigame/stylesheets/styles.css
If both assets/themes/xproject and assets/themes/genigame are added to the assets.paths then the app will only be able to access one of the styles.css files. It won't matter which theme is the current theme. All that will mater is the order that the themes were added.
So if this approach is followed, it means that every asset in a theme folder has to have a unique name.
There was a problem hiding this comment.
When we are adding the theme to the assets path we are ensuring that the path those themes are added under are prefixed by their theme_name. This allows us to use the tfr helpers and request "styles.css" in our code, the helpers prepend the theme_name and the rest is handled by the default rails asset pipeline.
We maintain ability to separate themes by doing this.
The code could certainly be cleaned up a bit, but I /think/ this is the best approach as the underlying "hike" doesn't allow us to define a prefix as being separate from the actual asset at this point.
Does this make more sense?
There was a problem hiding this comment.
Sorry, I still don't see the use case for adding the individual themes. By default the asset pipeline adds assets/themes to the paths, so in the example above the paths would look like:
["assets/themes/xproject", "assets/themes/genigame", "assets/themes"]
The themes_for_rails helpers in this code construct logical paths to assets like:
xproject/stylesheets/styles.css
So assets referenced using these helper methods will be found because of the default entry on the paths list not because of the individual theme entries.
Can you describe how you reference an asset that would take advantage of the individual theme entries?
I noticed this code and I'm asking about it, because the precompiling code in the asset pipeline doesn't seem to handle having multiple ways to reference the same asset. And this code causes that to happen. In the example above the xproject style.css would be available through 2 logical paths: xproject/stylesheets/style.css and stylesheets/style.css
There was a problem hiding this comment.
TFR allows us to configure where the theme files are stored. We don't store our themes in the assets folder (essentially git submodules under #{Rails.root}/themes/{theme_name}.)
In this case the theme path is not added as part of the default rails assets folder.
It may be worth checking the existing paths in sprockets to see if the theme_root is already in there to avoid duplication.
What do you think?
There was a problem hiding this comment.
Oh, so currently if I do this:
# assets_dir is the path to your theme assets.
config.assets_dir = ":root/app/themes/:name/assets"
# views_dir is the path to your theme views
config.views_dir = ":root/app/themes/:name/views"
The paths for the themes is not supposes to be added to assets.paths ?
Been fiddling with it for a while and couldn't get it to work.
Having those directories seems like a good default for unified themes though? Wouldn't want to put views in the rails assets folder.
…he default rails asset root of :root/app/assets
|
@jasherai Is this working? |
|
@mli-max its still working for us... 😄 let me know if you have any issues or questions... more than happy to help. |
Add support for the asset pipeline by leveraging it. Automatically add themes to the pipeline when it is in use.
Could you please let me know what you think?
I'm still looking into sprockets to see if we can avoid the extra nested folders required to make the asset pipeline work cleanly.