Use sass-graph for accurate sass watching

[xzyfer]

Problem

The watcher is broken. So very broken. Two major problems with it

• it compiles files starting with underscores (_) which is a big no-no
• it has no awareness of the the import graph

Assuming the following directory structure

src
|--foo.scss
|--bar.scss
|-- components
   |-- _baz.scss

where foo.scss and bar.scss, and running the following command node-sass -w -r --output dist src.

A modification to src/components/_baz.scss should not compile _baz.scss. It should traverse the import graph and compile dist/foo.css and dist/bar.css.

This is how the native sass watcher works.

Solution

Use sass-graph to build an in memory graph of the @imports to determine the correct compilation targets for a modified file.

I believe this is the issue at the heart of #157.

[am11]

This is super awesome. 👍

Considering this request: #590, I didn't knew something like sass-graph existed! 😄 I think node-sass should not expose this functionality (or expose the show-graph feature) to its API. That would be against the laws of DRY (especially now where have package referenced here).

[xzyfer]

I didn't knew something like sass-graph existed

It didn't really til today...

[browniefed]

@xzyfer any idea how it will play with custom importers in 2.0?
I can imagine that people will write some compass stuff to process Sprites, or other arbitrary things that aren't true file paths.

[xzyfer]

I think node-sass should not expose this functionality (or expose the show-graph feature) to its API.

I've isolated this functionality explicitly to the watch function in the cli. The graph doesn't even exist until you being watch so there shouldn't be any leakage.

[am11]

oOo.. Did you release it today for the first time? I saw old PRs in your repo, thought it out there for a while.. :)

[xzyfer]

@browniefed sass-graph currently follows the native sass model. It'll walk imports, if the file exists on disk in an include path it's be added to an in memory graph. That graph is then used to find the collection or top-most ancestors which should be re-compiled.

ATM it has no knowledge of custom importers.

[browniefed]

@xzyfer I guess I was implying it'd be cool if it could accept an importer, likely the same one that would be used for node-sass, so both things could pair up in terms of custom importer logic.

[am11]

Can we take the *cookie approach here as well to save the require('path/to/my-importer.js') object in graph?

Or if it is not possible to retain the JS object, then perhaps function-to-string approach: require('path/to/my-importer.js').toString() and eval it before usage.

[xzyfer]

@am11 It's been in various stages of development for a while, but I released 1.0.0 a couple hours ago in preparation for getting it into node-sass for my dev team :)

[xzyfer]

Custom importers may very well be possible in future versions.

[am11]

On second thought, we may only want to store the path to custom importer in graph and then re-require it in watch even handler. (whichever approach is elegant).

Yes the importers can certainly wait, since you might like to consider providing room for the future "custom functions" as well (which will behave the same way, except there will by a multiplicity factor -- spoiler alert!!). :)

[coveralls]

Coverage remained the same when pulling 81f76fc on xzyfer:feat/better-watcher-compilation-targets into c9e3eaa on sass:master.

[coveralls]

Coverage remained the same when pulling 81f76fc on xzyfer:feat/better-watcher-compilation-targets into c9e3eaa on sass:master.

[xzyfer]

aha! CI is passing :D 🎉

[am11]

This needs to be rebased out. :D

[xzyfer]

done in a35326a

[coveralls]

Coverage remained the same when pulling a35326a on xzyfer:feat/better-watcher-compilation-targets into c9e3eaa on sass:master.

[am11]

🎉

Feel free to merge it whenever you think this is ready.

Maybe we can set sass-graph to ^1.0.1 in package.json.

[xzyfer]

Maybe we can set sass-graph to ^1.0.1 in package.json.

I generally give npm the benefit of the doubt but better safe than sorry. I've update this. When all the builds pass I'll give it a run on my local Libsass 3.2.0 fork then ship it.

[coveralls]

Coverage remained the same when pulling fa13e98 on xzyfer:feat/better-watcher-compilation-targets into c9e3eaa on sass:master.

[xzyfer]

Works amazingly! Are those AppVeyor failures expected?

[am11]

Yup! :)

[xzyfer]

🎉 🚀

[am11]

Yay! :)