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(deps): update chokidar to v4 #5256

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

ggodlewski
Copy link

@ggodlewski ggodlewski commented Nov 16, 2024

PR Checklist

Overview

Current chokidar is bundled with tons of spam deps including braces which trigger some audit alert.

Uploading image.png…

All test are working. It supports globs unlike #5215

Copy link

linux-foundation-easycla bot commented Nov 16, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey thanks for getting this started! Sorry it took so long to review - since Chokidar didn't end up needing to be bumped with v11, this fell by the wayside. But I'm excited to help push this forward along now! 🚀

Requesting changes primarily on a performance point, as well as a design point. Happy to talk if you want. 🙂

lib/cli/watch-run.js Outdated Show resolved Hide resolved
lib/cli/watch-run.js Outdated Show resolved Hide resolved
lib/cli/watch-run.js Outdated Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author waiting on response from OP - more information needed label Jan 2, 2025
@ggodlewski
Copy link
Author

Hey @JoshuaKGoldberg, there is no way I remember the details after a month :). All I know it's been a struggle to get all tests working. In the meantime we switched to ava to resolve our issues, so I can't do much.
I updated the code to fix your issues. If there is anything that I can do fast let me know.

@Uzlopak Uzlopak changed the title chore(deps): update choikdar to v4 chore(deps): update chokidar to v4 Jan 2, 2025
@talentlessguy
Copy link

@JoshuaKGoldberg is there any blocker to this PR besides merge conflicts of a lockfile?

@JoshuaKGoldberg
Copy link
Member

Just a re-request for review. I'll add it to the queue for review :)

@JoshuaKGoldberg JoshuaKGoldberg added status: needs review a maintainer should (re-)review this pull request and removed status: waiting for author waiting on response from OP - more information needed labels Jan 8, 2025
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Source code looks good, thanks for the refactor!

I finally found time to really dig in and try this in production - and think I found a performance bug. LMK what you think of it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug] I tried this PR out on real-world code and found a performance issue from files being renamed. I found it on the ESLint codebase but also reproduced it in isolation here: https://github.com/JoshuaKGoldberg/repros/tree/mocha-chokidar-v4-pr-watching. Could you please take a look?

tl;dr: in a minimal codebase with two root-level test files, re-watching is reported as taking >200ms

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How exactly are you renaming the file? I run mv b.test.js z.test.js and I don't have the state with 0 passing:

$ node ../mocha/bin/mocha.js "*.test.js" --watch
  a
    ✔ passes
  b
    ✔ passes
  2 passing (2ms)

ℹ [mocha] waiting for changes...

  a
    ✔ passes
  b
    ✔ passes

  2 passing (1ms)

ℹ [mocha] waiting for changes...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I did it through the VS Code UI. This is on an m1 Mac.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm running Ubuntu. I have no hardware to reproduce it. It's strange because the tracker.regenerate(); is only triggered on event === 'add'. I don't see how the list of files can be empty during rename of 1 file. Maybe you can add some debug output so we can trace the tracker method calls?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 added to the README.md. Interestingly, the long delay of ~200ms only happens if an npm install populated node_modules/. If you rm -rf node_modules/ then the delay is only around 20-30ms.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added one more improvement. Glob should not traverse node_modules. Can you check if it improves anything?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 that fixed it for me! Back to 0-1ms on the middle waiting.

+1 that it's a generalized solution too, not specific to node_modules. Nice 🙂

@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author waiting on response from OP - more information needed label Jan 28, 2025
@JoshuaKGoldberg JoshuaKGoldberg removed the status: waiting for author waiting on response from OP - more information needed label Jan 30, 2025
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥 This is fantastic, thanks a bunch @ggodlewski! I really appreciate you sticking through & working with me on the reviews.

I've played with this a bunch locally on relatively small and large repros, and haven't found any issues. I'd really prefer to get another review from someone on @mochajs/maintenance-crew just to be safe - but otherwise ✅ on my end.

Nice job!

lib/cli/watch-run.js Outdated Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg removed the status: needs review a maintainer should (re-)review this pull request label Jan 30, 2025
Co-authored-by: Josh Goldberg ✨ <[email protected]>
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.

Chore: bump chokidar to v4
3 participants