Disable fsevents by default #60
Conversation
Since it would be a Semver-Major change to update chokidar this seems like the next best thing. This explicitly opts-out of using fsevents when instantiating chokidar as a default option. Folks could still manually change this behavior if they wished. While this is a behavior change it should only make things "slower" not "less reliable" and would avoid the breakages we are seeing on 12 / 14 which appear to be caused by .stop() being called on the watcher when nothing is being watched, which might be a race condition in the tests.
| @@ -13,6 +13,7 @@ var defaultOpts = { | |||
| ignored: [], | |||
| ignoreInitial: true, | |||
| queue: true, | |||
| useFsEvents: false, | |||
There was a problem hiding this comment.
alternative we could sniff version number and only apply this extra option for OSX + >=12.x
There was a problem hiding this comment.
I like sniffing. Might even want to include the patch version. We have some patterns for that so I'll look after work.
|
Hmm, this causes our test suite to randomly fail so I don't think I can ship it 🤔 |
@MylesBorins could that be solved by not calling |
|
Could you reopen this? I'd love to dig into the fails a bit and figure out what is going on here. It passed locally, and the change should have 0 observable change on non osx. I can do some local tests as well with what's going on with close... But keep in mind the fsevents crashes are happening on both 12 and 14, that specific backtracke we did was only on 12 |
|
With these changes, I am getting flakey failures on MacOS 10.15.5 - note that we don't run MacOS in our current CI but our new project scaffold will update our CI to GH Actions and testing on Mac. |
|
@phated which versions of node are you seeing the flaky failures on? I'm not able to reproduce on MacOS 10.15.5 using Node.js v10.22.0, v12.18.3, or v14.6.0 Are the failures you are seeing related to time outs? FWIW I am consistenly getting Abort traps (internal failures) on 12.18.3 and 14.6.0 and they are entirely related to the calls to |
|
Mac 10.15.5, Node 12.18.3 - with this patch I'm getting (random) tests that never complete, even with a timeout of 0 set (disabled). I haven't had glob-watchers' test suite fail yet while working on the 2 patches I released last night. |
|
@phated that is extremely strange because I cannot get the test suite to pass at all on Mac OSX 10.15.5 with 12.18.3. Do you have time to pair on this at all tomorrow? edit: to be clearer, the test suite doesn't pass without this change... so very odd that we are having opposite experiences |
|
how did you install node? |
|
|
|
Which version of npm?
When you run `npm install` do you see fs-events being compiled? You may
need to blow away node_modules and re-install
which version of xcode?
…On Thu, Jul 23, 2020 at 4:18 PM Blaine Bublitz ***@***.***> wrote:
nave usemain 12 and then verified the version. Looks like I got the binary
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#60 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADZYVZOCXOCNKEEB2IOKADR5CLLFANCNFSM4PEBJ3BQ>
.
|
|
Sorry, I'm at my work computer right now, not my personal. I'll have to check later. |
|
@MylesBorins you were right, I must have built fsevents on a non-broken node and then switched to the newly broken node. Rebuilding causes the abort traps |
|
@phated are you able to reproduce the failures you say you were seeing with this change? I could not get any flakes with this change |
|
It's look like about 1 in 10 runs will have a test that just won't complete. I'm not sure why. |
|
@MylesBorins I'm also wondering if this actually affects any end-users because we don't recommend or guide users to use |
|
The gulp test suite is failing too.
The big TL;DR is that the version of fsevents that is shipping with the
version of chokidar you use is not only unsupported on 12.x and 14.x but is
actively flaky and causing issues. It might only be rearing its head in the
test suite when calling watch.close, but I would be unsurprised if there
weren't a bunch of other ways it was broken.
I think disabling, at least on 12 / 14, until you can update versions is
the most prudent thing you can do. Does the test suite work at all without
this change? I am completely unable to get the test suite to work without
it.
…On Fri, Jul 24, 2020 at 1:29 PM Blaine Bublitz ***@***.***> wrote:
@MylesBorins <https://github.com/MylesBorins> I'm also wondering if this
actually affects any end-users because we don't recommend or guide users to
use watcher.close() in their applications. So is this just a workaround
for the test suite?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#60 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADZYV6W54ATHDZOEZZYU5TR5HAFZANCNFSM4PEBJ3BQ>
.
|
|
Can you just update chokidar? last version use newer version of fsevents which shouldn't have issues with node 12 |
Since it would be a Semver-Major change to update chokidar this seems
like the next best thing. This explicitly opts-out of using fsevents
when instantiating chokidar as a default option. Folks could still
manually change this behavior if they wished. While this is a behavior
change it should only make things "slower" not "less reliable" and
would avoid the breakages we are seeing on 12 / 14 which appear to be
caused by .stop() being called on the watcher when nothing is being
watched, which might be a race condition in the tests.