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
Adds type definitions for RollupWatcher events. #3302
Conversation
The type information is extracted from https://github.com/rollup/rollup/blob/master/src/watch/index.ts, someone should double check that I got it all right. I'm new to rollup and have never actually used any of this stuff, but when I tried to I ran into the problem of there being no useful type information which caused me to have to read source code. Rather than having everyone suffer like I did, I decided to create a PR to resolve the issue.
I didn't realize that `on` was chainable, this should resolve the issue.
Codecov Report
@@ Coverage Diff @@
## master #3302 +/- ##
==========================================
+ Coverage 93.17% 93.22% +0.04%
==========================================
Files 172 172
Lines 6041 6038 -3
Branches 1801 1801
==========================================
Hits 5629 5629
+ Misses 221 218 -3
Partials 191 191
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 good to me, except about the FATAL event, which is not really your doing but would be nice to clean up with this PR.
Also your definition of the TypedEventEmitter
got me thinking if it could not be a way forward for fixing our dependency on the Node types (that are causing a lot of headaches for some) to just repeat the types we depend on in types.d.ts
. To my knowledge, this would only be Buffer
. But definitely something for a separate PR.
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.
Thanks
🚢 it |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
Updated version of #3075 by @MicahZoltu
Closes #3075