feat: replace sane with chokidar#10048
Conversation
SimenB
requested review from
cpojer,
jeysal,
rubennorte,
scotthovestadt and
thymikee
| glob: patterns, | ||
| ignored: ignorePattern, | ||
| }) | ||
| : chokidarWatch(patterns, { |
There was a problem hiding this comment.
Surprising but .. they expose almost the same API? :O
There was a problem hiding this comment.
I dunno if the ignore pattern works, should probably verify
| (watcher as ChokidarFsWatcher).on('all', (type, filePath, stat) => { | ||
| onChange(type, filePath, root, stat); | ||
| }); |
There was a problem hiding this comment.
this is the part that makes me think creating some abstraction makes sense. There might be other things we want to tweak as well
cpojer
left a comment
cpojer
left a comment
There was a problem hiding this comment.
Goodbye sane, and goodbye @cnakazawa/watch. Nice work, I'm excited about this change as it took years :D
|
Hmm, #5387 imports some stuff from
We should vendor it, I guess |
SimenB
force-pushed
the
replace-sane-with-chokidar
branch
2 times, most recently
from
38a3a33 to
350f151
Compare
|
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? |
|
@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 |
SimenB
force-pushed
the
replace-sane-with-chokidar
branch
from
350f151 to
2535515
Compare
|
@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. |
|
Yeah, that's essentially it - make sure you're disabling 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). |
|
Given amasad/sane#159 (upstream dependency execa has critical vuln) and that sane is basically unmaintained, this PR would be really helpful! |
|
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
force-pushed
the
replace-sane-with-chokidar
branch
from
2535515 to
80cb8e1
Compare
SimenB
force-pushed
the
replace-sane-with-chokidar
branch
from
80cb8e1 to
22226cd
Compare
SimenB
force-pushed
the
replace-sane-with-chokidar
branch
from
22226cd to
6282290
Compare
SimenB
force-pushed
the
replace-sane-with-chokidar
branch
from
6282290 to
4587e53
Compare
|
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 In addition, on change it emits change events for So instead of spending time trying to figure out how to make |
|
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. |
5 participants
Summary
sanepulls in old version ofmicromatch. We're also already vendoring theirwatchmanwatcher (#5387) and thefseventswatcher they removed (#8258). Replacing withchokidarallows us to drop that last vendoring aschokidarabstracts over it.We might need to create a
chokidarabstraction 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.