New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore Sass dependencies #370

Merged
merged 4 commits into from Dec 9, 2013

Conversation

Projects
None yet
4 participants
@ddfreyne
Member

ddfreyne commented Dec 6, 2013

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?

# Create dependency
filter = options[:nanoc_current_filter]
item = filter.imported_filename_to_item(full_filename)
filter.depend_on([ item ]) unless item.nil?

This comment has been minimized.

@ddfreyne

ddfreyne Dec 6, 2013

Member

If a Sass file is found, but cannot be matched to an item in the nanoc site, what should we do? Error? Warning? Ignore?

@ddfreyne

ddfreyne Dec 6, 2013

Member

If a Sass file is found, but cannot be matched to an item in the nanoc site, what should we do? Error? Warning? Ignore?

This comment has been minimized.

@bobthecow

bobthecow Dec 6, 2013

Member

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"

@bobthecow

bobthecow Dec 6, 2013

Member

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"

This comment has been minimized.

@ddfreyne

ddfreyne Dec 6, 2013

Member

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.

@ddfreyne

ddfreyne Dec 6, 2013

Member

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.

This comment has been minimized.

@chriseppstein

chriseppstein Dec 6, 2013

Contributor

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.

@chriseppstein

chriseppstein Dec 6, 2013

Contributor

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.

This comment has been minimized.

@chriseppstein

chriseppstein Dec 6, 2013

Contributor

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.

@chriseppstein

chriseppstein Dec 6, 2013

Contributor

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.

This comment has been minimized.

@ddfreyne

ddfreyne Dec 6, 2013

Member

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.

@ddfreyne

ddfreyne Dec 6, 2013

Member

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.

This comment has been minimized.

@ddfreyne

ddfreyne Dec 6, 2013

Member

@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.)

@ddfreyne

ddfreyne Dec 6, 2013

Member

@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.)

This comment has been minimized.

@bobthecow

bobthecow Dec 6, 2013

Member

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?

@bobthecow

bobthecow Dec 6, 2013

Member

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?

This comment has been minimized.

@bobthecow

bobthecow Dec 7, 2013

Member

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.

@bobthecow

bobthecow Dec 7, 2013

Member

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.

This comment has been minimized.

@ddfreyne

ddfreyne Dec 7, 2013

Member

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?

@ddfreyne

ddfreyne Dec 7, 2013

Member

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?

def _find(dir, name, options)
# Find filename
full_filename, syntax = ::Sass::Util.destructure(find_real_file(dir, name, options))
return nil if full_filename.nil?

This comment has been minimized.

@ddfreyne

ddfreyne Dec 6, 2013

Member

What if there are other importers? Do we need to override all? Can we prevent other importers from being used?

@ddfreyne

ddfreyne Dec 6, 2013

Member

What if there are other importers? Do we need to override all? Can we prevent other importers from being used?

This comment has been minimized.

@bobthecow

bobthecow Dec 6, 2013

Member

doesn't compass ship with its own importer?

@bobthecow

bobthecow Dec 6, 2013

Member

doesn't compass ship with its own importer?

This comment has been minimized.

@ddfreyne

ddfreyne Dec 6, 2013

Member

One that inherits from Sass::Importers::Filesystem AFAIK.

@ddfreyne

ddfreyne Dec 6, 2013

Member

One that inherits from Sass::Importers::Filesystem AFAIK.

This comment has been minimized.

@chriseppstein

chriseppstein Dec 6, 2013

Contributor

Compass has a sprite importer, other than that it uses the Sass filesystem importer. There's also the glob importer but it ends up resulting in calls to the filesystem importer so it should be ok. The sprite importer, in theory, should create dependencies on the sprite source images.

@chriseppstein

chriseppstein Dec 6, 2013

Contributor

Compass has a sprite importer, other than that it uses the Sass filesystem importer. There's also the glob importer but it ends up resulting in calls to the filesystem importer so it should be ok. The sprite importer, in theory, should create dependencies on the sprite source images.

This comment has been minimized.

@ddfreyne

ddfreyne Dec 6, 2013

Member

@chriseppstein What should be overridden in the sprite importer to catch all @imported paths? I see

  • ::Compass::SpriteImporter.files(uri)
  • ::Compass::SpriteImporter.find_all_sprite_map_files(path) (not used within the class itself)
@ddfreyne

ddfreyne Dec 6, 2013

Member

@chriseppstein What should be overridden in the sprite importer to catch all @imported paths? I see

  • ::Compass::SpriteImporter.files(uri)
  • ::Compass::SpriteImporter.find_all_sprite_map_files(path) (not used within the class itself)

This comment has been minimized.

@chriseppstein

chriseppstein Dec 6, 2013

Contributor

the files method should do the trick.

@chriseppstein

chriseppstein Dec 6, 2013

Contributor

the files method should do the trick.

This comment has been minimized.

@chriseppstein

chriseppstein Dec 6, 2013

Contributor

Actually, it will be hard for you to accomplish this from the files method since there's no options there. I suggest overriding sass_engine instead.

@chriseppstein

chriseppstein Dec 6, 2013

Contributor

Actually, it will be hard for you to accomplish this from the files method since there's no options there. I suggest overriding sass_engine instead.

This comment has been minimized.

@ddfreyne

ddfreyne Dec 6, 2013

Member

@chriseppstein I’m not sure why I need access to options… should the return value of #files not be enough?

@ddfreyne

ddfreyne Dec 6, 2013

Member

@chriseppstein I’m not sure why I need access to options… should the return value of #files not be enough?

This comment has been minimized.

@ddfreyne

ddfreyne Dec 7, 2013

Member

On second thought, nanoc needs some work in order to properly support sprites, so I’d rather leave the part related to sprites for some other time.

@ddfreyne

ddfreyne Dec 7, 2013

Member

On second thought, nanoc needs some work in order to properly support sprites, so I’d rather leave the part related to sprites for some other time.

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Dec 6, 2013

Member

Once this fix is approved, I’ll push out 3.6.7.

Member

ddfreyne commented Dec 6, 2013

Once this fix is approved, I’ll push out 3.6.7.

@Leolik

This comment has been minimized.

Show comment
Hide comment
@Leolik

Leolik Dec 7, 2013

I checked this fix - bug #350 not reproduced! Thanks guys!

Leolik commented Dec 7, 2013

I checked this fix - bug #350 not reproduced! Thanks guys!

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Dec 9, 2013

Member

@bobthecow Can I get a +1?

Member

ddfreyne commented Dec 9, 2013

@bobthecow Can I get a +1?

@bobthecow

This comment has been minimized.

Show comment
Hide comment
@bobthecow

bobthecow Dec 9, 2013

Member

@ddfreyne You're holding off on the Compass sprite importer?

Member

bobthecow commented Dec 9, 2013

@ddfreyne You're holding off on the Compass sprite importer?

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Dec 9, 2013

Member

@bobthecow Yeah. In nanoc’s sass filter’s current state, the sprite importer won’t function correctly anyway.

Member

ddfreyne commented Dec 9, 2013

@bobthecow Yeah. In nanoc’s sass filter’s current state, the sprite importer won’t function correctly anyway.

@bobthecow

This comment has been minimized.

Show comment
Hide comment
@bobthecow

bobthecow Dec 9, 2013

Member

Then 👍

:shipit:

Member

bobthecow commented Dec 9, 2013

Then 👍

:shipit:

ddfreyne added a commit that referenced this pull request Dec 9, 2013

@ddfreyne ddfreyne merged commit 21d0b38 into release-3.6.x Dec 9, 2013

@ddfreyne ddfreyne deleted the bug/incorrect-sass-dependencies branch Dec 9, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment