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

Jump to bottom

remove events/EventEmitter && move Observer to ts #2097

Merged
merged 2 commits into from Feb 11, 2019
Merged

remove events/EventEmitter && move Observer to ts #2097

merged 2 commits into from Feb 11, 2019

Conversation

mad-gooze
Copy link
Contributor

This PR will...

Remove usage of EventEmitter from 'events' package in favor on EventEmitter3. It it already used in the project and claims to be a faster alternative of EventEmitter. I also moved Observer class to typescript.

Why is this Pull Request needed?

Remove code duplication, decrease bundle size (-2.9kb).

Are there any points in the code the reviewer needs to double check?

no?

Resolves issues:

Checklist

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • API or design changes are documented in API.md

@mad-gooze mad-gooze changed the title remove events EventEmitter; move Observer to ts remove events/EventEmitter; move Observer to ts Jan 25, 2019
@mad-gooze mad-gooze changed the title remove events/EventEmitter; move Observer to ts remove events/EventEmitter && move Observer to ts Jan 25, 2019
@mad-gooze
Copy link
Contributor Author

This also removes annoying 'possible EventEmitter memory leak detected' warning.

michaelcunningham19
michaelcunningham19 previously approved these changes Jan 25, 2019
View reviewed changes
Copy link
Member

@michaelcunningham19 michaelcunningham19 left a comment

Choose a reason for hiding this comment

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

Nice work! 👍

Quick question:

This also removes annoying 'possible EventEmitter memory leak detected' warning.

Where were you seeing this warning?

itsjamie
itsjamie reviewed Jan 25, 2019
View reviewed changes
src/observer.ts
* in every call to a handler, which is the purpose of our `trigger` method
* extending the standard API.
*/
trigger (event: string, ...data: Array<any>): void {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Something to consider for the future is if we made this generic so it took T as the data parameter so usage could look like:

trigger<T>(event: string, data: T): void { }

This could flow into emit, and then on the listen and the whole thing could have typed listeners.

Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM.

@mad-gooze
Copy link
Contributor Author

Where were you seeing this warning?

This is logged by EventEmitter if you attach more than maxListeners handlers:
https://github.com/Gozala/events/blob/master/events.js#L206

johnBartos
johnBartos reviewed Jan 30, 2019
View reviewed changes
Copy link
Collaborator

@johnBartos johnBartos left a comment

Choose a reason for hiding this comment

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

Is there a lint rule we can add to disable importing native modules?

@johnBartos
Copy link
Collaborator

@mad-gooze Can resolve the merge conflicts please?

@mad-gooze
Copy link
Contributor Author

Is there a lint rule we can add to disable importing native modules?

native modules can just be disabled in webpack so an error wil be thrown if you try to import then
https://webpack.js.org/configuration/node

@michaelcunningham19
Copy link
Member

native modules can just be disabled in webpack so an error wil be thrown if you try to import then
https://webpack.js.org/configuration/node

@mad-gooze Nice find, I like that 👍

I'm good either way when it comes to adding this webpack config tweak in this PR or in a separate PR

@mad-gooze
Copy link
Contributor Author

mad-gooze commented Feb 2, 2019 •
edited

@michaelcunningham19 disabled native node modules enabled by default 9fc1b75

michaelcunningham19
michaelcunningham19 approved these changes Feb 5, 2019
View reviewed changes
Copy link
Member

@michaelcunningham19 michaelcunningham19 left a comment

Choose a reason for hiding this comment

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

LGTM

@johnBartos johnBartos merged commit 48f64de into video-dev:master Feb 11, 2019
@mad-gooze mad-gooze deleted the remove-events branch February 11, 2019 15:47
dyue added a commit to dyue/hls.js that referenced this pull request Mar 9, 2019
@dyue
Merge remote-tracking branch 'origin/master' into master-dev
cb9d289 
* origin/master: (59 commits)
  Change networkDetails type to "unknown".
  mp4-remuxer / remuxAudio() : recompute mdatSize / dont rely on audioTrack.len (video-dev#2096)
  Typo fix for destory -> destroy.
  Change responseType to string
  Callbacks are defined, removing TODO.
  video-devGH-2082: Replacing usage of Array.prototype.find/findIndex (video-dev#2117)
  Reduce GC pressure from ID3._utf8ArrayToStr (video-dev#2035)
  (misc): converting binary search to typescript (video-dev#2129)
  (misc) Removing IE8-specific (dead) vtt/cue code
  Fixed case of 'They use hls.js in production!' header and updated Viacom image link to new domain and using https (video-dev#2127)
  remove events/EventEmitter && move Observer to ts (video-dev#2097)
  Basic typescript of event handler.
  Update webpack to use babel loader with support for TS. (video-dev#2119)
  Allow console statement in test bench and saucelabs code.
  Lint fixes
  Playlist Loader conversion to TypeScript.
  Playlist-loader rename.
  Change IV handling for initSegments to just log a warning.
  Add handling for AES-128 encrypted initialization segments needing IV.
  (misc) Adding ace editor to demo to enable advanced customization (video-dev#2077)
  ...
@johnBartos johnBartos added this to In progress in Typescript Integration via automation Aug 13, 2019
@johnBartos johnBartos added this to the 0.13.0 milestone Aug 13, 2019
@itsjamie itsjamie moved this from In progress to Reviewer approved in Typescript Integration Aug 28, 2019
@itsjamie itsjamie moved this from Reviewer approved to Done in Typescript Integration Aug 28, 2019
@robwalch robwalch added this to Done in 0.13.0 Nov 6, 2019
@snyk-bot snyk-bot mentioned this pull request Mar 8, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itsjamie itsjamie itsjamie left review comments

@johnBartos johnBartos johnBartos left review comments

@michaelcunningham19 michaelcunningham19 michaelcunningham19 approved these changes

@tchakabam tchakabam Awaiting requested review from tchakabam

@tjenkinson tjenkinson Awaiting requested review from tjenkinson

Assignees
No one assigned
Labels
Enhancement
Projects
No open projects
0.13.0
  
Done
Typescript Integration
  
Done
Typescript Integration
Archived in project
Milestone
0.13.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mad-gooze @johnBartos @michaelcunningham19 @itsjamie