Skip to content

EventHandle and option to disable EventHandler chaining. #5481

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.

Sign up for GitHub

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

Merged
willeastcott merged 17 commits into from
Aug 8, 2023
Merged

EventHandle and option to disable EventHandler chaining. #5481

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
8e582b7
EventHandle and option to disable EventHandler chaining
Maksims Jul 14, 2023
fc10c4d
EventHandle.removed is readonly
Maksims Jul 14, 2023
e46456b
lint fixes and behvaiour consistency for eventhandle
Maksims Jul 14, 2023
fa4b665
EventHandle fix types definitions
Maksims Jul 14, 2023
438ac8b
merge
Maksims Jul 28, 2023
aceb84a
eventhandle some properties should be @ignore instead of @private
Maksims Jul 28, 2023
26705b7
eventhandle.destroy should be non-public
Maksims Jul 28, 2023
54d9b16
ensure we debug log when subscribing to events without arguments
Maksims Jul 28, 2023
c36cd0a
it is possible to subscribe without callback, and we need to avoid cr…
Maksims Jul 28, 2023
cb932ae
lint update
Maksims Aug 1, 2023
587f1a2
Do not use chaining for EventHandler
Maksims Aug 3, 2023
b0d4eb8
Add on and once methods to EventHandle for backwards compatibility wi…
Maksims Aug 3, 2023
e03357f
Update src/core/event-handle.js
Maksims Aug 4, 2023
3a614e6
Update src/core/event-handle.js
Maksims Aug 4, 2023
a85aff9
Update src/core/event-handler.js
Maksims Aug 4, 2023
e2c62a5
minor jsdoc corrections
Maksims Aug 7, 2023
9cb72cb
remove EventHandle.destroy method
Maksims Aug 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 103 additions & 0 deletions src/core/event-handle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
/**
* Event Handle that is created by {@link EventHandler} and can be used for easier removing and events management.
Comment thread
Maksims marked this conversation as resolved.
Outdated
* @example
* const evt = obj.on('test', (a, b) => {
* console.log(a + b);
* });
* obj.fire('test');
*
* evt.off(); // easy way to remove this event
* obj.fire('test'); // this will not trigger an event
* @example
* // store an array of event handles
* let events = [ ];
*
* events.push(objA.on('testA', () => { }));
* events.push(objB.on('testB', () => { }));
*
* // when needed, remove all events
* events.forEach((evt) => {
* evt.off();
* });
* events = [ ];
*/
class EventHandle {
/**
* @type {import('./event-handler.js').EventHandler}
* @private
*/
handler;

/**
* @type {string}
* @private
*/
name;

/**
* @type {import('./event-handler.js').HandleEventCallback}
* @ignore
*/
callback;

/**
* @type {object}
* @ignore
*/
scope;

/**
* @type {boolean}
* @ignore
*/
once;

/**
* True if event has been removed.
* @type {boolean}
* @private
*/
_removed = false;

/**
* @param {import('./event-handler.js').EventHandler} handler - source object of the event.
Comment thread
Maksims marked this conversation as resolved.
* @param {string} name - Name of the event.
* @param {import('./event-handler.js').HandleEventCallback} callback - Function that is called when event is fired.
* @param {object} scope - Object that is used as `this` when event is fired.
* @param {boolean} [once] - If this is a single event and will be removed after event is fired.
*/
constructor(handler, name, callback, scope, once = false) {

@willeastcott willeastcott Aug 3, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we hide the constructor from the docs? Presumably end users would never construct the EventHandle themselves and just use instances returned from on and once?

this.handler = handler;
this.name = name;
this.callback = callback;
this.scope = scope;
this.once = once;
}

/**
* Remove references.
* @ignore
*/
destroy() {
if (this._removed) return;
this._removed = true;
}

@kungfooman kungfooman Jul 15, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Shouldn't a destroyed handle off itself at least?

@Maksims Maksims Jul 15, 2023

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

destroy is a private method of EventHandle, and cleans its data, and does not modify data outside of its class.

@kungfooman kungfooman Jul 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Every other destroy method we have cleans up its events in other objects, usually in the form of:

    destroy() {
        // remove any outstanding listeners
        this._registry.off("load", this._onLoad);
        this._registry.off("error", this._onError);
        // ...
    }

Why is this a special case?

It also doesn't really do anything at all right now, is it a draft?

@Maksims Maksims Jul 17, 2023

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Every other destroy method we have cleans up its events in other objects, usually in the form of:

    destroy() {
        // remove any outstanding listeners
        this._registry.off("load", this._onLoad);
        this._registry.off("error", this._onError);
        // ...
    }

Why is this a special case?

It also doesn't really do anything at all right now, is it a draft?

Events are "detached" rather than "destroyed". This PR does not change that and follows an existing pattern.
I've added clear references to destroy, like this.callback = null and other things, but tests did not like that. As the current implementation does not clear such references from event objects, their properties are accessed after during tests. So such change would have the potential to break.


/**
* Remove this event from its handler.
*/
off() {
if (this._removed) return;
this.handler.off(this.name, this.callback, this.scope);
}

/**
* True if event has been removed.
* @type {boolean}
*/
get removed() {
return this._removed;
}
Comment on lines +107 to +113

@kungfooman kungfooman Jul 30, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we really need all this extra getter code which kinda does nothing? Why not simple and short:

    /**
     * True if event has been removed.
     * @type {boolean}
     * @readonly
     */
    removed = false;

If I get your intention right, you just want to have it readable and not writable?

@kungfooman kungfooman Aug 3, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sorry, I just realized we still lack proper types to express this idiom: microsoft/TypeScript#37487

My favorite would be this, which is a pattern all over the engine:

~~

    destroy() {
        this.handler?.off(this.name, this.callback, this.scope);
        this.handler = null;
    }

~~

So we don't introduce removed/_removed at all.

Edit: outdated / do-not-use / etc.

}

export { EventHandle };
68 changes: 47 additions & 21 deletions src/core/event-handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { EventHandle } from './event-handle.js';

/**
* Callback used by {@link EventHandler} functions. Note the callback is limited to 8 arguments.
*
Expand Down Expand Up @@ -28,6 +30,13 @@
* ```
*/
class EventHandler {
/**
* When set to true, `on` and `once` methods will return `this`.
* When set to false, `on` and `once` will return `EventHandle` for easier events management.
* @type {boolean}
*/
static chaining = true;

/**
* @type {Map<string,Array<object>>}
* @private
Expand Down Expand Up @@ -58,11 +67,14 @@ class EventHandler {
* @param {object} scope - Object to use as 'this' when the event is fired, defaults to
* current this.
* @param {boolean} once - If true, the callback will be unbound after being fired once.
* @returns {EventHandle} Created {@link EventHandle}.
* @ignore
*/
_addCallback(name, callback, scope, once) {
// #if _DEBUG
if (!name || typeof name !== 'string' || !callback)
return;
console.warn(`EventHandler: subscribing to an event (${name}) with missing arguments`, callback);
// #endif

if (!this._callbacks.has(name))
this._callbacks.set(name, []);
Expand All @@ -76,11 +88,9 @@ class EventHandler {
}
}

this._callbacks.get(name).push({
callback: callback,
scope: scope,
once: once
});
const evt = new EventHandle(this, name, callback, scope, once);
this._callbacks.get(name).push(evt);
return evt;
}

/**
Expand All @@ -91,16 +101,16 @@ class EventHandler {
* the callback is limited to 8 arguments.
* @param {object} [scope] - Object to use as 'this' when the event is fired, defaults to
* current this.
* @returns {EventHandler} Self for chaining.
* @returns {EventHandle|EventHandler} {@link EventHandle} if `EventHandler.chaining` set to `false`, otherwise returns `this` (self) for chaining.
* @example
* obj.on('test', function (a, b) {
* console.log(a + b);
* });
* obj.fire('test', 1, 2); // prints 3 to the console
*/
on(name, callback, scope = this) {
this._addCallback(name, callback, scope, false);
return this;
const evt = this._addCallback(name, callback, scope, false);
return EventHandler.chaining ? this : evt;
}

/**
Expand All @@ -111,7 +121,7 @@ class EventHandler {
* the callback is limited to 8 arguments.
* @param {object} [scope] - Object to use as 'this' when the event is fired, defaults to
* current this.
* @returns {EventHandler} Self for chaining.
* @returns {EventHandle|EventHandler} {@link EventHandle} if `EventHandler.chaining` set to `false`, otherwise returns `this` (self) for chaining.
* @example
* obj.once('test', function (a, b) {
* console.log(a + b);
Expand All @@ -120,8 +130,8 @@ class EventHandler {
* obj.fire('test', 1, 2); // not going to get handled
*/
once(name, callback, scope = this) {
this._addCallback(name, callback, scope, true);
return this;
const evt = this._addCallback(name, callback, scope, true);
return EventHandler.chaining ? this : evt;
}

/**
Expand Down Expand Up @@ -165,33 +175,46 @@ class EventHandler {

if (!name) {
// remove all events
for (const callbacks of this._callbacks.values()) {
for (let i = 0; i < callbacks.length; i++) {
callbacks[i].destroy();
}
}
this._callbacks.clear();
} else if (!callback) {
// remove all events of a specific name
if (this._callbacks.has(name))
const callbacks = this._callbacks.get(name);
if (callbacks) {
for (let i = 0; i < callbacks.length; i++) {
callbacks[i].destroy();
}
this._callbacks.delete(name);
}
} else {
const events = this._callbacks.get(name);
if (!events)
const callbacks = this._callbacks.get(name);
if (!callbacks)
return this;

let count = events.length;
let count = callbacks.length;

for (let i = 0; i < count; i++) {
// remove all events with a specific name and a callback
if (events[i].callback !== callback)
if (callbacks[i].callback !== callback)
continue;

// could be a specific scope as well
if (scope && events[i].scope !== scope)
if (scope && callbacks[i].scope !== scope)
continue;

events[i--] = events[--count];
callbacks[i].destroy();
// potential issue with such way of removing items from an array,
// as it changes the order of event handlers
callbacks[i--] = callbacks[--count];
Comment thread
Maksims marked this conversation as resolved.
Outdated
}

events.length = count;
callbacks.length = count;

if (events.length === 0)
if (callbacks.length === 0)
this._callbacks.delete(name);
}

Expand Down Expand Up @@ -237,6 +260,8 @@ class EventHandler {
// eslint-disable-next-line no-unmodified-loop-condition
for (let i = 0; (callbacks || this._callbackActive.get(name)) && (i < (callbacks || this._callbackActive.get(name)).length); i++) {
const evt = (callbacks || this._callbackActive.get(name))[i];
if (!evt.callback) continue;

evt.callback.call(evt.scope, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8);

if (evt.once) {
Expand All @@ -250,6 +275,7 @@ class EventHandler {

const callbacks = this._callbacks.get(name);
if (!callbacks) continue;
callbacks[ind].destroy();
callbacks.splice(ind, 1);

if (callbacks.length === 0)
Expand Down