Asset dependencies are not respected in `render` plugin
Closed this issue · 7 comments
Hi Jeremy,
thanks for making & maintaining a really cool framework. I've found some unexpected behavior when rendering SCSS assets with dependencies.
Summary
When I change a dependent SCSS file, the rendered output does not reflect my changes.
Given this configuration
plugin :assets,
css: {
checkout: 'index.scss',
},
dependencies: {
expand_path('assets/css/checkout/index.scss') => Dir['assets/**/*.css', 'assets/**/*.scss']
},
css_opts: { style: :compressed, cache: false },
timestamp_paths: true
assets/css/checkout/index.scss
:
@import 'other';
assets/css/checkout/other.scss
:
<content is irrelevant>
Expected behavior
Changes to other.scss
should be reflected upon page reload.
Actual behavior
Changes to other.scss
are only reflected upon server restart, not before.
Probable cause
Looking at https://github.com/jeremyevans/roda/blame/master/lib/roda/plugins/render.rb#L262,
the TemplateMtimeWrapper
's modified?
method has no clue about the template's dependencies (i.e. other.scss
), and as such always uses the mtime
for the template file itself (i.e. index.scss
), instead of the highest mtime
of its dependencies. This leads to the template not actually being reloaded in line 270. This, in turn, means that the changes in other.scss
are not reflected when rendering.
Changing the render
function to always set @mtime = Time.now
at the beginning of the function produces the expected behavior, but I'm guessing this "disables" template caching altogether.
I can submit a PR if you point me in the right direction for a fix.
Thanks!
I agree this is a bug. We probably need the same type of code that the assets plugin uses, where you can pass an array of dependencies and it checks the mtime for all of them. Maybe we can pass the :dependencies
option for the asset in the render
call in the assets plugin, and then in render plugin, in retrieve_template
, after creating the TemplateMtimeWrapper
, set the dependencies on the template if render_opts[:dependencies]
is set. Then in TemplateMtimeWrapper#modifed?
, have it check all dependencies as well as the path itself, similar to the code in the assets plugin.
@jgarth Is this something you plan to work on in the near future? If not, I can work on it before the next release.
Yes, I'd like to submit a PR over the weekend. However, if this is more urgent to you then by all means go ahead.
That sounds great. The weekend should be fine. The next release isn't for a couple weeks.
I plan to work on this shortly.
I added a PR with what I currently have, but I failed to incept a proper spec for the changes to the asset plugin
OK, thank you. I was working on a spec first, so once I have a failing spec, I can hopefully merge the PR and test that it fixes the issue.