Invoking sass once with all files to compile instead of invoking sass once per file to compile

danfinnie commented 9 years ago

I was recently looking into using this plugin to run sass --watch. I wanted to do this instead of using grunt-contrib-watch because sass --watch will use a cache to do incremental builds much faster than grunt-contrib-watch combined with this plugin (even with the update configuration option set to true).

I realized that even though getting grunt-contrib-sass to send sass the --watch option is trivial, this strategy doesn't really work because sass --watch will block and grunt-contrib-sass invokes sass once per file to compile. Why not send all the files to sass at once? This would make --watch a usable option for development. It also seems that minimizing the number of shell invocations and the number of times that the sass gem needs to boot would lead to better performance, but I haven't tested that hypothesis.

I did some digging in Git to see if I could figure out why sass was invoked this way. Here's what I found:

In the initial commit, there's no looping over sass invocations, but I also didn't find how sass was invoked at all.
Looping over this.files and invoking sass for each one was first introduced in 38320df, but this commit doesn't really explain why.
The loop was removed in 7a68a29 as part of some changes about how grunt handles this.files vs. this.file.
The subsequent commit, d6dfe52, brings the loop back and uses this.files again. This was a result of gruntjs/grunt#606
In commit f6bef05, async.forEachLimit instead of async.forEachSeries so that if you had multiple CPUs, they would all be put to work. This was a result of gruntjs/grunt-contrib-sass#43. What's interesting to me is that this PR states "The sass command line parameters don't allow processing multiple files per run, which would presumably be even faster." Maybe things have changed since that comment, or I'm misunderstanding what @altano is saying, but currently sass does let you specify multiple sources and destinations on the command line.
In commit 14fdd13, async.forEachLimit was switched to async.eachLimit. These are synonymous functions, so this change was purely cosmetic.

tl;dr: I don't understand why this plugin invokes sass once per element in this.files rather than joining this.files into one big argument list and invoking sass once. I couldn't find the reason for this in the git history, is there a reason? Or can it be changed to invoking sass once so that sass --watch works and fewer processeses are spawned in general?

@jonathanpberger @geoffwa

sindresorhus commented 9 years ago

How would you specify multiple files with different destinations using the Sass binary?

geoffwa commented 9 years ago

You can specify multiple inputs/output using a colon delimited format: sass file1.css.scss:file1.css file2.css.scss:file2.css

With multiple files specified this is the same as running sass --update. Alternately you can also run sass --watch to make this a permanent background process (which is fantastic for development).

sindresorhus commented 9 years ago

@geoffwa I did not know about that :)

PR welcome as long as it doesn't break anything.

danfinnie commented 9 years ago

Awesome! I'll put together a pull request soon based on this for you to take a peek at. On Jan 3, 2015 10:02 AM, "Sindre Sorhus" notifications@github.com wrote:

@geoffwa https://github.com/geoffwa I did not know about that. PR welcome as long as it doesn't break anything.

— Reply to this email directly or view it on GitHub https://github.com/gruntjs/grunt-contrib-sass/issues/178#issuecomment-68597481 .

danfinnie commented 9 years ago

@sindresorhus I put together a proof of concept: https://github.com/danfinnie/grunt-contrib-sass/compare/gruntjs:master...master. This code passes all of the tests that were included in the project, I was thinking about adding some more as well.

Upon further inspection, I saw that grunt-contrib-sass does not always pass the same command line options sass for every invocation in a specific grunt task. For instance, you could run grunt sass:compile and it could call sass --update file1.scss file1.css and then sass --scss file1.css file1.out.css This makes it impossible to make exactly one sass call because some calls need --update and some cannot have --update. Instead, I group all of the files to compile based on the command line flags they need and run sass once per command line flag combination. I plan on parallelizing this so that --watch will work well. The combinations of flags are:

--scss if the source file is a .css file (can these just be copied into place?)
--update if options.update was specified and the file was previously compiled
--scss --update if both of the above are true
No extra arguments if none of the above is true

I ran into one issue that zapped a lot of time: sass automatically applies --update if you pass multiple files to compile, which is the entire point of this GitHub issue. I found no documentation for this feature (bug? property?), but I found it in the source. So, I pass the --force option which is what used to be happening implicitly. There's a lot of complexity around this, however, and I need to come up with some real use cases to make sure I got it right. grunt-contrib-sass does it's own handling based on if the user passed --update and if the file exists. Sass then runs it's own logic based on how many files were passed and if --force was passed.

I want to clean up the code linked above, but what do you think overall? I believe that I've maintained existing behavior but want your opinion :P.

sindresorhus commented 9 years ago

Nice start, but this looks a bit convoluted. Submit a PR as it's easier to discuss there.

This makes it impossible to make exactly one sass call because some calls need --update and some cannot have --update.

Why? What calls cannot have --update?

I'm having a hard time understanding the complication here, though I'm probably missing something.

danfinnie commented 9 years ago

@sindresorhus I will submit a PR soon.

I was trying to maintain backwards-compatibility with the current behavior. Currently, grunt-contrib-sass invokes sass with one SCSS file, which automatically disables --update. This change is to invoke sass with multiple SCSS files, which automatically enables --update. A lot of the complication is to maintain the old behavior, which is probably only necessary for a very small number of users with weird workflows that modify the CSS files after compilation.

Would you prefer cleaner code that didn't account for this?

I wrote a shell script to experiment with this behavior, because it's not really documented by SASS:

#!/usr/bin/env bash

set -e
set -x

sass -v
ruby -v

SASS_OPTS="--sourcemap=none"

rm -f *.css *.map
sass $SASS_OPTS foo.scss:foo.css bar.scss:bar.css

echo "junk" > foo.css
echo "junk" > bar.css
sass $SASS_OPTS foo.scss:foo.css bar.scss:bar.css

echo
echo "Tried to recompile both files, but both are junk:"
cat foo.css
cat bar.css

sass $SASS_OPTS foo.scss:foo.css

echo
echo "Only recompiled foo.css, and that file is updated:"
cat foo.css
cat bar.css

sass $SASS_OPTS bar.scss:bar.css

echo
echo "Only recompiled bar.css, and that file is updated:"
cat foo.css
cat bar.css

This is the output on my system:

+ sass -v
Sass 3.4.9 (Selective Steve)
+ ruby -v
ruby 2.0.0p576 (2014-09-19 revision 47628) [x86_64-linux]
+ SASS_OPTS=--sourcemap=none
+ rm -f bar.css foo.css '*.map'
+ sass --sourcemap=none foo.scss:foo.css bar.scss:bar.css
      write foo.css
      write bar.css
+ echo junk
+ echo junk
+ sass --sourcemap=none foo.scss:foo.css bar.scss:bar.css
+ echo

+ echo 'Tried to recompile both files, but both are junk:'
Tried to recompile both files, but both are junk:
+ cat foo.css
junk
+ cat bar.css
junk
+ sass --sourcemap=none foo.scss:foo.css
+ echo

+ echo 'Only recompiled foo.css, and that file is updated:'
Only recompiled foo.css, and that file is updated:
+ cat foo.css
.foo {
  content: 'foo'; }
+ cat bar.css
junk
+ sass --sourcemap=none bar.scss:bar.css
+ echo

+ echo 'Only recompiled bar.css, and that file is updated:'
Only recompiled bar.css, and that file is updated:
+ cat foo.css
.foo {
  content: 'foo'; }
+ cat bar.css
.bar {
  content: 'bar'; }

sindresorhus commented 9 years ago

Would you prefer cleaner code that didn't account for this?

Yes

I wrote a shell script to experiment with this behavior, because it's not really documented by SASS:

Would you mind opening an issue on Sass to get it documented?

danfinnie commented 9 years ago

I just created the issue with sass as you mentioned, it's linked above.

I've been on vacation but want to get to this soon :)

gruntjs / grunt-contrib-sass

Invoking sass once with all files to compile instead of invoking sass once per file to compile #178