Skip to content
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

Merged
merged 7 commits into from Jan 8, 2020
Merged

Adds type definitions for RollupWatcher events. #3302

merged 7 commits into from Jan 8, 2020

Conversation

NotWoods
Copy link
Member

@NotWoods NotWoods commented Dec 24, 2019

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:

Description

Updated version of #3075 by @MicahZoltu
Closes #3075

The type information is extracted from /src/watch/index.ts@master , 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.

MicahZoltu and others added 5 commits August 22, 2019 19:06
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
Copy link

codecov bot commented Dec 24, 2019

Codecov Report

Merging #3302 into master will increase coverage by 0.04%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/rollup/index.ts 96.96% <ø> (ø) ⬆️
src/utils/PluginContext.ts 100% <100%> (ø) ⬆️
src/utils/deconflictChunk.ts 100% <100%> (ø) ⬆️
src/watch/index.ts 85.32% <100%> (ø) ⬆️
cli/run/watch.ts 43.58% <66.66%> (+1.61%) ⬆️

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 b99758b...04b56a8. Read the comment docs.

Copy link
Member

@lukastaegert lukastaegert left a 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.

src/rollup/types.d.ts Outdated Show resolved Hide resolved
Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@shellscape
Copy link
Contributor

🚢 it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants