Improved watcher hooks

[Amareis]

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers:
Closes #3821

Description

1. Implemented closeWatcher hook
2. Added getWatchFiles method to PluginContext
3. Extended watchChange hook to get isDeleted parameter
4. Also added this: PluginContext for watchChange

[codecov]

Codecov Report

Merging #3841 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3841   +/-   ##
=======================================
  Coverage   97.06%   97.06%           
=======================================
  Files         184      184           
  Lines        6503     6518   +15     
  Branches     1884     1887    +3     
=======================================
+ Hits         6312     6327   +15     
  Misses        101      101           
  Partials       90       90           

Impacted Files	Coverage Δ
src/utils/PluginDriver.ts	100.00% <ø> (ø)
src/Graph.ts	100.00% <100.00%> (ø)
src/utils/PluginContext.ts	100.00% <100.00%> (ø)
src/watch/fileWatcher.ts	96.77% <100.00%> (+0.34%)	⬆️
src/watch/watch.ts	99.09% <100.00%> (+0.06%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c57aede...4e6946e. Read the comment docs.

[lukastaegert]

Looks really good already, just some comments with regard to handling add vs change and delete and how to handle multiple events in a single cycle

[Amareis]

Hmm, additional miss line is for scenario where file added and immediatly deleted. Good point, codecov!

Pretty strange what "buggy" scenario is covered by tests, hmm...

[lukastaegert]

Looks great! Only some minor comments left that you may want to have a look at.

[Amareis]

All done!

[lukastaegert]

Just noticed there are issues on Windows and MacOS. Will need to take a look before this can be released.

[Amareis]

I think, it's needed to improve tests. Currently timeout-based approach is really non determenistic - failed build prove it.

[Amareis]

Can we turn buildDelay to accept a function, which managed when re-run build? I think it both make tests determenistic and can be useful for scenarios where watcher is runned by external toold.

{
  watcher: {
    buildDelay: (build: () => void): Disposer => {
      let t = setTimeout(build, 1000)
      return () => clearTimeout(t)
    }
  }
}

[lukastaegert]

That is by itself an interesting idea. Ideally, plugins should be able to hold off a rebuild until they say: Ok, now I am done, please rebuild.

But the error I am seeing is not from non-determinism, it is very deterministically broken for me, and I do not understand the error yet.

[lukastaegert]

In short, it looks like the test runner is telling me that "done" was called multiple times, which is particularly odd as we are not using a "done" function.

[Amareis]

Uh-oh, cannot help, 'cause don't have nor win nor mac. But I'll think about plugins build delay.

[lukastaegert]

Maybe if we allow plugins to return a Promise from the watchChange hook to block rebuilds?

[lukastaegert]

So in the end, I did not really understand the issue with the test runner, but it only seemed to occur when there was an actual error, so I rather fixed the issues. So what I found:

• Creating a file on Mac and immediately afterwards starting a watcher can trigger an "update" event for that file on that watcher. Waiting a few ms between creating and starting the watcher seems to help
• On Windows, deleting a file and recreating it causes an additional update event to be created. BEFORE the actual create event. This might really deserve some more looking into it, but for now, I decided to reorder the events in the test. I hate this.

[Amareis]

Maybe if we allow plugins to return a Promise from the watchChange hook to block rebuilds?

Good idea! I'll make PR soon, also fix #3789 and buildDelay