-
-
Notifications
You must be signed in to change notification settings - Fork 194
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
#700@minor: Add event listener options. #763
#700@minor: Add event listener options. #763
Conversation
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.
Great contribution! 🙂 Just found some minor things.
@@ -723,7 +723,7 @@ export default class Document extends Node implements IDocument { | |||
this._isFirstWriteAfterOpen = true; | |||
|
|||
for (const eventType of Object.keys(this._listeners)) { | |||
const listeners = this._listeners[eventType]; | |||
const listeners = this._listeners[eventType].map((listener) => listener.fn); |
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.
I think we can speed this up a bit by checking directly in the for loop instead as we can avoid the extra loop with the map()
.
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.
Yes, you are right. 👍
@@ -32,7 +41,8 @@ export default abstract class EventTarget implements IEventTarget { | |||
listener: ((event: Event) => void) | IEventListener | |||
): void { | |||
if (this._listeners[type]) { | |||
const index = this._listeners[type].indexOf(listener); | |||
const _listeners = this._listeners[type].map((l) => l.fn); |
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.
For loops are faster than map(), so I think we are better of with a for loop here, but we could improve performance even further if we store options and listeners in separate objects.
Example:
public readonly _options: {
[k: string]: { once: boolean }[];
} = {};
public readonly _listeners: {
[k: string]: (((event: Event) => void) | IEventListener)[];
} = {};
public addEventListener(
type: string,
listener: ((event: Event) => void) | IEventListener,
options?: { once: boolean }
): void {
this._listeners[type] = this._listeners[type] || [];
this._options[type] = this._options[type] || [];
this._listeners[type].push(listener);
this._options[type].push(options || null);
}
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.
Great.
…and options in separate objects.
@btea I made a commit with the proposed adjustments for improved performance. Please have a look if you want 🙂 |
It looks very good. 👍 |
rel #700