Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

Fix watching of CSS files to avoid infinite build cycle #1925

Closed
wants to merge 1 commit into from

Conversation

Nateowami
Copy link

Previously when the src and dest dirs were the same, whenever a CSS file changed (either manually or as the result of a build) it was recursively recompiled in an infinite loop, unless a build finished in less than
500ms (Gaze's default file watching debounce). Now, when the input and output dirs are the same, CSS files are treated like includes (files starting with an underscore) in that they are never built into CSS, but
they are watched in case they are included in a Sass file.

When the input and output dirs differ, functionality is unchanged.

I'm not sure the best way to go about writing tests for this. The infinite cycle only starts when builds takes longer than Gaze's default debounce timeout of 500ms (which is happening in a project I'm working on). A small logic bug in Gaze (incorrect operator short-circuiting) prevents setting the timeout to 0 (poking into node_modules is how I demonstrated this issue while testing my changes). The one symptom you could test for is that if you modify a CSS file node-sass will process it and reformat it.

Currently, for reasons unknown to me, all the watch tests are being skipped (except the check that node-sass doesn't immediately exit). This causes me think the authors have been unable to get the watch tests passing. As someone unfamiliar with the project I would most likely be unable to get watch tests passing either.

This is related to #1758 (Inconsistent behavior when using directory input with --watch and without --watch). It would make the most sense to not compile CSS files at all (or at least make it consistent with and without --watch). As, @xzyfer points out though, changing it now could break builds that rely on node-sass compiling CSS files to another directory. However, this change only affects the scenario where the input directory is the same as the output directory, and therefore should not cause a problem.

Previously when the src and dest dirs were the same, whenever a CSS file
changed (either manually or as the result of a build) it was recursively
recompiled in an infinite loop, unless a build finished in less than
500ms (Gaze's default file watching debounce). Now, when the input and
output dirs are the same, CSS files are treated like includes (files
starting with an underscore) in that they are never built into CSS, but
they are watched in case they are included in a Sass file.

When the input and output dirs differ, functionality is unchanged.
@xzyfer
Copy link
Contributor

xzyfer commented Mar 21, 2017

I appreciate you taking the time, however we cannot accept this change.

This is a significant breaking change for node-sass users. Additionally causes node-sass to differ from other LibSass bindings i.e. sassc, libsass-python, perl-sass etc..

This behaviour is due to a known bug in LibSass. The bug, although easy to fix, cannot be patched because it would unnecessarily disrupt the community. Sass 4 will introduce first class modules that will allow us to support importing of CSS files in an official way.

For now our advice for now is to not use the same directory for input and output. Alternative explicitly add a .scss extension to your @imports.

@xzyfer xzyfer closed this Mar 21, 2017
@Nateowami
Copy link
Author

Thanks for taking the time to look at this.

I wonder though whether I may not have explained the changes clearly, as I do not believe this is a breaking change. I tried to track down the bug you mentioned, but I'm not sure I found it. Perhaps sass/libsass#2096, or #1758?

In this PR, the behavior of watching CSS files remains unchanged. They must be watched because they can be included into Sass files. The only thing this changes is that when watching files, if the input dir is the same as the output dir, CSS files will not be built on top of themselves recursively. They'll still be watched, and can be included into other files. They become more like _include files (though only when watching, and only when input and output are identical directories—not in any other case). This is the same issue @kibbled mentioned at the end of #1758.

@Livven
Copy link

Livven commented Jun 25, 2017

@xzyfer Is this documented anywhere? This is a very surprising issue that is very hard to search for, e.g. these people over at create-react-app don't seem to be aware that it is indeed a known bug.

Livven added a commit to Livven/tabcenter-redux that referenced this pull request Jun 25, 2017
Livven added a commit to Livven/tabcenter-redux that referenced this pull request Jun 25, 2017
@RoyTinker
Copy link

RoyTinker commented Feb 9, 2018

AFAIK, the current best workaround for this issue is to use node-sass-chokidar.

xzyfer added a commit that referenced this pull request Apr 16, 2018
This was a non-standared feature in LibSass we had to support. It
is being removed in LibSass 3.6.0.

This prevents people from output their source directory.

See sass/libsass#2611
See #2184
See #2006
See #1933
See #1925
See #1867
See #1845
nschonni pushed a commit to nschonni/node-sass that referenced this pull request Jul 9, 2018
This was a non-standared feature in LibSass we had to support. It
is being removed in LibSass 3.6.0.

This prevents people from output their source directory.

See sass/libsass#2611
See sass#2184
See sass#2006
See sass#1933
See sass#1925
See sass#1867
See sass#1845
xzyfer added a commit that referenced this pull request Jul 16, 2018
This was a non-standared feature in LibSass we had to support. It
is being removed in LibSass 3.6.0.

This prevents people from output their source directory.

See sass/libsass#2611
See #2184
See #2006
See #1933
See #1925
See #1867
See #1845
xzyfer added a commit that referenced this pull request Jul 16, 2018
This was a non-standared feature in LibSass we had to support. It
is being removed in LibSass 3.6.0.

This prevents people from output their source directory.

See sass/libsass#2611
See #2184
See #2006
See #1933
See #1925
See #1867
See #1845
xzyfer added a commit that referenced this pull request Nov 7, 2019
This was a non-standared feature in LibSass we had to support. It
is being removed in LibSass 3.6.0.

This prevents people from output their source directory.

See sass/libsass#2611
See #2184
See #2006
See #1933
See #1925
See #1867
See #1845
xzyfer added a commit that referenced this pull request Nov 7, 2019
This was a non-standared feature in LibSass we had to support. It
is being removed in LibSass 3.6.0.

This prevents people from output their source directory.

See sass/libsass#2611
See #2184
See #2006
See #1933
See #1925
See #1867
See #1845
xzyfer added a commit that referenced this pull request Nov 7, 2019
This was a non-standared feature in LibSass we had to support. It
is being removed in LibSass 3.6.0.

This prevents people from output their source directory.

See sass/libsass#2611
See #2184
See #2006
See #1933
See #1925
See #1867
See #1845
xzyfer added a commit that referenced this pull request Nov 8, 2019
This was a non-standared feature in LibSass we had to support. It
is being removed in LibSass 3.6.0.

This prevents people from output their source directory.

See sass/libsass#2611
See #2184
See #2006
See #1933
See #1925
See #1867
See #1845
xzyfer added a commit that referenced this pull request Mar 21, 2020
This was a non-standared feature in LibSass we had to support. It
is being removed in LibSass 3.6.0.

This prevents people from output their source directory.

See sass/libsass#2611
See #2184
See #2006
See #1933
See #1925
See #1867
See #1845
saper pushed a commit to saper/node-sass that referenced this pull request May 17, 2020
This was a non-standared feature in LibSass we had to support. It
is being removed in LibSass 3.6.0.

This prevents people from output their source directory.

See sass/libsass#2611
See sass#2184
See sass#2006
See sass#1933
See sass#1925
See sass#1867
See sass#1845
jiongle1 pushed a commit to scantist-ossops-m2/node-sass that referenced this pull request Apr 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants