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

Bring back chokidar v3 #130

Closed
paulmillr opened this issue Nov 2, 2019 · 26 comments
Closed

Bring back chokidar v3 #130

paulmillr opened this issue Nov 2, 2019 · 26 comments

Comments

@paulmillr
Copy link
Contributor

paulmillr commented Nov 2, 2019

We've released Chokidar 3 in April. See my post on that: Chokidar 3: How to save 32TB of traffic every week with one NPM package

The changes are pretty big. First of all, the package is 16 times smaller and uses 15 dependencies instead of 201. We've switched to n-api, which means users won't download fsevents binaries anymore. n-api is included in every nodejs installation, and it's tiny. Directory walking has been improved massively with stream implementation of readdirp. Massive RAM & CPU improvements etc.

I really suggest switching back to chokidar. You'll receive tons and tons of bug reports otherwise, since file watching is a complex task. You're using fs.watch — it is unreliable and doesn't solve the issue. For example, i've spent a couple days this week trying to fix race conditions appearing due to async nature of nodejs IO. That's why VS Code uses chokidar.

I can submit a pull request if you'll agree.

@alexander-akait
Copy link
Member

alexander-akait commented Nov 5, 2019

@paulmillr thanks for issue, we are in beta testing and nobody report about problems, maybe you can provide some examples where we potentially can be broken?

@paulmillr
Copy link
Contributor Author

CPU & RAM usage in big projects. Did you do any tests for that? Symlink support. Support for every editor and ide e.g atomic writes.

@alexander-akait
Copy link
Member

@paulmillr As I said earlier, we are in beta and no reports on very large projects, but i believe you. Maybe you can reproduce problems with the current version and show them?

@coreyward
Copy link

coreyward commented Mar 31, 2020

Can this issue please get some attention? This package is single-handedly holding back adoption of Chokidar (and thus, creating a bunch of issues for macOS Catalina users thanks to fsevents needing node-gyp to build, and node-gyp doesn't work right on Node 12+ on macOS Catalina) on every single webpack project. Most users aren't going to realize that watchpack is even to blame because they aren't going lockfile spelunking, but I've been bit by this over and over. I actually typically just force the resolution of chokidar to 3.3.0, but I'd prefer not to have to do that on every single project I touch.

@coreyward
Copy link

@sokra It looks like you're pretty active on this repo—perhaps you can see some reason w/r/t the above and make this possible?

@paulmillr
Copy link
Contributor Author

My suggestion here would be to release webpack-major with min node 8.16+ or 10+ and current (webpack 4) codebase while moving "v5" to "v6". You will have more time to evaluate options. Meanwhile, all the users would receive massive performance, RAM, CPU and dependency count improvements. Most importantly, chokidar 3 won't download binaries on EVERY npm install.

@alexander-akait
Copy link
Member

@paulmillr Sorry, it is really breaking change and can break some stuff, maybe we can do branch v3-node6 with supporting node@6 and release it, it is allow to update many stuff and avoid breaking changes, I known it is not good idea, but I can't see other ideas?

@paulmillr
Copy link
Contributor Author

@evilebottnawi No, we cannot. N-API is only supported since node 8.0. And it was stabilized in node 8.16. That's the whole point. Without it it was not possible to create fsevents v2 that doesn't host binaries on AWS.

On a side note, the AWS S3 binary hosting account is not controlled by any of fsevents maintainers (pipobscure or me). So it could potentially distribute malware to anyone who downloads fsevents v1.

Node.js v8 stopped receiving security updates 3 months ago, on Dec 31, 2019.
Node.js v6 stopped receiving security updates 1 year ago.

I don't suggest bumping requirement to node v10. Just to node v8. And releasing a new Webpack release that would show it contains breaking change, you could name it Webpack 5.

@alexander-akait
Copy link
Member

@paulmillr We can't do it, we already have webpack@5 and new version uses watchpack without chokidar, for webpack@4 it will be big breaking change

@paulmillr
Copy link
Contributor Author

@evilebottnawi I don't see v5 released. So, you don't really have it. It's in beta. Beta users should know about unstable software. Just bump v4 to v5.

It's either version bump to v5, or keeping insecure dependencies which can totally distribute malware. Even worse: malware on insecure / unsupported node.js version, so a vulnerability could appear in node v6, malware would abuse it, and nothing would be done about that.

You can also bring back chokidar v3 to webpack 5 as i've mentioned in the first post. What's the point in writing your own file watcher if it won't be as performant as chokidar+fsevents anyway? You will spend 400 hours on fixing watchpack issues which will appear at some point, or you could just use chokidar like you've did before, and combine our community effort. Of course, current watchpack implementation may work for basic cases, but there's a reason why file watching is hard.

@alexander-akait
Copy link
Member

It's either version bump to v5, or keeping insecure dependencies which can totally distribute malware.

webpack@5 is not ready yet to release

there's a reason why file watching is hard

Please provide cases as I written above

A lot of projects try webpack@5 and very few problems with watching (they fixed)

@paulmillr
Copy link
Contributor Author

paulmillr commented Apr 1, 2020

@evilebottnawi webpack@5 is not ready to release. So, bump v4 to v5 WITHOUT changes, and delay v5 to v6. That's simple. You can do the bump, or you can take responsibility if/when malware would be distributed. There is no third option.

As for "fixed" problems. Do 686 closed chokidar issues during 2012-2019 mean chokidar is fixed to you? Any software may seem without problems during some particular moment. But no software is without bugs. I won't point you to any particular issue; all i'm saying is that problems will appear, and you will spend time fixing them — a lot of time, to cover all edge cases.

@alexander-akait
Copy link
Member

alexander-akait commented Apr 1, 2020

So, bump v4 to v5 WITHOUT changes, and delay v5 to v6.

No, we have release plan for each verison

As for "fixed" problems. Do 686 closed chokidar issues during 2012-2019 mean chokidar is fixed to you? Any software may seem without problems during some particular moment. But no software is without bugs. I won't point you to any particular issue; all i'm saying is that problems will appear, and you will spend time fixing them — a lot of time, to cover all edge cases.

This is not very constructive, with that logic, you can argue that any new software is bad initially

@coreyward
Copy link

Why not simply release a backwards compatible version of watchpack v1 with chokidar 3?

@alexander-akait
Copy link
Member

@coreyward Because we support node@6 and node@8 in watchpack, also it can break code in some cases, because API for chokidar@2 and chokidar@3 is difference

christopherthielen added a commit to ui-router/core that referenced this issue Apr 19, 2020
…s in a row.

without this yarn resolution, karma runner fails on mac with 'fsevents is not a constructor'
see: webpack/watchpack#153 webpack/watchpack#130
christopherthielen added a commit to ui-router/core that referenced this issue Apr 19, 2020
…ks in a row.

 without this yarn resolution, karma runner fails on mac with 'fsevents is not a constructor'

 see:
 webpack/watchpack#153
 webpack/watchpack#130
mergify bot pushed a commit to ui-router/core that referenced this issue Apr 19, 2020
…ks in a row.

 without this yarn resolution, karma runner fails on mac with 'fsevents is not a constructor'

 see:
 webpack/watchpack#153
 webpack/watchpack#130
@sokra
Copy link
Member

sokra commented Apr 24, 2020

Let's assume we don't care about breaking semver for node@6 since hopefully nobody uses it anyway, or doesn't care about upgrading dependencies and would not upgrade webpack anyway.

Is there any other breaking change in chokidar 2 to 3?

@sokra
Copy link
Member

sokra commented Apr 25, 2020

On a side note, the AWS S3 binary hosting account is not controlled by any of fsevents maintainers (pipobscure or me). So it could potentially distribute malware to anyone who downloads fsevents v1.

If this is really true and you've lost control over binary hosting you should issue a security advistory and publish a version which points to a new hosting location.

fsevents@1 is still used pretty often: >450k references in yarn.locks on github

@paulmillr
Copy link
Contributor Author

paulmillr commented Apr 25, 2020

Is there any other breaking change in chokidar 2 to 3?

No. I’ve worked hard to ensure there are none. The only one is making “close()” method asynchronous, for which i’ve provided 3 line patch for webpack-dev-server.

@csvan
Copy link

csvan commented May 4, 2020

This should be considered a high priority issue as Nodejs 14 has now been stable for some time and is explicitly incompatible with Chokidar v2:

webpack > watchpack > [email protected]: Chokidar 2 will break on node v14+. Upgrade to chokidar 3 with 15x less dependencies.

@youcandanch
Copy link

"I really suggest switching back to chokidar. You'll receive tons and tons of bug reports otherwise, since file watching is a complex task." @paulmillr

I've been absolutely struggling to get webpack watching on v5 working with a not-too-complex React/Redux app within Docker. It works totally fine on my Linux host, but once it's in a Docker container, it's like flipping a coin to see whether or not events get properly propagated. I'm going to drop a more robust repro case in the Webpack 5 beta thread, but it's worth noting here that I'm concerned about moving to 5 just because of Watchpack 2 alone.

@polarathene
Copy link

polarathene commented Jun 22, 2020

I'm confused, what is the motivation to ditch chokidar for the next major version? Paul makes some pretty valid points, and afaik there isn't any reason to not stick to v3?

If you later find Paul's advice is right and ends up causing more maintainership burden as users like @youcandanch have brought up, then won't it be an issue to switch back to chokidar after v5 is released?

The time between the v4 and v5 releases has been quite a while, if you're going to potentially take this risk with v5 of webpack, will a switch back to chokidar need to wait on a v6 release of webpack?

How many beta users are you getting testing v5 vs the amount of v4 users? Is it possible that the type of users opting to beta test may not cover a wide variety of use cases that will cause a tonne of bugs upon release to the wider community?

It would seem wise to leverage the experience of the chokidar project which specializes at that task, rather than risk regressions unless there is a valid reason to drop chokidar?

@kane-mason
Copy link

#165 FYI our watch is failing silently after updates to use chokidar@3

@osddeitf
Copy link

osddeitf commented Jul 2, 2020

Can't wait v2.0.0 to be stable, as chokidar@2 still in dependencies:

"chokidar": "^3.4.0",
"watchpack-chokidar2": "^2.0.0"

@paulmillr
Copy link
Contributor Author

re: kane-mason's comment — unrelated to chokidar 3, this is related to graceful-fs module which monkey-patches fs and has a long history of similar issues

@Hackwar
Copy link

Hackwar commented Aug 2, 2020

So it's 9 months since the initial report and you are still in beta and the issue is still persisting. We are using it in a rather big project and more or less regularly get reports from others "Hey, your dependencies are outdated and insecure." Please either finally release webpack@5 or follow along with @paulmillr s proposal to do another major release before putting out your latest changes.

@polarathene
Copy link

do another major release before putting out your latest changes.

Webpack 5 beta is seeing more adoption in other projects beta support now, like the next.js framework just had a new release with support for Webpack 5 beta as a preview, where they've also been writing new code for future next.js releases that integrates with that beta IIRC, may be starting to get too late to take this major version change.

EDIT: Oh, my bad you were talking about a release of webpack 5, but the major version release only needs to be done for watchpack afaik.


We are using it in a rather big project and more or less regularly get reports from others "Hey, your dependencies are outdated and insecure."

This might be a useful workaround in the meantime?: https://classic.yarnpkg.com/en/docs/selective-version-resolutions/

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants