Restore Sass dependencies #370
Conversation
| # Create dependency | ||
| filter = options[:nanoc_current_filter] | ||
| item = filter.imported_filename_to_item(full_filename) | ||
| filter.depend_on([ item ]) unless item.nil? |
There was a problem hiding this comment.
If a Sass file is found, but cannot be matched to an item in the nanoc site, what should we do? Error? Warning? Ignore?
There was a problem hiding this comment.
I vote ignore, but would settle for warning as well. I feel like that's a buyer-beware sort of moment, right?
Maybe a warning, then a way to force Sass files including non-nanoc files to always recompile? "Yo, your stuff's going to be slow because you didn't actually make your include files trackable"
There was a problem hiding this comment.
Plain ignore gets a -1 from me, because this might lead to problems that are very hard to debug.
Ignore + always recompiling the item that has the @import would work if nanoc had a way of marking an item as “always dirty”, which it doesn’t, so that also gets a -1 from me (unless we can implement “always dirty”?).
Warning is good.
There was a problem hiding this comment.
It's very common for a sass file to depend on files that are not in the project. E.g. Compass. If you warn on this, it will be very annoying.
There was a problem hiding this comment.
Are project partials considered items even though they have no output representation? Is it possible to depend on something that is not an item. IMO, that would be the cleanest. you just need to check it's mtime.
There was a problem hiding this comment.
Oh yeah, warnings really don’t make any sense (and errors even less).
The only sensible option is ignoring.
Ignoring + always recompiling doesn’t really make sense. If we let nanoc always recompile anything that depends on an external file, then everything will always be recompiled, because there’s plenty of dependencies on Ruby source code, external libraries, OS functionality, etc… so always recompiling is not a sensible option.
There was a problem hiding this comment.
@chriseppstein Sass partials are considered items (without output paths, usually*) and dependencies are handled properly.
(* Partials are usually not outputted, although they strictly speaking can be. That doesn’t have an effect on dependency tracking though.)
There was a problem hiding this comment.
For dependencies on files in Compass, they would only change if the Compass version changes, right? IIRC Changing a gem version would result in a full recompile anyway. Confirm, @ddfreyne?
There was a problem hiding this comment.
If they're in the site's content folder and are just routed to nil, yeah, they're items and are handled as dependencies. But if they're outside the site's content directories, then changes wouldn't trigger a recompile.
There was a problem hiding this comment.
Changing a gem version doesn’t cause a full recompile. I’ve thought about doing this, but I don’t think it makes a lot of sense. Maybe for major (X.y.z) versions it does, if you consider semantic versioning?
|
Once this fix is approved, I’ll push out 3.6.7. |
|
I checked this fix - bug #350 not reproduced! Thanks guys! |
|
@bobthecow Can I get a +1? |
|
@ddfreyne You're holding off on the Compass sprite importer? |
|
@bobthecow Yeah. In nanoc’s |
|
Then 👍
|
Restore Sass dependencies
Sass
@imports no longer generate the right dependencies in nanoc 3.6.6. This PR fixes that.nanoc 3.6.6 includes a change that reduces the number of redundant dependencies. However, this change caused required dependencies to be removed as well, erroneously assuming they were redundant. The problem existed before 3.6.6, but was masked because the redundant dependencies had the surprisingly beneficial effect of causing Sass files to be recompiled anyway.
The right fix is not to rollback the change introduced in 3.6.6, but rather fix it properly by finding the right imported files. This eliminates any redundant dependencies as well. This is what this PR does, and in the process also greatly simplifies the existing code.
This is a fix for #350.
@Leolik @phklevets Can you check whether this fix solves the problem for your sites? (and/or review the code for this fix)
@bobthecow @chriseppstein Can you review this code?