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

feat: replace sane with chokidar #10048

Closed
wants to merge 3 commits into from

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented May 15, 2020

Summary

sane pulls in old version of micromatch. We're also already vendoring their watchman watcher (#5387) and the fsevents watcher they removed (#8258). Replacing with chokidar allows us to drop that last vendoring as chokidar abstracts over it.

We might need to create a chokidar abstraction here, I just did the minimal to make it work.

This is very untested

Test plan

Dunno. It works for the simple cases I played with locally, but it needs real usage.

glob: patterns,
ignored: ignorePattern,
})
: chokidarWatch(patterns, {
Copy link
Member

Choose a reason for hiding this comment

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

Surprising but .. they expose almost the same API? :O

Copy link
Member Author

Choose a reason for hiding this comment

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

I dunno if the ignore pattern works, should probably verify

Comment on lines +810 to +822
(watcher as ChokidarFsWatcher).on('all', (type, filePath, stat) => {
onChange(type, filePath, root, stat);
});
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the part that makes me think creating some abstraction makes sense. There might be other things we want to tweak as well

Copy link
Member

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Goodbye sane, and goodbye @cnakazawa/watch. Nice work, I'm excited about this change as it took years :D

@cpojer cpojer mentioned this pull request May 15, 2020
@SimenB
Copy link
Member Author

SimenB commented May 15, 2020

Hmm, #5387 imports some stuff from sane

image

We should vendor it, I guess

@SimenB SimenB force-pushed the replace-sane-with-chokidar branch 2 times, most recently from 38a3a33 to 350f151 Compare August 6, 2020 12:15
@arhelmus
Copy link

arhelmus commented Sep 14, 2020

Hey @SimenB, thanks for your amazing work, its actually should also improve DX for Windows users of Metro. What is currently blocking you from merging this PR?

@SimenB
Copy link
Member Author

SimenB commented Sep 14, 2020

@arhelmus no real blocker, just time and energy to work on this. It'll require extensive testing, so I haven't really found the time to do it.

This is the part of the code base I've touched the least (along with watch mode, which also mostly is haste-map), so I'm a bit hesitant to merge it. I can rebase it to resolve conflicts, tho

@arhelmus
Copy link

arhelmus commented Sep 14, 2020

@SimenB I would love to help you to test it on Mac and Windows, but I missing lots of context. Can you provide some test plan which I can follow. The best I see now is: create new file/rename it/upper lower case it/delete and see how watchers will react.

@SimenB
Copy link
Member Author

SimenB commented Sep 14, 2020

Yeah, that's essentially it - make sure you're disabling watchman (by either not having it installed or by passing --no-watchman).

Note that the linting errors now (#10048 (comment)) need to be solved first. We don't really have many tests for watch mode on CI which is why it's passing even though those imports does not resolve presumably).

@mikemoate
Copy link

Given amasad/sane#159 (upstream dependency execa has critical vuln) and that sane is basically unmaintained, this PR would be really helpful!

@SimenB
Copy link
Member Author

SimenB commented Oct 16, 2020

Feel free to open PRs against my branch (or opening a new PR based on this branch) fixing the type error. This will need to be tested thoroughly, but once we have a green PR I can make alpha releases

@SimenB
Copy link
Member Author

SimenB commented Dec 5, 2020

Testing this, it's horribly slow during first run - about 50x regression over what's currently in master. I think this is due to it reading (or at least stating) all files it finds during initial crawl instead of just registering a watcher.

In addition, on change it emits change events for node_modules - I was unable to figure out how or where. In monorepos any local files are usually symlinked in node_modules, meaning they all triggered a change event (even worse, it happened staggered. So changing one file which ended up regenerating a bunch of .d.ts changes made the tests re-run 10-12 times on my machine).


So instead of spending time trying to figure out how to make chokidar do what I want, I think it's better to just vendor the last watcher from sane (the node watcher - we already vendor both the watchman watcher and the fsevents one).

#10919

@SimenB SimenB closed this Dec 5, 2020
@SimenB SimenB deleted the replace-sane-with-chokidar branch December 5, 2020 13:27
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants