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.

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
merged 17 commits into from
Aug 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
116 changes: 116 additions & 0 deletions src/core/event-handle.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
import { Debug } from '../core/debug.js';

/**
* Event Handle that is created by {@link EventHandler} and can be used for easier event removal and management.
* @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.
* @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) {
Copy link
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 this event from its handler.
*/
off() {
if (this._removed) return;
this.handler.off(this.name, this.callback, this.scope);
}

on(name, callback, scope = this) {
Debug.deprecated('Using chaining with EventHandler.on is deprecated, subscribe to an event from EventHandler directly instead.');
return this.handler._addCallback(name, callback, scope, false);
}

once(name, callback, scope = this) {
Debug.deprecated('Using chaning with EventHandler.once is deprecated, subscribe to an event from EventHandler directly instead.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Debug.deprecated('Using chaning with EventHandler.once is deprecated, subscribe to an event from EventHandler directly instead.');
Debug.deprecated('Using chaining with EventHandler.once is deprecated, subscribe to an event from EventHandler directly instead.');

return this.handler._addCallback(name, callback, scope, true);
}

/**
* Mark if event has been removed.
* @type {boolean}
* @internal
*/
set removed(value) {
if (!value) return;
this._removed = true;
}

/**
* True if event has been removed.
* @type {boolean}
*/
get removed() {
return this._removed;
}
Comment on lines +107 to +113
Copy link
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?

Copy link
Collaborator

@kungfooman kungfooman Aug 3, 2023

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 };
72 changes: 45 additions & 27 deletions src/core/event-handler.js
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 @@ -29,13 +31,13 @@
*/
class EventHandler {
/**
* @type {Map<string,Array<object>>}
* @type {Map<string,Array<EventHandle>>}
* @private
*/
_callbacks = new Map();

/**
* @type {Map<string,Array<object>>}
* @type {Map<string,Array<EventHandle>>}
* @private
*/
_callbackActive = new Map();
Expand All @@ -58,11 +60,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 +81,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 +94,21 @@ 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} Can be used for removing event in the future.
* @example
* obj.on('test', function (a, b) {
* console.log(a + b);
* });
* obj.fire('test', 1, 2); // prints 3 to the console
* @example
* const evt = obj.on('test', function (a, b) {
* console.log(a + b);
* });
* // some time later
* evt.off();
*/
on(name, callback, scope = this) {
this._addCallback(name, callback, scope, false);
return this;
return this._addCallback(name, callback, scope, false);
}

/**
Expand All @@ -111,7 +119,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} - can be used for removing event in the future.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @returns {EventHandle} - can be used for removing event in the future.
* @returns {EventHandle} Can be used for removing event in the future.

* @example
* obj.once('test', function (a, b) {
* console.log(a + b);
Expand All @@ -120,8 +128,7 @@ 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;
return this._addCallback(name, callback, scope, true);
}

/**
Expand Down Expand Up @@ -165,33 +172,41 @@ class EventHandler {

if (!name) {
// remove all events
for (const callbacks of this._callbacks.values()) {
for (let i = 0; i < callbacks.length; i++) {
callbacks[i].removed = true;
}
}
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].removed = true;
}
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;

for (let i = 0; i < count; i++) {
for (let i = 0; i < callbacks.length; 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].removed = true;
callbacks.splice(i, 1);
i--;
}

events.length = count;

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

Expand Down Expand Up @@ -237,9 +252,11 @@ 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) {
if (evt._once) {
// check that callback still exists because user may have unsubscribed in the event handler
const existingCallback = this._callbacks.get(name);
const ind = existingCallback ? existingCallback.indexOf(evt) : -1;
Expand All @@ -250,6 +267,7 @@ class EventHandler {

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

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