-
Notifications
You must be signed in to change notification settings - Fork 142
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
🐛 Use EventTarget.prototype.addEventListener instead of the method #3137
🐛 Use EventTarget.prototype.addEventListener instead of the method #3137
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3137 +/- ##
=======================================
Coverage 93.44% 93.44%
=======================================
Files 279 280 +1
Lines 7683 7679 -4
Branches 1718 1719 +1
=======================================
- Hits 7179 7176 -3
+ Misses 504 503 -1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
const addEventListenerSpy = jasmine.createSpy() | ||
const removeEventListenerSpy = jasmine.createSpy() | ||
|
||
// The stopLeakDetection function of the global after each hook will reset the EventTarget.prototype.addEventListener |
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.
The addEventListener override conflicts with the leakDetection. The best way I found is to let the stopLeakDetection remove the override
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.
Okay I found the root cause. Once this fix is merged I'll cleanup the tests properly
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
type DeflateWorkerListener = (event: { data: DeflateWorkerResponse }) => void | ||
|
||
export class MockWorker implements DeflateWorker { | ||
export class MockWorker extends MockEventTarget implements DeflateWorker { |
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.
❓ question: appart from factorization (which is great!), is there another reason why we needed this change?
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.
No just factorization.
Motivation
Use the window.EventTarget.prototype when possible to avoid wrong overrides (e.g: salesforce/lwc#1824)
Changes
browser-sdk/packages/core/src/browser/addEventListener.ts
Lines 131 to 132 in 91bace6
Testing
I have gone over the contributing documentation.