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

Switch to Chokidar. #844

Closed
wants to merge 2 commits into from
Closed

Switch to Chokidar. #844

wants to merge 2 commits into from

Conversation

paulmillr
Copy link

Closes gh-636. Tests fail because of something else.

To summarize: chokidar is very stable, doesn't have 52 open issues and is much faster on OSX, with less CPU usage because of fsevents.

@am11
Copy link
Contributor

am11 commented Apr 9, 2015

Thanks for the contribution! :)

It is failing AppVeyor CI Windows tests because npm writes warnings to stderr, AppVeyor chokes on switching sub-processes (like npm install) within a job, if anything was written on stderr. One solution is to try-catch the require() of optionalDependencies in Chokidar's code. With the current state of this PR, it will prevent us from running ~750 tests on Windows based CI.

@paulmillr
Copy link
Author

We do this already. https://github.com/paulmillr/chokidar/blob/master/lib/fsevents-handler.js

In fact, it was failing before this commit.

@paulmillr
Copy link
Author

Hmm I think I saw some packages that use chokidar and are tested by appveyor. Need to find these

@paulmillr
Copy link
Author

@am11
Copy link
Contributor

am11 commented Apr 9, 2015

Hmm, then I think we should remove ps from appveyor.yml#L23.

In fact, it was failing before this commit.

Nope, the current master is not failing: https://ci.appveyor.com/project/sass/node-sass/build/889.

@paulmillr
Copy link
Author

Ah, i've meant the tests are failing on my machine. I will provide logs later (were doing npm test)

@am11
Copy link
Contributor

am11 commented May 8, 2015

Ping 🔔
{{Pong}}

@paulmillr
Copy link
Author

@am11 what's required from me? That doesn't seem like Chokidar error.

Just tested it locally — all tests pass.

@@ -20,6 +20,5 @@ install:
- node --version
- npm --version
- git submodule update --init --recursive
- ps: npm install --msvs_version=2013
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just remove ps: prefix, not the entire statement.

- npm install --msvs_version=2013

@am11
Copy link
Contributor

am11 commented May 9, 2015

LGTM! :shipit:
Thanks @paulmillr. 👍

@xzyfer, if this has been triaged with CI tests, then it can probably be considered for vNext as a non-breaking infrastructure change. Thoughts?

@paulmillr
Copy link
Author

@xzyfer ping.

@am11 am11 mentioned this pull request Jul 2, 2015
@marlun
Copy link

marlun commented Dec 30, 2015

Curious on that status of this ticket since when installing node-sass now you get a warning that it uses a deprecated version of lodash. The reason is that node-sass is using an old version of gaze (0.5.* instead of 0.6.*). Maybe you could try the new version until you change to Chokidar?

@xzyfer
Copy link
Contributor

xzyfer commented Dec 30, 2015

We've been waiting to see if Chokidar would gain traction.

As for gaze, the latest dist-tag is current 0.5.2. If/when the maintainer updates the 0.6.x to be latest we'll update.

@paulmillr
Copy link
Author

@xzyfer curious what do you actually mean by "traction".

chokidar: 463202 downloads in the last week
gaze: 376144 downloads in the last week

@xzyfer
Copy link
Contributor

xzyfer commented Dec 30, 2015

@paulmillr I understand what you're saying. Chokidar has clearly gained traction since this PR opened.

To be more explicit there are a couple reasons we have not moved forward on this:

  • our current watcher gives us little to no trouble
  • most people use gulp or grunt watch for node-sass

At the moment, as far as I know, neither grunt or gulp has switch to chokidar in their stable releases.
I am also aware that gulp has switched to chokidar in their unstable 4 branch, which is exciting.

Once gulp 4 is stable a large portion of our user base will be using chokidar and the migration will be a much smaller support risk.

I hope you understand. We're a very small team and we hugely appreciate your contribution.

@saper
Copy link
Member

saper commented Dec 31, 2015

We also had to make a one step back - all tests for the CLI watcher have been removed, because they were poorly written. We will need those tests back.

@lo1tuma
Copy link

lo1tuma commented Mar 2, 2016

Any update on this?

@saper saper force-pushed the master branch 2 times, most recently from 6c128d9 to 1e4bba8 Compare April 19, 2016 21:45
@michaelwayman
Copy link

in the meantime checkout https://github.com/michaelwayman/node-sass-chokidar

npm install node-sass-chokidar

@realityking
Copy link
Contributor

I think this has been taken care of with 80fc103

@xzyfer
Copy link
Contributor

xzyfer commented Apr 9, 2018

Correct

@xzyfer xzyfer closed this Apr 9, 2018
@xzyfer xzyfer added the v5 label Apr 9, 2018
@paulmillr
Copy link
Author

😸

@nschonni nschonni added this to the 5.0 milestone Dec 20, 2018
@mcmire
Copy link

mcmire commented Apr 10, 2019

Hey, what's the status on this? It seems like node-sass is still using Gaze and the commit linked to wasn't in fact merged in. Did it get reverted?

@tofran
Copy link

tofran commented May 6, 2019

@mcmire Looks like it has been merged to v5 more information in #2111

@mcmire
Copy link

mcmire commented May 6, 2019

Ooh, good to hear! Thanks!

@realityking
Copy link
Contributor

It looks like this didn't land in the final v5 release. What happened?

jiongle1 pushed a commit to scantist-ossops-m2/node-sass that referenced this pull request Apr 7, 2024
Correctly bubble keyfrome directives
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to a better file watcher