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

chore: bump chokidar to v4 #2235

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

chore: bump chokidar to v4 #2235

wants to merge 1 commit into from

Conversation

zjbob
Copy link

@zjbob zjbob commented Nov 7, 2024

Chokidar v4 removed a bunch of its dependencies.
Implicit dependecies, such as braces, have critical issues.

  • braces's issue is particular is: Uncontrolled resource consumption

This issue was brought to my attention from Dependabot in my project and is the reason I am proposing this change.

Most notable is that Chokidar v4 reduced its dependencies from 13 to 1.

Updating Chokidar implicitly changes the requirement of supported node to >=14.16.0 since it is a requirement of Chokidar's now only dependency:

  • readdirp

Due to the minimum requirement of node being changed to 14.6, I believe that this change requires a nodemon major version bump. I will defer this decision and action to the repo maintainer.

Following are edits by @remy

  • Tested on MacOS
  • Tested on Windows cmd
  • Tested on Windows powershell
  • Tested on Windows WSL (jeez, this list goes on)
  • Tested on linux

Chokidar v4 removed a bunch of its dependencies.
Implicit dependecies, such as braces, have critical issues.
  braces's issue is particular is Uncontrolled resource consumption

Updating Chokidar implicitly changes the requirement of supported
node to >=14.16.0 since it is a requirement of its dependency:
  readdirp
Copy link

netlify bot commented Nov 7, 2024

Deploy Preview for nodemon ready!

Name Link
🔨 Latest commit a64b064
🔍 Latest deploy log https://app.netlify.com/sites/nodemon/deploys/672cf20a30c56400082c1f46
😎 Deploy Preview https://deploy-preview-2235--nodemon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@zjbob
Copy link
Author

zjbob commented Nov 11, 2024

@remy When you have a spare moment, curious if you have any thoughts

@remy
Copy link
Owner

remy commented Nov 27, 2024

I've not run tests on this yet, but I'm wary of creating a major version bump (because support is reduced).

Nodemon doesn't have any telemetry (though there is an updater) so I can't tell with any degree at all how many devs are still on node@12.

Just adding this comment so people understand where I'm coming from. I'll do some digging into the pr to understand the change that Chokidar brings and where the lost support is.

@remy remy added the not-stale Tell stalebot to ignore this issue label Nov 27, 2024
@remy
Copy link
Owner

remy commented Nov 27, 2024

Note for testing - CI fails on this test file: test/monitor/count.test.js

@remy
Copy link
Owner

remy commented Nov 27, 2024

Related issue on Chokiar to see if node@12 can be supported through polyfill: paulmillr/chokidar#1390

@benmccann
Copy link

Note for testing - CI fails on this test file: test/monitor/count.test.js

I think there might be a couple of other test failures as well. I looked at them briefly. I think that the issue is that the nodemon watcher ignore uses glob patterns and glob support was dropped in chokidar 4 in favor of a filter function. If those glob patterns can be replaced that would be the best solution.

@benmccann benmccann mentioned this pull request Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-stale Tell stalebot to ignore this issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants