fix(plugin-compat): update fsevents patch to support virtual path #1671
Conversation
missing1984
changed the title
| + let pnpApi; | ||
| + try { | ||
| + if (process.versions.pnp) { | ||
| + pnpApi = require(`pnpapi`); |
There was a problem hiding this comment.
@arcanis I am assuming calling require(pnpapi) many times won't have performance implication
| @@ -2,20 +2,21 @@ diff --git a/fsevents.js b/fsevents.js | |||
| semver exclusivity ^1 | |||
There was a problem hiding this comment.
the API fsevents 1.x exposed makes it quite challenging to watch multiple files so I only watch the path file instead.( same behavior as current patch)
|
I really want to look but the diff is difficult to read due to the GH interface not working super well for diffs 😞 I'll try to improve a bit our patch infra tomorrow to store the changes under JS syntax, and reapply your diff on top of it. |
|
I think calling the watcher multiple times is problematic, as it will open multiple watchers on the same location. Since we already have reports that webpack sometimes open too many file descriptors, better be conservative here! I also would prefer to memoize as much work as possible, and to simultaneously avoid the I've made a refactoring to instead compute an alias map . The advantage is that this map is shared between all calls, and it doesn't require much changes inside the fsevents files themselves. There's still room for perf improvements (I'm currently doing a crawl on all the options, which isn't ideal - a binary tree would be more efficient), but that should work without too much code. Can you check whether it works for you? I've added a test for both FSEvents 1 & 2, but better be sure 😄 |
arcanis
force-pushed
the
mluo/fix-fsevents-virtual-path
branch
from
a22fb54
to
4de6b1c
Compare
merceyz
changed the title
What's the problem this PR addresses?
Patch fsevents 2.x so it works with pnp virtual folders.
Close #1209
...
How did you fix it?
Find virtual paths for a given watch root and setup sub watchers accordingly.
...
Checklist