[bug] ensure that SassCompiler is not run on files that are not (.scss|.sass|.css) #215
Conversation
|
@simonexmachina would you be able to review this? |
|
@rwjblue this seems to be plaguing many others would you be able to see if we can merge this? |
| } | ||
|
|
||
| processString() { | ||
| if(this.inputTree._buildStart !== this.lastBuildStart) { |
There was a problem hiding this comment.
I am not sure of any way to make sure the files related to a specific input tree don't build again. Can multiple input trees have the same _buildStart? Is there a way we can distinguish the identity?
There was a problem hiding this comment.
@stefanpenner would you know of a good way to distinguish the identity of the input tree to avoid building specific trees that have built before?
There was a problem hiding this comment.
Why do we care? We should not be mucking with the passed in tree directly, only with this.inputPaths...
|
This is just not cared about anymore? Is there a version I can revert to for this? |
| - "10" | ||
| - "12" |
There was a problem hiding this comment.
This is odd, was this not handled by #216 ?
| - EMBER_TRY_SCENARIO=ember-lts-2.12 | ||
| - EMBER_TRY_SCENARIO=ember-lts-2.16 | ||
| - EMBER_TRY_SCENARIO=ember-lts-2.18 |
There was a problem hiding this comment.
This seems unrelated to the goal of this PR
| } | ||
|
|
||
| processString() { | ||
| if(this.inputTree._buildStart !== this.lastBuildStart) { |
There was a problem hiding this comment.
Why do we care? We should not be mucking with the passed in tree directly, only with this.inputPaths...
| sourceMapFile = destFile + '.map'; | ||
| } | ||
|
|
||
| mkdirp.sync(path.dirname(destFile)); |
There was a problem hiding this comment.
Speculative mkdirp.sync is somewhat expensive, oftentimes we can avoid it by using try/catch and handling ENOENT + retry..
| var mergeTrees = require('broccoli-merge-trees'); | ||
|
|
||
| const path = require('path'); | ||
| const Filter = require('broccoli-persistent-filter'); |
There was a problem hiding this comment.
The use of broccoli-persistent-filter here seems odd to me. As far as I can tell, this is not a 1:1 compiler (which is essentially what broccoli-persistent-filter expects to be working with).
There was a problem hiding this comment.
what would be a good way to get this type of caching without broccoli-persistent-filter?
|
Any chance this will get merged soon? This PR seems to have worked, SassCompiler is gone from e-cli rebuild slowest nodes list. I'm still having a pretty slow entry for Is this also caused by this addon, or does it come from somewhere else? |
|
Looks like there is still some feedback that needs to be addressed before this can be merged. |
|
My codebase is big and usually around 3 seconds for SassCompiler when there are no SCSS changes. After this change, SassCompiler takes around 150ms and overall build time improvement is about 16%. For a SCSS change, the improvement is around 40%. Can we merge this? |
|
Willing to merge if feedback is addressed |
Motivation
#214 #178
The sass compiler has an unintended overhead when running in projects as it rebuilds when it is not necessary.
What happens after this change?
> When changing a non `.scss` fileThis results in a net savings for rebuilds of 50%!
Change itself
broccoli-sass-source-mapscopied into the project? I was hoping to have a discussion around this change before opening up two PRs. In the context of ember it might make sense to care about the buildStart time where as in the more genericbroccoli-sass-source-mapsit would be better to not have a filter between file changes. Sass is different thenjsandhbswhere the build output is the result of running sass on the top level input file.hbsandjscan be altered and combined at the end of the build.