Skip to content
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

Recursive watch flag has no effect on imported files #1731

Closed
fr-ser opened this issue Sep 25, 2016 · 18 comments
Closed

Recursive watch flag has no effect on imported files #1731

fr-ser opened this issue Sep 25, 2016 · 18 comments

Comments

@fr-ser
Copy link

fr-ser commented Sep 25, 2016

Oddly enough the -r flag does not seem to make any difference on my system. I use the following command:

node-sass --watch development/style/main.sass production/bundle.css --recursive

This process builds my file just fine, but it ignores any changes done in imported files. The imports work however as the changes to the imported files are included once I change my main.sass again...

Any ideas why this is?

General information:

  • OS: Windows 10
  • NPM version: 3.8.6
  • Node version: 6.0.0
  • Node Process: { http_parser: '2.7.0', node: '6.0.0', v8: '5.0.71.35', uv: '1.9.0', zlib: '1.2.8', ares: '1.10.1-DEV', icu: '56.1', modules: '48', openssl: '1.0.2g' }
  • Node Platform: win32
  • Node architecture: ia32
  • node-sass version:
    node-sass 3.10.0 (Wrapper) [JavaScript]
    libsass 3.3.6 (Sass Compiler) [C/C++]
@xzyfer
Copy link
Contributor

xzyfer commented Sep 25, 2016

Recursive flag is for watching directories. It doesn't make sense to
recursively watch a single file.

As for watching imported files I think that may not work when watching a
single file. This may be a bug.

On 25 Sep 2016 6:06 PM, "Sergej Fröhlich" notifications@github.com wrote:

Oddly enough the -r flag does not seem to make any difference on my
system. I use the following command:

node-sass --watch development/style/main.sass production/bundle.css
--recursive

This process builds my file just fine, but it ignores any changes done in
imported files. The imports work however as the changes to the imported
files are included once I change my main.sass again...

Any ideas why this is?

General information:

  • OS: Windows 10
  • NPM version: 3.8.6
  • Node version: 6.0.0
  • Node Process: { http_parser: '2.7.0', node: '6.0.0', v8:
    '5.0.71.35', uv: '1.9.0', zlib: '1.2.8', ares: '1.10.1-DEV', icu: '56.1',
    modules: '48', openssl: '1.0.2g' }
  • Node Platform: win32
  • Node architecture: ia32
  • node-sass version: node-sass 3.10.0 (Wrapper) [JavaScript] libsass
    3.3.6 (Sass Compiler) [C/C++]


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#1731, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAjZWCrp3DwF2ncWsZBpCPiVoSY9spM8ks5qtiuagaJpZM4KF2Rt
.

@fr-ser
Copy link
Author

fr-ser commented Sep 25, 2016

I thought, the recursive flag would exactly do that. Also watch imported files. If that is not the case, I suppose I don't need to keep looking.

Is there a better option for my use case? Watching a single file and it's imports?

@xzyfer
Copy link
Contributor

xzyfer commented Sep 25, 2016

There's no other option I'm aware. This is a bug in node sass.

On 25 Sep 2016 7:00 PM, "Sergej Fröhlich" notifications@github.com wrote:

I thought, the recursive flag would exactly do that. Also watch imported
files. If that is not the case, I suppose I don't need to keep looking.

Is there a better option for my use case? Watching a single file and it's
imports?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1731 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAjZWAUbnYe7jnfD5-EReGioc1oG7Vy1ks5qtjgrgaJpZM4KF2Rt
.

@fr-ser fr-ser changed the title Recursive flag has no effect Recursive flag has no effect on imported files Sep 25, 2016
@fr-ser fr-ser changed the title Recursive flag has no effect on imported files Recursive watch flag has no effect on imported files Sep 25, 2016
@MrSkinny
Copy link

MrSkinny commented Oct 9, 2016

Can anyone shine a light on a best practice here? The preference is the same behavior as webpack with js files -- you have a single entry index.js and import all modules from there and webpack watches all imports...

@fr-ser
Copy link
Author

fr-ser commented Oct 9, 2016

I switched to using a custom build file and a custom filewatcher, which then triggers node-sass. The preferred behaviour (and in my opinion the default) should however indeed be that one entry file and all its imports are watched by node-sass alone

@xzyfer
Copy link
Contributor

xzyfer commented Oct 9, 2016

The expected behaviour of node-sass is what you're describing. This is a
big with either node-sass or sass-graph, which maps the import graph.

On 9 Oct 2016 6:10 PM, "Sergej Fröhlich" notifications@github.com wrote:

I switched to using a custom build file and a custom filewatcher, which
then triggers node-sass. The preferred behaviour (and in my opinion the
default) should however indeed be that one entry file and all its imports
are watched by node-sass alone


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1731 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAjZWDUUokZftH-wxnBKalQ6HIzK-49qks5qyJNngaJpZM4KF2Rt
.

@fr-ser
Copy link
Author

fr-ser commented Oct 9, 2016

Sadly it's not working for me in the above mentioned setup. Don't know how many others this problem have and how replicable it is.
I'll probably have to wait and see how much traction this issue gets...

@marvinhagemeister
Copy link
Contributor

marvinhagemeister commented Oct 9, 2016

I'm experiencing the same issue. This seems to be a bug of our watcher gaze: shama/gaze#205.

As a workaround we've put a chokidar instance in front of node-sass and this has worked out great us. Haven't had any issues with missed fs changes anymore!

@fvgs
Copy link

fvgs commented Oct 9, 2016

I just tested the --watch flag to determine whether or not it would watch partials imported using @import. I tested both using a single file as the source and using a directory as the source. In both cases, imported files were watched (tested to a depth of 1), and the css output for the root file in the tree was updated.

This seems like the most sensible behavior and is what I expected as the default. However, these findings seem to contradict those of other people above.

It's difficult to say what the cause of the problem is for those affected. However, based on my experience debugging issues related to file watchers in other projects, my suggestion is to update to the latest version of node (if possible) and reinstall all node_modules i.e. rm -r node_modules/ && npm install. Updates to your OS or node can easily cause dependencies to break. By reinstalling all dependencies, you can at least make sure you have the latest versions with any potential patches.

Finally, I want to point out that the phrasing of the description of the --recursive flag in the readme is a bit confusing

Recursively watch directories or files

As @xzyfer said above

Recursive flag is for watching directories. It doesn't make sense to recursively watch a single file.

Like @fr-ser, I took the description to mean that using --recursive would enable watching of imports. Thus, I was unsure whether --watch was meant to watch imports, which is why I did the above tests. Therefore, I think it would be helpful to clarify the description of --recursive to reduce confusion.

@MrSkinny
Copy link

MrSkinny commented Oct 9, 2016

@fvgs per this:

I just tested the --watch flag to determine whether or not it would watch partials imported using @import. I tested both using a single file as the source and using a directory as the source. In both cases, imported files were watched (tested to a depth of 1), and the css output for the root file in the tree was updated.

This is also the behavior I expect (and I presume many on this thread who are having similar problems). I will say that there appears to be a strange inconsistency to it. From my testing, here is something I noticed:

  1. Start watcher
  2. Create index.scss
  3. Create second.scss with a css block
  4. Add @import "second" to index -- watcher correctly compiles
  5. Make change to second -- watcher fails to compile
  6. Add newline to index and re-save -- watcher correctly compiles

...now here's where it gets even weirder...

  1. CTRL-C watcher and start it again
  2. Make change to second -- watcher correctly compiles!

...because of the preexistence of @import, the watcher is actively checking second. But if I repeat the process and add another new file and then import, I get the same problem until the next watch process restart.

I've tried deleting the project's node_modules as well as starting new projects with same problem.

@xzyfer
Copy link
Contributor

xzyfer commented Oct 10, 2016

As a team we're pressed for time. I personally am currently focused on shipping a major LibSass update.
I appreciate the affect this has of everyones work flow. My suspicion, without looking too closely, is that this should be a good first time contributor bug.

I'll outline some details for anyone keen to investigate and PR a fix. When the watch is started via the CLI when build an import graph. The graph tells up which files are imported by other files, and which files import them etc.. Using this graph, given a file, we can walk up the graph to find the top most files and compile them.

For this to work we must watch all the files in the graph. When a file is changed we look the file's ancestors in the import graph. If an ancestor does not start with an _ we compile it.

The bug is either in the watch logic, in sass-graph.

@walston
Copy link

walston commented Oct 11, 2016

wouldn't you want files that DO begin with an _ to cause a recompile of the ancestor?

that's definitely the intuitive functionality i'm looking for that lead me to this thread.

marvinhagemeister added a commit to marvinhagemeister/node-sass that referenced this issue Oct 11, 2016
@xzyfer
Copy link
Contributor

xzyfer commented Oct 12, 2016

@walston

For this to work we must watch all the files in the graph. When a file is changed we look the file's ancestors in the import graph. If an ancestor does not start with an _ we compile it.

We watch for changes to files that begin with an _ (partials), but we don't compile them. We compile the files that imported them. Partials themselves are never compile targets in Sass.

marvinhagemeister added a commit to marvinhagemeister/node-sass that referenced this issue Nov 8, 2016
marvinhagemeister added a commit to marvinhagemeister/node-sass that referenced this issue Nov 8, 2016
marvinhagemeister added a commit to marvinhagemeister/node-sass that referenced this issue Nov 8, 2016
@charleschenster
Copy link

charleschenster commented Dec 8, 2016

I ran into this issue and I was able to track down why descendent files were not being watched.

The import regex in https://github.com/xzyfer/sass-graph/blob/master/parse-imports.js#L3 is looking for imports of the following format:

@import "foobar"

My imports originally were not in single or double quotes (e.g. @import foobar) so my import files were not properly being watched. After adding double quotes around my imports, they started being watched by node-sass properly.

This is something to watch out for if anyone else is running into the same issue.

@mmase
Copy link

mmase commented Jan 26, 2017

This change has caused the watched to become sluggish for us, sometimes taking a minute to trigger node-sass on changes.

@marvinhagemeister
Copy link
Contributor

@mmase That's worrysome. Can you share your setup or better a sample repro where the problem is reproducable?

@xzyfer
Copy link
Contributor

xzyfer commented Jan 27, 2017

Moved to #1869

@leognmotta
Copy link

leognmotta commented Aug 24, 2018

Here it is how I do it in my console, works just fine:
node-sass --watch --recursive development/style --output production

And use your import as:
@import "partials/main-styles";

jiongle1 pushed a commit to scantist-ossops-m2/node-sass that referenced this issue Apr 7, 2024
Fix error messages for `min` and `max`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants