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

Nothing happens on watch in windows 7/64 #755

Closed
IGRACH opened this issue Mar 13, 2015 · 11 comments
Closed

Nothing happens on watch in windows 7/64 #755

IGRACH opened this issue Mar 13, 2015 · 11 comments

Comments

@IGRACH
Copy link

IGRACH commented Mar 13, 2015

I have just installed node-sass with npm. When I type node-sass -w main.scss it says Provide a Sass file to render. I have been using sass with ruby and it works fine. I'm using powershell 4.0 and win7/64

@am11
Copy link
Contributor

am11 commented Mar 14, 2015

I can reproduce it on Ubuntu on Win8.

Thanks for reporting this. 👍

What surprise me most is we have a test for it (cli.js#L154-L178), which passes on TravisCI! (Update: The test also specifies the src file 😕)

The temporary workaround is to provide scss twice: node-sass -w main.scss main.scss (the latter one is to satisfy CLI input file argument condition, which does not compile the file right away when watch is set).

am11 added a commit to am11/node-sass that referenced this issue Mar 14, 2015
* Removes input file parameter from watch test.
* Uses `once` instead of `on` in CLI tests.
  * Removes `bin.kill` in favour of `once`.

Issue URL: sass#755.
PR URL: sass#761.
@am11
Copy link
Contributor

am11 commented Mar 14, 2015

Fixed by 7a9238d via #761.

@am11 am11 closed this as completed Mar 14, 2015
@am11 am11 added this to the 3.0 milestone Mar 14, 2015
@mgreter
Copy link
Contributor

mgreter commented Mar 15, 2015

@am11 FYI (just some shameless self advertising): I have added watch option to perl-libsass too (since I had the code laying around anyway in webmerge) 😉

@am11
Copy link
Contributor

am11 commented Mar 15, 2015

@mgreter, this is great! I was actually thinking about discussing it with @xzyfer to drop the -watch support (oops! 😨), because there already are gulp, grunt (and whatnot) file watcher (satellite) solutions available which support node-sass like a champ! Therefore, it seems redundant. Besides, our implementation is flaky on multiple accounts. I don't know how reliable it is in long-term usage (after all, I don't use node-sass in my day job!)

@mgreter
Copy link
Contributor

mgreter commented Mar 15, 2015

@am11 IMO it's a pretty usefull feature. But I actually thought about putting the cli part into another module, since it is the only part that needs some special dependencies. I opted to have all the watcher related dependencies optional (the command will simply fail with a message, if a module is not installed). But the installer will not force those deps to be installed!

I also agree that it is actually pretty tricky to get this done correctly. IMO most important is a good library that abstracts the change event gathering across multiple archs. In my case I had to accept a blocking call. Therefore I had to put that code into a seperate forked process and add some inter-process comunication (IPC). But this actually has the benefit that I simply can kill and restart that child when the include files array has changed.

In case of perl-libsass there are not really any satellite solutions (beside my own webmerge project). And most of the new code was already in use for some years now (linux and windows). I guess at least some node-sass users would miss that feature to be directly available. But I guess that's your call.

@am11
Copy link
Contributor

am11 commented Mar 15, 2015

Thanks for the insights. I agree this is a useful feature. We are using a file-watcher dependency (gaze), which takes care of cross architectures and low-level API calls.

The issue is with the way we are recursively calling node-sass CLI within the CLI code is questionable.

One reason is that we are not guaranteeing that if the other compile time artifacts change (such as, custom importers, and in future custom functions), that change will also take effect in future compilations triggered by compiler. Also, we are not providing options to set custom destinations and glob paths in CLI either.

I attempted to explore the options once, #601 (comment), but that is a very slippery slope. Perhaps it will take some iterations for it to qualify as a robust solution.

@mgreter
Copy link
Contributor

mgreter commented Mar 15, 2015

IMO that's exactly why I had to fork the actual event watcher to its own process "thread". The main (parent) process and the (child) watcher communicate only via a pretty dumb IPC message queue. So the parent only waits for stuff to arive on the queue and manages the file-watcher children, effectively killing a child if the watchlist needs to change and starting a new one (actually it should be reversed to really guarantee that no changes go missing). In webmerge I support multiple files to be compiled, so the watcher there actually triggers multiple parallel compilations, when one file in the set is changed. But once the system/arch specific details are working, it's not very hard to add more bells and whistles 😄

@IGRACH
Copy link
Author

IGRACH commented Mar 15, 2015

Well ability to watch files, without setting up gulp or grunt tasks, is core functionality of text preprocessor as sass is.

@xzyfer
Copy link
Contributor

xzyfer commented Mar 15, 2015

I agree with @mgreter and @IGRACH I would vote emphatically for keeping the watcher. I also agree we could do it better but having gaze abstract away all the hard work gets us 95% the way there.

FWIW gulp and grunt use gaze (for now) so there's no reason for us to be any less reliable than them :)

@am11
Copy link
Contributor

am11 commented Mar 16, 2015

@xzyfer, I am ignoring the gaze reliability (BTW, alternatives also possess limitations); its our CLI code for watch which needs to be rewritten for it to reflect all the node-sass API features and support glob pattern for input path.

@xzyfer
Copy link
Contributor

xzyfer commented Mar 16, 2015

Totally agree. This also worth a look at for globbing https://github.com/jonschlinkert/micromatch

jiongle1 pushed a commit to scantist-ossops-m2/node-sass that referenced this issue Apr 7, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

No branches or pull requests

4 participants