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
Improved watcher hooks #3841
Improved watcher hooks #3841
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Only some minor comments left that you may want to have a look at.
All done! |
Just noticed there are issues on Windows and MacOS. Will need to take a look before this can be released. |
I think, it's needed to improve tests. Currently timeout-based approach is really non determenistic - failed build prove it. |
Can we turn {
watcher: {
buildDelay: (build: () => void): Disposer => {
let t = setTimeout(build, 1000)
return () => clearTimeout(t)
}
}
} |
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. |
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. |
Uh-oh, cannot help, 'cause don't have nor win nor mac. But I'll think about plugins build delay. |
Maybe if we allow plugins to return a Promise from the watchChange hook to block rebuilds? |
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:
|
Good idea! I'll make PR soon, also fix #3789 and buildDelay |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Closes #3821
Description
closeWatcher
hookgetWatchFiles
method toPluginContext
watchChange
hook to getisDeleted
parameterthis: PluginContext
forwatchChange