Update chokidar to 3.3.0

[coreyward]

Regardless of what happens in watchpack 2.0.0, this fix will sidestep node-gyp issues for at least several hundred macOS Catalina users. The tests still turn up green w/o changes, but perhaps the coverage is poor? Fixes #130.

[jsf-clabot]

All committers have signed the CLA.

[alexander-akait]

It is breaking change

[coreyward]

@evilebottnawi Please provide a failing test. If you’re not willing to do the work to make this function I need to know where it fails.

[RDIL]

v3 requires at least node 8, maybe it is time to drop smaller versions?

[sokra]

The tests still turn up green w/o changes

Installation fails to start on node 6, which seem to be expected as node 6 support was dropped.

But the tests are also failing on node 8 on linux and windows (not on mac). This is unexpected.

[sokra]

The problem seem to be caused by bugs in readdirp. These bugs are already fixed in a newer version. I was able to fix it locally by enforcing readirp@3.4.0, but this is not in the current range of chokidar. We need a new chokidar version pushing readdirp to 3.4.0. @paulmillr

[sokra]

hmm this fixes only the problems on linux. Windows is still broken on node.js 8

[paulmillr]

Pushing new chokidar is simple. What exactly is broken on node 8 @ windows? Hard to tell from tests.

[paulmillr]

BTW I suggest to switch to GitHub actions. It supports all three platforms.

[paulmillr]

Doesn't seem like the close method was updated to use asynchronous chokidar.watcher.close - the only API change in chokidar 3. We've decided to adjust it because it's not possible to stop all IO syncronously.

See this pull request for reference: webpack/webpack-dev-server#2539

[sokra]

What exactly is broken on node 8 @ windows? Hard to tell from tests.

Not sure yet. Errors are weird.

On linux node 8 it also logs:

(node:5243) UnhandledPromiseRejectionWarning: TypeError: this._removeWatcher is not a function
    at DirEntry.remove (/home/travis/build/webpack/watchpack/node_modules/chokidar/index.js:161:14)
    at <anonymous>
(node:5243) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:5243) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
(node:5243) UnhandledPromiseRejectionWarning: TypeError: this._removeWatcher is not a function
    at DirEntry.remove (/home/travis/build/webpack/watchpack/node_modules/chokidar/index.js:161:14)
    at <anonymous>
(node:5243) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 2)

remove seem to be an async method but none of its callsites handle the returned promise. Seems remove should handle the case that the DirEntry can be disposed duing the async readdir operation.

[sokra]

Doesn't seem like the close method was updated to use asynchronous chokidar.watcher.close - the only API change in chokidar 3. We've decided to adjust it because it's not possible to stop all IO syncronously.

I added a catch handler there. The operation can continue in background, consumers of watchpack are not interested when the watcher is fully stopped. I assume it won't emit change events any longer once close has been called.

[sokra]

What exactly is broken on node 8 @ windows? Hard to tell from tests.

Not sure yet. Errors are weird.

I guess it's the readdirp thing. appveyor CI incorrectly runs npm install instead of yarn, which ignores the resolutions workaround I added.

[paulmillr]

Pushing 3.4 in a bit.

[codecov]

Codecov Report

Merging #153 into watchpack-1 will decrease coverage by 0.64%.
The diff coverage is 81.25%.

@@               Coverage Diff               @@
##           watchpack-1     #153      +/-   ##
===============================================
- Coverage        95.22%   94.57%   -0.65%     
===============================================
  Files                3        4       +1     
  Lines              356      369      +13     
  Branches            97       98       +1     
===============================================
+ Hits               339      349      +10     
- Misses              17       20       +3     

Impacted Files	Coverage Δ
lib/DirectoryWatcher.js	94.62% <80.00%> (-0.38%)	⬇️
lib/chokidar.js	81.81% <81.81%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88e13a6...f53f6c7. Read the comment docs.

[paulmillr]

Is there any point in keeping old chokidar around? webpack-dev-server switched to v3 completely. You'll just keep seeing v2-related warnings.

[sokra]

Is there any point in keeping old chokidar around?

Yep to not break semver. We must keep support for node.js 6 in this major version. As chokidar doesn't support node 6, we fallback to old chokidar for node 6.

These warnings seem to be a bit weird anyway. Why do you need to tell someone that it doesn't work for node 14 when they not using node 14 anyway? These warnings should only appear for node 14 users. Or even better would be if node 14 users get an error.

[paulmillr]

Because it can work. Or it can break. It doesn't always break. We've been receiving bug reports over and over regarding old versions. The amount of bug reports keeps getting increased for those. We need to make users upgrade at some point.

[surfaceowl]

@sokra

...Why do you need to tell someone that it doesn't work for node 14 when they not using node 14 anyway? These warnings should only appear for node 14 users.

I prefer the warnings so I can see potential problems in advance, rather than upgrading node and then discovering a branch won't build.

Also - chokidar bumped up to @v3.4.0 a few weeks ago, looks to be version bump on internal library for typing

https://www.npmjs.com/package/chokidar

[sokra]

Thanks