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

Bump chokidar to v4 #2347

Merged
merged 1 commit into from
Sep 16, 2024
Merged

Bump chokidar to v4 #2347

merged 1 commit into from
Sep 16, 2024

Conversation

ntkme
Copy link
Contributor

@ntkme ntkme commented Sep 16, 2024

Chokidar v4 targets node >=14, which is the same as the current sass package, so there is no breaking change for end users. See: https://github.com/paulmillr/chokidar?tab=readme-ov-file#upgrading

@jathak jathak merged commit a957eea into sass:main Sep 16, 2024
38 checks passed
@ntkme ntkme deleted the chokidar-4 branch September 16, 2024 20:41
sciborrudnicki added a commit to angular-package/dart-sass that referenced this pull request Sep 16, 2024
@nex3
Copy link
Contributor

nex3 commented Oct 7, 2024

The new Chokidar version has caused a couple end user issues (#2362, #2373). It may be worth considering rolling it back until Chokidar v4 is more stable. @ntkme, is there a particular reason you wanted to do this update, or was it just general dependency hygiene?

@ntkme
Copy link
Contributor Author

ntkme commented Oct 7, 2024

v3 was very flaky in our GitHub CI, and I was mainly trying to see if v4 is less flaky. It seems like that it’s less flaky in CI but is giving end users lots of trouble.

Maybe we should revert this. On the other hand, maybe we should consider other options like https://www.npmjs.com/package/@parcel/watcher, although it doesn’t have a real “poll” mode.

@nex3
Copy link
Contributor

nex3 commented Oct 7, 2024

The lack of a polling watcher isn't an insurmountable problem, but it would require some engineering effort to deal with (possibly by moving the Dart watcher package's PollingDirectoryWatcher to a dart:io-independent library and adding the ability to pass in a custom filesystem; possibly by just running brute-force Parcel queries in a loop by hand). Otherwise, the Parcel watcher does look good. But maybe it's better to just go back to chokidar v3 for now until we can put in that work.

@ntkme
Copy link
Contributor Author

ntkme commented Oct 8, 2024

running brute-force Parcel queries in a loop by hand

I actually did a POC for that at some point and concluded it was flaky and kind of dirty (with what it supports as of now). The problem is that the “query” mode requires user to first write a snapshot to disk, and later “query” the current state against the previous snapshot. This introduces two ugly issues: 1. It does not support snapshot in memory, meaning each directory watched needs at least one temporary file on disk for snapshot, which needs clean up. 2. It does not have atomic “query and update snapshot”, meaning depends on whether we update snapshot or query first at each interval, we either risks double firing or missing events due to non-atomic operation.

Other than not having a good polling solution, pracel/watcher has been pretty good in the POC I did.

@nex3
Copy link
Contributor

nex3 commented Oct 8, 2024

Ah, that's too bad. We may need to rely on the Dart logic in that case.

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

Successfully merging this pull request may close these issues.

Upgrade to chokidar 4
3 participants