From 0c4ce2e09c6d5d74478d9d0ef97b2845386723fa Mon Sep 17 00:00:00 2001 From: Arkadiusz Filipczak Date: Thu, 21 Jul 2022 15:33:06 +0200 Subject: [PATCH 1/6] Changed mixin style: EmitterMixin --- .../ckeditor5-utils/src/dom/emittermixin.ts | 26 +- packages/ckeditor5-utils/src/emittermixin.ts | 531 +++++++++--------- packages/ckeditor5-utils/src/mix.ts | 6 +- .../ckeditor5-utils/src/observablemixin.ts | 16 +- .../tests/_utils-tests/utils.js | 5 +- .../ckeditor5-utils/tests/_utils/utils.js | 4 +- .../ckeditor5-utils/tests/dom/emittermixin.js | 4 +- .../ckeditor5-utils/tests/emittermixin.js | 25 +- .../ckeditor5-utils/tests/keystrokehandler.js | 4 +- .../ckeditor5-utils/tests/observablemixin.js | 4 +- 10 files changed, 340 insertions(+), 285 deletions(-) diff --git a/packages/ckeditor5-utils/src/dom/emittermixin.ts b/packages/ckeditor5-utils/src/dom/emittermixin.ts index 384f77111a6..970aaf37392 100644 --- a/packages/ckeditor5-utils/src/dom/emittermixin.ts +++ b/packages/ckeditor5-utils/src/dom/emittermixin.ts @@ -11,7 +11,7 @@ import { default as EmitterMixin, _getEmitterListenedTo, _setEmitterId, - type Emitter as BaseEmitter, + Emitter as BaseEmitter, type CallbackOptions, type BaseEvent, type GetCallback @@ -45,7 +45,7 @@ import mix from '../mix'; * @mixes module:utils/emittermixin~EmitterMixin * @implements module:utils/dom/emittermixin~Emitter */ -const DomEmitterMixin: Emitter = extend( {}, EmitterMixin, { +const DomEmitterMixin: Emitter = extend( {}, EmitterMixin.mixinMethods, { /** * Registers a callback function to be executed when an event is fired in a specific Emitter or DOM Node. * It is backwards compatible with {@link module:utils/emittermixin~EmitterMixin#listenTo}. @@ -80,7 +80,7 @@ const DomEmitterMixin: Emitter = extend( {}, EmitterMixin, { this.listenTo( proxyEmitter, event, callback, options ); } else { // Execute parent class method with Emitter (or ProxyEmitter) instance. - EmitterMixin.listenTo.call( this, emitter, event, callback, options ); + BaseEmitter.prototype.listenTo.call( this, emitter, event, callback, options ); } }, @@ -114,7 +114,7 @@ const DomEmitterMixin: Emitter = extend( {}, EmitterMixin, { } } else { // Execute parent class method with Emitter (or ProxyEmitter) instance. - EmitterMixin.stopListening.call( this, emitter, event, callback ); + BaseEmitter.prototype.stopListening.call( this, emitter, event, callback ); } }, @@ -151,9 +151,19 @@ const DomEmitterMixin: Emitter = extend( {}, EmitterMixin, { { capture: true, passive: false }, { capture: true, passive: true } ].map( options => this._getProxyEmitter( node, options ) ).filter( proxy => !!proxy ) as any; - } + }, + + on: BaseEmitter.prototype.on, + once: BaseEmitter.prototype.once, + fire: BaseEmitter.prototype.fire, + off: BaseEmitter.prototype.off, + delegate: BaseEmitter.prototype.delegate, + stopDelegating: BaseEmitter.prototype.stopDelegating } ); +( DomEmitterMixin as any )._addEventListener = ( BaseEmitter.prototype as any )._addEventListener; +( DomEmitterMixin as any )._removeEventListener = ( BaseEmitter.prototype as any )._removeEventListener; + export default DomEmitterMixin; /** @@ -276,7 +286,7 @@ class ProxyEmitter { // are awaiting given event, detach native DOM listener from DOM Node. // See: {@link attach}. - if ( this._domListeners![ event ] && ( !( events = this._events![ event ] ) || !events.callbacks.length ) ) { + if ( this._domListeners![ event ] && ( !( events = ( this as any )._events![ event ] ) || !events.callbacks.length ) ) { this._domListeners![ event ].removeListener(); } } @@ -299,7 +309,7 @@ class ProxyEmitter { options: CallbackOptions ): void { this.attach( event ); - EmitterMixin._addEventListener.call( this, event, callback, options ); + ( BaseEmitter.prototype as any )._addEventListener.call( this, event, callback, options ); } /** @@ -311,7 +321,7 @@ class ProxyEmitter { * @param {Function} callback The function to stop being called. */ public _removeEventListener( event: string, callback: Function ) { - EmitterMixin._removeEventListener.call( this, event, callback ); + ( BaseEmitter.prototype as any )._removeEventListener.call( this, event, callback ); this.detach( event ); } diff --git a/packages/ckeditor5-utils/src/emittermixin.ts b/packages/ckeditor5-utils/src/emittermixin.ts index 0f65c0e9c5e..f8b99b8e76e 100644 --- a/packages/ckeditor5-utils/src/emittermixin.ts +++ b/packages/ckeditor5-utils/src/emittermixin.ts @@ -7,6 +7,8 @@ * @module utils/emittermixin */ +/* eslint-disable new-cap */ + import EventInfo from './eventinfo'; import uid from './uid'; import priorities, { type PriorityString } from './priorities'; @@ -31,300 +33,314 @@ const _delegations = Symbol( 'delegations' ); * @mixin EmitterMixin * @implements module:utils/emittermixin~Emitter */ -const EmitterMixin: Emitter = { - /** - * @inheritDoc - */ - on( event, callback, options = {} ) { - this.listenTo( this, event, callback, options ); - }, - - /** - * @inheritDoc - */ - once( event, callback, options ) { - let wasFired = false; +export default function EmitterMixin object>( + base: Base +): { + new( ...args: ConstructorParameters ): InstanceType & Emitter; + prototype: InstanceType & Emitter; +} { + class Mixin extends base implements EmitterInternal { + public on( + event: TEvent[ 'name' ], + callback: GetCallback, + options?: CallbackOptions + ): void { + this.listenTo( this, event, callback, options ); + } - const onceCallback: typeof callback = ( event, ...args ) => { - // Ensure the callback is called only once even if the callback itself leads to re-firing the event - // (which would call the callback again). - if ( !wasFired ) { - wasFired = true; + public once( + event: TEvent[ 'name' ], + callback: GetCallback, + options?: CallbackOptions + ): void { + let wasFired = false; - // Go off() at the first call. - event.off(); + const onceCallback: typeof callback = ( event, ...args ) => { + // Ensure the callback is called only once even if the callback itself leads to re-firing the event + // (which would call the callback again). + if ( !wasFired ) { + wasFired = true; - // Go with the original callback. - callback.call( this, event, ...args ); - } - }; + // Go off() at the first call. + event.off(); - // Make a similar on() call, simply replacing the callback. - this.listenTo( this, event, onceCallback, options ); - }, + // Go with the original callback. + callback.call( this, event, ...args ); + } + }; - /** - * @inheritDoc - */ - off( event, callback ) { - this.stopListening( this, event, callback ); - }, + // Make a similar on() call, simply replacing the callback. + this.listenTo( this, event, onceCallback, options ); + } - /** - * @inheritDoc - */ - listenTo( emitter, event, callback, options = {} ) { - let emitterInfo, eventCallbacks; - - // _listeningTo contains a list of emitters that this object is listening to. - // This list has the following format: - // - // _listeningTo: { - // emitterId: { - // emitter: emitter, - // callbacks: { - // event1: [ callback1, callback2, ... ] - // .... - // } - // }, - // ... - // } - - if ( !this[ _listeningTo ] ) { - this[ _listeningTo ] = {}; + public off( event: string, callback: Function ): void { + this.stopListening( this, event, callback ); } - const emitters = this[ _listeningTo ]!; + public listenTo( + emitter: Emitter, + event: TEvent[ 'name' ], + callback: GetCallback, + options: CallbackOptions = {} + ): void { + let emitterInfo, eventCallbacks; + + // _listeningTo contains a list of emitters that this object is listening to. + // This list has the following format: + // + // _listeningTo: { + // emitterId: { + // emitter: emitter, + // callbacks: { + // event1: [ callback1, callback2, ... ] + // .... + // } + // }, + // ... + // } + + if ( !this[ _listeningTo ] ) { + this[ _listeningTo ] = {}; + } - if ( !_getEmitterId( emitter ) ) { - _setEmitterId( emitter ); - } + const emitters = this[ _listeningTo ]!; - const emitterId = _getEmitterId( emitter )!; + if ( !_getEmitterId( emitter ) ) { + _setEmitterId( emitter ); + } - if ( !( emitterInfo = emitters[ emitterId ] ) ) { - emitterInfo = emitters[ emitterId ] = { - emitter, - callbacks: {} - }; - } + const emitterId = _getEmitterId( emitter )!; - if ( !( eventCallbacks = emitterInfo.callbacks[ event ] ) ) { - eventCallbacks = emitterInfo.callbacks[ event ] = []; - } + if ( !( emitterInfo = emitters[ emitterId ] ) ) { + emitterInfo = emitters[ emitterId ] = { + emitter, + callbacks: {} + }; + } - eventCallbacks.push( callback ); + if ( !( eventCallbacks = emitterInfo.callbacks[ event ] ) ) { + eventCallbacks = emitterInfo.callbacks[ event ] = []; + } - // Finally register the callback to the event. - addEventListener( this, emitter, event, callback, options ); - }, + eventCallbacks.push( callback ); - /** - * @inheritDoc - */ - stopListening( emitter?: Emitter, event?: string, callback?: Function ) { - const emitters = this[ _listeningTo ]; - let emitterId = emitter && _getEmitterId( emitter ); - const emitterInfo = ( emitters && emitterId ) ? emitters[ emitterId ] : undefined; - const eventCallbacks = ( emitterInfo && event ) ? emitterInfo.callbacks[ event ] : undefined; - - // Stop if nothing has been listened. - if ( !emitters || ( emitter && !emitterInfo ) || ( event && !eventCallbacks ) ) { - return; + // Finally register the callback to the event. + addEventListener( this, emitter, event, callback, options ); } - // All params provided. off() that single callback. - if ( callback ) { - removeEventListener( this, emitter!, event!, callback ); + public stopListening( emitter?: Emitter, event?: string, callback?: Function ): void { + const emitters = this[ _listeningTo ]; + let emitterId = emitter && _getEmitterId( emitter ); + const emitterInfo = ( emitters && emitterId ) ? emitters[ emitterId ] : undefined; + const eventCallbacks = ( emitterInfo && event ) ? emitterInfo.callbacks[ event ] : undefined; - // We must remove callbacks as well in order to prevent memory leaks. - // See https://github.com/ckeditor/ckeditor5/pull/8480 - const index = eventCallbacks!.indexOf( callback ); - - if ( index !== -1 ) { - if ( eventCallbacks!.length === 1 ) { - delete emitterInfo!.callbacks[ event! ]; - } else { - removeEventListener( this, emitter!, event!, callback ); - } + // Stop if nothing has been listened. + if ( !emitters || ( emitter && !emitterInfo ) || ( event && !eventCallbacks ) ) { + return; } - } - // Only `emitter` and `event` provided. off() all callbacks for that event. - else if ( eventCallbacks ) { - while ( ( callback = eventCallbacks.pop() ) ) { + + // All params provided. off() that single callback. + if ( callback ) { removeEventListener( this, emitter!, event!, callback ); + + // We must remove callbacks as well in order to prevent memory leaks. + // See https://github.com/ckeditor/ckeditor5/pull/8480 + const index = eventCallbacks!.indexOf( callback ); + + if ( index !== -1 ) { + if ( eventCallbacks!.length === 1 ) { + delete emitterInfo!.callbacks[ event! ]; + } else { + removeEventListener( this, emitter!, event!, callback ); + } + } } + // Only `emitter` and `event` provided. off() all callbacks for that event. + else if ( eventCallbacks ) { + while ( ( callback = eventCallbacks.pop() ) ) { + removeEventListener( this, emitter!, event!, callback ); + } - delete emitterInfo!.callbacks[ event! ]; - } - // Only `emitter` provided. off() all events for that emitter. - else if ( emitterInfo ) { - for ( event in emitterInfo.callbacks ) { - this.stopListening( emitter!, event ); + delete emitterInfo!.callbacks[ event! ]; } - delete emitters[ emitterId! ]; - } - // No params provided. off() all emitters. - else { - for ( emitterId in emitters ) { - this.stopListening( emitters[ emitterId ].emitter ); + // Only `emitter` provided. off() all events for that emitter. + else if ( emitterInfo ) { + for ( event in emitterInfo.callbacks ) { + this.stopListening( emitter!, event ); + } + delete emitters[ emitterId! ]; + } + // No params provided. off() all emitters. + else { + for ( emitterId in emitters ) { + this.stopListening( emitters[ emitterId ].emitter ); + } + delete this[ _listeningTo ]; } - delete this[ _listeningTo ]; } - }, - /** - * @inheritDoc - */ - fire( eventOrInfo, ...args ) { - try { - const eventInfo = eventOrInfo instanceof EventInfo ? eventOrInfo : new EventInfo( this, eventOrInfo ); - const event = eventInfo.name; - let callbacks = getCallbacksForEvent( this, event ); - - // Record that the event passed this emitter on its path. - eventInfo.path.push( this ); - - // Handle event listener callbacks first. - if ( callbacks ) { - // Arguments passed to each callback. - const callbackArgs = [ eventInfo, ...args ]; - - // Copying callbacks array is the easiest and most secure way of preventing infinite loops, when event callbacks - // are added while processing other callbacks. Previous solution involved adding counters (unique ids) but - // failed if callbacks were added to the queue before currently processed callback. - // If this proves to be too inefficient, another method is to change `.on()` so callbacks are stored if same - // event is currently processed. Then, `.fire()` at the end, would have to add all stored events. - callbacks = Array.from( callbacks ); + public fire( + eventOrInfo: GetNameOrEventInfo, + ...args: TEvent[ 'args' ] + ): GetEventInfo[ 'return' ] { + try { + const eventInfo = eventOrInfo instanceof EventInfo ? eventOrInfo : new EventInfo( this, eventOrInfo ); + const event = eventInfo.name; + let callbacks = getCallbacksForEvent( this, event ); + + // Record that the event passed this emitter on its path. + eventInfo.path.push( this ); + + // Handle event listener callbacks first. + if ( callbacks ) { + // Arguments passed to each callback. + const callbackArgs = [ eventInfo, ...args ]; + + // Copying callbacks array is the easiest and most secure way of preventing infinite loops, when event callbacks + // are added while processing other callbacks. Previous solution involved adding counters (unique ids) but + // failed if callbacks were added to the queue before currently processed callback. + // If this proves to be too inefficient, another method is to change `.on()` so callbacks are stored if same + // event is currently processed. Then, `.fire()` at the end, would have to add all stored events. + callbacks = Array.from( callbacks ); + + for ( let i = 0; i < callbacks.length; i++ ) { + callbacks[ i ].callback.apply( this, callbackArgs ); + + // Remove the callback from future requests if off() has been called. + if ( eventInfo.off.called ) { + // Remove the called mark for the next calls. + delete eventInfo.off.called; + + this._removeEventListener( event, callbacks[ i ].callback ); + } + + // Do not execute next callbacks if stop() was called. + if ( eventInfo.stop.called ) { + break; + } + } + } - for ( let i = 0; i < callbacks.length; i++ ) { - callbacks[ i ].callback.apply( this, callbackArgs ); + // Delegate event to other emitters if needed. + const delegations = this[ _delegations ]; - // Remove the callback from future requests if off() has been called. - if ( eventInfo.off.called ) { - // Remove the called mark for the next calls. - delete eventInfo.off.called; + if ( delegations ) { + const destinations = delegations.get( event ); + const passAllDestinations = delegations.get( '*' ); - this._removeEventListener( event, callbacks[ i ].callback ); + if ( destinations ) { + fireDelegatedEvents( destinations, eventInfo, args ); } - // Do not execute next callbacks if stop() was called. - if ( eventInfo.stop.called ) { - break; + if ( passAllDestinations ) { + fireDelegatedEvents( passAllDestinations, eventInfo, args ); } } - } - // Delegate event to other emitters if needed. - const delegations = this[ _delegations ]; + return eventInfo.return; + } catch ( err ) { + // @if CK_DEBUG // throw err; + /* istanbul ignore next */ + CKEditorError.rethrowUnexpectedError( err as Error, this ); + } + } - if ( delegations ) { - const destinations = delegations.get( event ); - const passAllDestinations = delegations.get( '*' ); + public delegate( ...events: string[] ): EmitterMixinDelegateChain { + return { + to: ( emitter, nameOrFunction ) => { + if ( !this[ _delegations ] ) { + this[ _delegations ] = new Map(); + } - if ( destinations ) { - fireDelegatedEvents( destinations, eventInfo, args ); + // Originally there was a for..of loop which unfortunately caused an error in Babel that didn't allow + // build an application. See: https://github.com/ckeditor/ckeditor5-react/issues/40. + events.forEach( eventName => { + const destinations = this[ _delegations ]!.get( eventName ); + + if ( !destinations ) { + this[ _delegations ]!.set( eventName, new Map( [ [ emitter, nameOrFunction ] ] ) ); + } else { + destinations.set( emitter, nameOrFunction ); + } + } ); } + }; + } - if ( passAllDestinations ) { - fireDelegatedEvents( passAllDestinations, eventInfo, args ); - } + public stopDelegating( event?: string, emitter?: Emitter ): void { + if ( !this[ _delegations ] ) { + return; } - return eventInfo.return; - } catch ( err ) { - // @if CK_DEBUG // throw err; - /* istanbul ignore next */ - CKEditorError.rethrowUnexpectedError( err as Error, this ); - } - }, + if ( !event ) { + this[ _delegations ]!.clear(); + } else if ( !emitter ) { + this[ _delegations ]!.delete( event ); + } else { + const destinations = this[ _delegations ]!.get( event ); - /** - * @inheritDoc - */ - delegate( ...events ) { - return { - to: ( emitter, nameOrFunction ) => { - if ( !this[ _delegations ] ) { - this[ _delegations ] = new Map(); + if ( destinations ) { + destinations.delete( emitter ); } + } + } - // Originally there was a for..of loop which unfortunately caused an error in Babel that didn't allow - // build an application. See: https://github.com/ckeditor/ckeditor5-react/issues/40. - events.forEach( eventName => { - const destinations = this[ _delegations ]!.get( eventName ); + public _addEventListener( + event: TEvent[ 'name' ], + callback: GetCallback, + options: CallbackOptions + ): void { + createEventNamespace( this, event ); - if ( !destinations ) { - this[ _delegations ]!.set( eventName, new Map( [ [ emitter, nameOrFunction ] ] ) ); - } else { - destinations.set( emitter, nameOrFunction ); - } - } ); - } - }; - }, + const lists = getCallbacksListsForNamespace( this, event ); + const priority = priorities.get( options.priority ); - /** - * @inheritDoc - */ - stopDelegating( event?: string, emitter?: Emitter ) { - if ( !this[ _delegations ] ) { - return; + const callbackDefinition = { + callback, + priority + }; + + // Add the callback to all callbacks list. + for ( const callbacks of lists ) { + // Add the callback to the list in the right priority position. + insertToPriorityArray( callbacks, callbackDefinition ); + } } - if ( !event ) { - this[ _delegations ]!.clear(); - } else if ( !emitter ) { - this[ _delegations ]!.delete( event ); - } else { - const destinations = this[ _delegations ]!.get( event ); + public _removeEventListener( event: string, callback: Function ): void { + const lists = getCallbacksListsForNamespace( this, event ); - if ( destinations ) { - destinations.delete( emitter ); + for ( const callbacks of lists ) { + for ( let i = 0; i < callbacks.length; i++ ) { + if ( callbacks[ i ].callback == callback ) { + // Remove the callback from the list (fixing the next index). + callbacks.splice( i, 1 ); + i--; + } + } } } - }, - /** - * @inheritDoc - */ - _addEventListener( event, callback, options ) { - createEventNamespace( this, event ); + public _events?: { [ eventName: string ]: EventNode }; - const lists = getCallbacksListsForNamespace( this, event ); - const priority = priorities.get( options.priority ); + public [ _emitterId ]?: string; - const callbackDefinition = { - callback, - priority + public [ _listeningTo ]?: { + [ emitterId: string ]: { + emitter: Emitter; + callbacks: { [ event: string]: Function[] }; + }; }; - // Add the callback to all callbacks list. - for ( const callbacks of lists ) { - // Add the callback to the list in the right priority position. - insertToPriorityArray( callbacks, callbackDefinition ); - } - }, - - /** - * @inheritDoc - */ - _removeEventListener( event, callback ) { - const lists = getCallbacksListsForNamespace( this, event ); - - for ( const callbacks of lists ) { - for ( let i = 0; i < callbacks.length; i++ ) { - if ( callbacks[ i ].callback == callback ) { - // Remove the callback from the list (fixing the next index). - callbacks.splice( i, 1 ); - i--; - } - } - } + public [ _delegations ]?: Map string ) | undefined>>; } -}; -export default EmitterMixin; + return Mixin as any; +} + +export const Emitter = EmitterMixin( Object ); + +EmitterMixin.mixinMethods = Emitter.prototype; /** * Emitter/listener interface. @@ -492,11 +508,13 @@ export interface Emitter { * If omitted, stops delegation of `event` to all emitters. */ stopDelegating( event?: string, emitter?: Emitter ): void; +} + +interface EmitterInternal extends Emitter { /** * Adds callback to emitter for given event. * - * @internal * @protected * @method #_addEventListener * @param {String} event The name of the event. @@ -506,30 +524,26 @@ export interface Emitter { * the priority value the sooner the callback will be fired. Events having the same priority are called in the * order they were added. */ - _addEventListener( + _addEventListener?: ( event: TEvent[ 'name' ], callback: GetCallback, options: CallbackOptions - ): void; + ) => void; /** * Removes callback from emitter for given event. * - * @internal * @protected * @method #_removeEventListener * @param {String} event The name of the event. * @param {Function} callback The function to stop being called. */ - _removeEventListener( event: string, callback: Function ): void; + _removeEventListener?: ( event: string, callback: Function ) => void; - /** @internal */ _events?: { [ eventName: string ]: EventNode }; - /** @internal */ [ _emitterId ]?: string; - /** @internal */ [ _listeningTo ]?: { [ emitterId: string ]: { emitter: Emitter; @@ -537,7 +551,6 @@ export interface Emitter { }; }; - /** @internal */ [ _delegations ]?: Map string ) | undefined>>; } @@ -579,7 +592,7 @@ export interface CallbackOptions { * @returns {module:utils/emittermixin~Emitter|null} */ export function _getEmitterListenedTo( listeningEmitter: Emitter, listenedToEmitterId: string ): Emitter | null { - const listeningTo = listeningEmitter[ _listeningTo ]; + const listeningTo = ( listeningEmitter as EmitterInternal )[ _listeningTo ]; if ( listeningTo && listeningTo[ listenedToEmitterId ] ) { return listeningTo[ listenedToEmitterId ].emitter; } @@ -598,8 +611,8 @@ export function _getEmitterListenedTo( listeningEmitter: Emitter, listenedToEmit * @param {String} [id] Unique id to set. If not passed, random unique id will be set. */ export function _setEmitterId( emitter: Emitter, id?: string ): void { - if ( !emitter[ _emitterId ] ) { - emitter[ _emitterId ] = id || uid(); + if ( !( emitter as EmitterInternal )[ _emitterId ] ) { + ( emitter as EmitterInternal )[ _emitterId ] = id || uid(); } } @@ -612,7 +625,7 @@ export function _setEmitterId( emitter: Emitter, id?: string ): void { * @returns {String|undefined} */ export function _getEmitterId( emitter: Emitter ): string | undefined { - return emitter[ _emitterId ]; + return ( emitter as EmitterInternal )[ _emitterId ]; } interface EventNode { @@ -623,7 +636,7 @@ interface EventNode { // Gets the internal `_events` property of the given object. // `_events` property store all lists with callbacks for registered event names. // If there were no events registered on the object, empty `_events` object is created. -function getEvents( source: Emitter ): { [ eventName: string ]: EventNode } { +function getEvents( source: EmitterInternal ): { [ eventName: string ]: EventNode } { if ( !source._events ) { Object.defineProperty( source, '_events', { value: {} @@ -646,7 +659,7 @@ function makeEventNode(): EventNode { // is foo:bar:abc, it will create foo:bar:abc, foo:bar and foo event and tie them together. // It also copies callbacks from more generic events to more specific events when // specific events are created. -function createEventNamespace( source: Emitter, eventName: string ): void { +function createEventNamespace( source: EmitterInternal, eventName: string ): void { const events = getEvents( source ); // First, check if the event we want to add to the structure already exists. @@ -711,7 +724,7 @@ function createEventNamespace( source: Emitter, eventName: string ): void { // Gets an array containing callbacks list for a given event and it's more specific events. // I.e. if given event is foo:bar and there is also foo:bar:abc event registered, this will // return callback list of foo:bar and foo:bar:abc (but not foo). -function getCallbacksListsForNamespace( source: Emitter, eventName: string ): EventNode[ 'callbacks' ][] { +function getCallbacksListsForNamespace( source: EmitterInternal, eventName: string ): EventNode[ 'callbacks' ][] { const eventNode = getEvents( source )[ eventName ]; if ( !eventNode ) { @@ -732,7 +745,7 @@ function getCallbacksListsForNamespace( source: Emitter, eventName: string ): Ev // Get the list of callbacks for a given event, but only if there any callbacks have been registered. // If there are no callbacks registered for given event, it checks if this is a specific event and looks // for callbacks for it's more generic version. -function getCallbacksForEvent( source: Emitter, eventName: string ): EventNode[ 'callbacks' ] | null { +function getCallbacksForEvent( source: EmitterInternal, eventName: string ): EventNode[ 'callbacks' ] | null { let event; if ( !source._events || !( event = source._events[ eventName ] ) || !event.callbacks.length ) { @@ -779,8 +792,8 @@ function fireDelegatedEvents( // Helper for registering event callback on the emitter. function addEventListener( - listener: Emitter, - emitter: Emitter, + listener: EmitterInternal, + emitter: EmitterInternal, event: TEvent[ 'name' ], callback: GetCallback, options: CallbackOptions @@ -790,18 +803,18 @@ function addEventListener( } else { // Allow listening on objects that do not implement Emitter interface. // This is needed in some tests that are using mocks instead of the real objects with EmitterMixin mixed. - listener._addEventListener.call( emitter, event, callback, options ); + listener._addEventListener!.call( emitter, event, callback, options ); } } // Helper for removing event callback from the emitter. -function removeEventListener( listener: Emitter, emitter: Emitter, event: string, callback: Function ): void { +function removeEventListener( listener: EmitterInternal, emitter: EmitterInternal, event: string, callback: Function ): void { if ( emitter._removeEventListener ) { emitter._removeEventListener( event, callback ); } else { // Allow listening on objects that do not implement Emitter interface. // This is needed in some tests that are using mocks instead of the real objects with EmitterMixin mixed. - listener._removeEventListener.call( emitter, event, callback ); + listener._removeEventListener!.call( emitter, event, callback ); } } diff --git a/packages/ckeditor5-utils/src/mix.ts b/packages/ckeditor5-utils/src/mix.ts index f3f525621b0..80388f8fa6d 100644 --- a/packages/ckeditor5-utils/src/mix.ts +++ b/packages/ckeditor5-utils/src/mix.ts @@ -30,8 +30,12 @@ * @param {Function} [baseClass] Class which prototype will be extended. * @param {Object} [...mixins] Objects from which to get properties. */ -export default function mix( baseClass: Function, ...mixins: object[] ): void { +export default function mix( baseClass: Function, ...mixins: any[] ): void { mixins.forEach( mixin => { + if ( 'mixinMethods' in mixin ) { + mixin = mixin.mixinMethods; + } + const propertyNames: ( string | symbol )[] = Object.getOwnPropertyNames( mixin ); const propertySymbols = Object.getOwnPropertySymbols( mixin ); diff --git a/packages/ckeditor5-utils/src/observablemixin.ts b/packages/ckeditor5-utils/src/observablemixin.ts index 89fcec04b34..0f786743faf 100644 --- a/packages/ckeditor5-utils/src/observablemixin.ts +++ b/packages/ckeditor5-utils/src/observablemixin.ts @@ -9,7 +9,7 @@ * @module utils/observablemixin */ -import EmitterMixin, { type Emitter } from './emittermixin'; +import EmitterMixin, { Emitter } from './emittermixin'; import CKEditorError from './ckeditorerror'; import { isObject } from 'lodash-es'; @@ -270,9 +270,19 @@ const ObservableMixin: Observable = { this[ decoratedMethods ]!.push( methodName ); }, - ...EmitterMixin + on: Emitter.prototype.on, + once: Emitter.prototype.once, + fire: Emitter.prototype.fire, + listenTo: Emitter.prototype.listenTo, + off: Emitter.prototype.off, + stopListening: Emitter.prototype.stopListening, + delegate: Emitter.prototype.delegate, + stopDelegating: Emitter.prototype.stopDelegating }; +( ObservableMixin as any )._addEventListener = ( Emitter.prototype as any )._addEventListener; +( ObservableMixin as any )._removeEventListener = ( Emitter.prototype as any )._removeEventListener; + // Override the EmitterMixin stopListening method to be able to clean (and restore) decorated methods. // This is needed in case of: // 1. Have x.foo() decorated. @@ -293,7 +303,7 @@ ObservableMixin.stopListening = function( delete this[ decoratedMethods ]; } - EmitterMixin.stopListening.call( this, emitter, event, callback ); + Emitter.prototype.stopListening.call( this, emitter, event, callback ); }; export default ObservableMixin; diff --git a/packages/ckeditor5-utils/tests/_utils-tests/utils.js b/packages/ckeditor5-utils/tests/_utils-tests/utils.js index 8284dd74920..63453b257de 100644 --- a/packages/ckeditor5-utils/tests/_utils-tests/utils.js +++ b/packages/ckeditor5-utils/tests/_utils-tests/utils.js @@ -5,7 +5,7 @@ import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; import ObservableMixin from '../../src/observablemixin'; -import EmitterMixin from '../../src/emittermixin'; +import { Emitter } from '../../src/emittermixin'; import { createObserver } from '../_utils/utils'; describe( 'utils - testUtils', () => { @@ -29,9 +29,6 @@ describe( 'utils - testUtils', () => { } ); it( 'should create an observer', () => { - function Emitter() {} - Emitter.prototype = EmitterMixin; - expect( observer ).to.be.instanceof( Emitter ); expect( observer.observe ).is.a( 'function' ); expect( observer.stopListening ).is.a( 'function' ); diff --git a/packages/ckeditor5-utils/tests/_utils/utils.js b/packages/ckeditor5-utils/tests/_utils/utils.js index 192f9cc3294..173dfa5d8ec 100644 --- a/packages/ckeditor5-utils/tests/_utils/utils.js +++ b/packages/ckeditor5-utils/tests/_utils/utils.js @@ -5,7 +5,7 @@ /* global console:false */ -import EmitterMixin from '../../src/emittermixin'; +import { Emitter } from '../../src/emittermixin'; import CKEditorError from '../../src/ckeditorerror'; import areConnectedThroughProperties from '../../src/areconnectedthroughproperties'; @@ -31,7 +31,7 @@ import areConnectedThroughProperties from '../../src/areconnectedthroughproperti * @returns {Emitter} The observer. */ export function createObserver() { - const observer = Object.create( EmitterMixin, { + const observer = Object.create( Emitter.prototype, { observe: { value: function observe( observableName, observable, filterNames ) { observer.listenTo( observable, 'change', ( evt, propertyName, value, oldValue ) => { diff --git a/packages/ckeditor5-utils/tests/dom/emittermixin.js b/packages/ckeditor5-utils/tests/dom/emittermixin.js index 727ea8ae01c..8907896b200 100644 --- a/packages/ckeditor5-utils/tests/dom/emittermixin.js +++ b/packages/ckeditor5-utils/tests/dom/emittermixin.js @@ -7,7 +7,7 @@ import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; import DomEmitterMixin from '../../src/dom/emittermixin'; -import EmitterMixin from '../../src/emittermixin'; +import { Emitter } from '../../src/emittermixin'; describe( 'DomEmitterMixin', () => { let emitter, domEmitter, node; @@ -15,7 +15,7 @@ describe( 'DomEmitterMixin', () => { testUtils.createSinonSandbox(); beforeEach( () => { - emitter = Object.create( EmitterMixin ); + emitter = new Emitter(); domEmitter = Object.create( DomEmitterMixin ); node = document.createElement( 'div' ); } ); diff --git a/packages/ckeditor5-utils/tests/emittermixin.js b/packages/ckeditor5-utils/tests/emittermixin.js index 986b2471a44..0d4022ad85d 100644 --- a/packages/ckeditor5-utils/tests/emittermixin.js +++ b/packages/ckeditor5-utils/tests/emittermixin.js @@ -3,7 +3,9 @@ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license */ -import { default as EmitterMixin, _getEmitterListenedTo, _getEmitterId, _setEmitterId } from '../src/emittermixin'; +/* eslint-disable new-cap */ + +import EmitterMixin, { _getEmitterListenedTo, _getEmitterId, _setEmitterId } from '../src/emittermixin'; import EventInfo from '../src/eventinfo'; import { expectToThrowCKEditorError } from './_utils/utils'; import CKEditorError from '../src/ckeditorerror'; @@ -16,6 +18,23 @@ describe( 'EmitterMixin', () => { listener = getEmitterInstance(); } ); + describe( 'create', () => { + it( 'should inherit from the given class', () => { + class TestClass { + constructor( value ) { + this.value = value; + } + } + + const EmitterClass = EmitterMixin( TestClass ); + + const emitter = new EmitterClass( 5 ); + + expect( emitter ).to.be.instanceOf( TestClass ); + expect( emitter.value ).to.equal( 5 ); + } ); + } ); + describe( 'fire', () => { it( 'should execute callbacks in the right order without priority', () => { const spy1 = sinon.spy().named( 1 ); @@ -1413,5 +1432,7 @@ describe( '_getEmitterListenedTo', () => { } ); function getEmitterInstance() { - return Object.create( EmitterMixin ); + class BrandNewClass {} + + return new ( EmitterMixin( BrandNewClass ) )(); } diff --git a/packages/ckeditor5-utils/tests/keystrokehandler.js b/packages/ckeditor5-utils/tests/keystrokehandler.js index f47a286fa7a..8b830eef5df 100644 --- a/packages/ckeditor5-utils/tests/keystrokehandler.js +++ b/packages/ckeditor5-utils/tests/keystrokehandler.js @@ -3,7 +3,7 @@ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license */ -import EmitterMixin from '../src/emittermixin'; +import { Emitter } from '../src/emittermixin'; import KeystrokeHandler from '../src/keystrokehandler'; import { keyCodes } from '../src/keyboard'; import env from '../src/env'; @@ -15,7 +15,7 @@ describe( 'KeystrokeHandler', () => { beforeEach( () => { env.isMac = false; - emitter = Object.create( EmitterMixin ); + emitter = new Emitter(); keystrokes = new KeystrokeHandler(); keystrokes.listenTo( emitter ); diff --git a/packages/ckeditor5-utils/tests/observablemixin.js b/packages/ckeditor5-utils/tests/observablemixin.js index c64d577636b..c009689a2ea 100644 --- a/packages/ckeditor5-utils/tests/observablemixin.js +++ b/packages/ckeditor5-utils/tests/observablemixin.js @@ -6,7 +6,7 @@ import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; import { assertBinding, expectToThrowCKEditorError } from '../tests/_utils/utils'; import ObservableMixin from '../src/observablemixin'; -import EmitterMixin from '../src/emittermixin'; +import { Emitter } from '../src/emittermixin'; import EventInfo from '../src/eventinfo'; import mix from '../src/mix'; @@ -18,7 +18,7 @@ describe( 'ObservableMixin', () => { } ); it( 'mixes in EmitterMixin', () => { - expect( ObservableMixin ).to.have.property( 'on', EmitterMixin.on ); + expect( ObservableMixin ).to.have.property( 'on', Emitter.prototype.on ); } ); it( 'implements set, bind, and unbind methods', () => { From 531f4061a5aaf2d73ae6008c27e23e7adbb8752d Mon Sep 17 00:00:00 2001 From: Arkadiusz Filipczak Date: Thu, 21 Jul 2022 16:08:31 +0200 Subject: [PATCH 2/6] Changed mixin style: ObservableMixin --- .../ckeditor5-utils/src/observablemixin.ts | 461 +++++++++--------- .../tests/_utils-tests/utils.js | 6 +- .../ckeditor5-utils/tests/emittermixin.js | 20 +- .../ckeditor5-utils/tests/observablemixin.js | 93 +++- 4 files changed, 308 insertions(+), 272 deletions(-) diff --git a/packages/ckeditor5-utils/src/observablemixin.ts b/packages/ckeditor5-utils/src/observablemixin.ts index 0f786743faf..64d06f91309 100644 --- a/packages/ckeditor5-utils/src/observablemixin.ts +++ b/packages/ckeditor5-utils/src/observablemixin.ts @@ -3,13 +3,13 @@ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license */ -/* eslint-disable @typescript-eslint/unified-signatures */ +/* eslint-disable @typescript-eslint/unified-signatures, new-cap */ /** * @module utils/observablemixin */ -import EmitterMixin, { Emitter } from './emittermixin'; +import { Emitter } from './emittermixin'; import CKEditorError from './ckeditorerror'; import { isObject } from 'lodash-es'; @@ -33,280 +33,275 @@ const decoratedOriginal = Symbol( 'decoratedOriginal' ); * @mixes module:utils/emittermixin~EmitterMixin * @implements module:utils/observablemixin~Observable */ -const ObservableMixin: Observable = { - /** - * @inheritDoc - */ - set( name: string | { [ name: string ]: unknown }, value?: unknown ): void { - // If the first parameter is an Object, iterate over its properties. - if ( isObject( name ) ) { - Object.keys( name ).forEach( property => { - this.set( property as any, name[ property ] ); - }, this ); - - return; - } +export default function ObservableMixin Emitter>( + base: Base +): { + new( ...args: ConstructorParameters ): InstanceType & Observable; + prototype: InstanceType & Observable; +} { + class Mixin extends base implements ObservableInternal { + public set( name: string | { [ name: string ]: unknown }, value?: unknown ): void { + // If the first parameter is an Object, iterate over its properties. + if ( isObject( name ) ) { + Object.keys( name ).forEach( property => { + this.set( property as any, name[ property ] ); + }, this ); + + return; + } - initObservable( this ); + initObservable( this ); - const properties = this[ observablePropertiesSymbol ]; + const properties = this[ observablePropertiesSymbol ]; - if ( ( name in this ) && !properties!.has( name ) ) { - /** - * Cannot override an existing property. - * - * This error is thrown when trying to {@link ~Observable#set set} a property with - * a name of an already existing property. For example: - * - * let observable = new Model(); - * observable.property = 1; - * observable.set( 'property', 2 ); // throws - * - * observable.set( 'property', 1 ); - * observable.set( 'property', 2 ); // ok, because this is an existing property. - * - * @error observable-set-cannot-override - */ - throw new CKEditorError( 'observable-set-cannot-override', this ); - } + if ( ( name in this ) && !properties!.has( name ) ) { + /** + * Cannot override an existing property. + * + * This error is thrown when trying to {@link ~Observable#set set} a property with + * a name of an already existing property. For example: + * + * let observable = new Model(); + * observable.property = 1; + * observable.set( 'property', 2 ); // throws + * + * observable.set( 'property', 1 ); + * observable.set( 'property', 2 ); // ok, because this is an existing property. + * + * @error observable-set-cannot-override + */ + throw new CKEditorError( 'observable-set-cannot-override', this ); + } - Object.defineProperty( this, name, { - enumerable: true, - configurable: true, + Object.defineProperty( this, name, { + enumerable: true, + configurable: true, - get() { - return properties!.get( name ); - }, + get() { + return properties!.get( name ); + }, - set( this: Observable, value ) { - const oldValue = properties!.get( name ); + set( this: Observable, value ) { + const oldValue = properties!.get( name ); - // Fire `set` event before the new value will be set to make it possible - // to override observable property without affecting `change` event. - // See https://github.com/ckeditor/ckeditor5-utils/issues/171. - let newValue = this.fire( `set:${ name }`, name, value, oldValue ); + // Fire `set` event before the new value will be set to make it possible + // to override observable property without affecting `change` event. + // See https://github.com/ckeditor/ckeditor5-utils/issues/171. + let newValue = this.fire( `set:${ name }`, name, value, oldValue ); - if ( newValue === undefined ) { - newValue = value; - } + if ( newValue === undefined ) { + newValue = value; + } - // Allow undefined as an initial value like A.define( 'x', undefined ) (#132). - // Note: When properties map has no such own property, then its value is undefined. - if ( oldValue !== newValue || !properties!.has( name ) ) { - properties!.set( name, newValue ); - this.fire( `change:${ name }`, name, newValue, oldValue ); + // Allow undefined as an initial value like A.define( 'x', undefined ) (#132). + // Note: When properties map has no such own property, then its value is undefined. + if ( oldValue !== newValue || !properties!.has( name ) ) { + properties!.set( name, newValue ); + this.fire( `change:${ name }`, name, newValue, oldValue ); + } } - } - } ); - - ( this as any )[ name ] = value; - }, - - /** - * @inheritDoc - */ - bind( ...bindProperties: string[] ): any { - if ( !bindProperties.length || !isStringArray( bindProperties ) ) { - /** - * All properties must be strings. - * - * @error observable-bind-wrong-properties - */ - throw new CKEditorError( 'observable-bind-wrong-properties', this ); - } + } ); - if ( ( new Set( bindProperties ) ).size !== bindProperties.length ) { - /** - * Properties must be unique. - * - * @error observable-bind-duplicate-properties - */ - throw new CKEditorError( 'observable-bind-duplicate-properties', this ); + ( this as any )[ name ] = value; } - initObservable( this ); - - const boundProperties = this[ boundPropertiesSymbol ]; + public bind( ...bindProperties: string[] ): any { + if ( !bindProperties.length || !isStringArray( bindProperties ) ) { + /** + * All properties must be strings. + * + * @error observable-bind-wrong-properties + */ + throw new CKEditorError( 'observable-bind-wrong-properties', this ); + } - bindProperties.forEach( propertyName => { - if ( boundProperties!.has( propertyName ) ) { + if ( ( new Set( bindProperties ) ).size !== bindProperties.length ) { /** - * Cannot bind the same property more than once. + * Properties must be unique. * - * @error observable-bind-rebind + * @error observable-bind-duplicate-properties */ - throw new CKEditorError( 'observable-bind-rebind', this ); + throw new CKEditorError( 'observable-bind-duplicate-properties', this ); } - } ); - const bindings = new Map(); + initObservable( this ); - // @typedef {Object} Binding - // @property {Array} property Property which is bound. - // @property {Array} to Array of observable–property components of the binding (`{ observable: ..., property: .. }`). - // @property {Array} callback A function which processes `to` components. - bindProperties.forEach( a => { - const binding = { property: a, to: [] }; + const boundProperties = this[ boundPropertiesSymbol ]; - boundProperties!.set( a, binding ); - bindings.set( a, binding ); - } ); + bindProperties.forEach( propertyName => { + if ( boundProperties!.has( propertyName ) ) { + /** + * Cannot bind the same property more than once. + * + * @error observable-bind-rebind + */ + throw new CKEditorError( 'observable-bind-rebind', this ); + } + } ); - // @typedef {Object} BindChain - // @property {Function} to See {@link ~ObservableMixin#_bindTo}. - // @property {Function} toMany See {@link ~ObservableMixin#_bindToMany}. - // @property {module:utils/observablemixin~Observable} _observable The observable which initializes the binding. - // @property {Array} _bindProperties Array of `_observable` properties to be bound. - // @property {Array} _to Array of `to()` observable–properties (`{ observable: toObservable, properties: ...toProperties }`). - // @property {Map} _bindings Stores bindings to be kept in - // {@link ~ObservableMixin#_boundProperties}/{@link ~ObservableMixin#_boundObservables} - // initiated in this binding chain. - return { - to: bindTo, - toMany: bindToMany, - - _observable: this, - _bindProperties: bindProperties, - _to: [], - _bindings: bindings - }; - }, + const bindings = new Map(); - /** - * @inheritDoc - */ - unbind( ...unbindProperties: string[] ): void { - // Nothing to do here if not inited yet. - if ( !( this[ observablePropertiesSymbol ] ) ) { - return; - } + // @typedef {Object} Binding + // @property {Array} property Property which is bound. + // @property {Array} to Array of observable–property components of the binding (`{ observable: ..., property: .. }`). + // @property {Array} callback A function which processes `to` components. + bindProperties.forEach( a => { + const binding = { property: a, to: [] }; - const boundProperties = this[ boundPropertiesSymbol ]!; - const boundObservables = this[ boundObservablesSymbol ]!; + boundProperties!.set( a, binding ); + bindings.set( a, binding ); + } ); - if ( unbindProperties.length ) { - if ( !isStringArray( unbindProperties ) ) { - /** - * Properties must be strings. - * - * @error observable-unbind-wrong-properties - */ - throw new CKEditorError( 'observable-unbind-wrong-properties', this ); - } + // @typedef {Object} BindChain + // @property {Function} to See {@link ~ObservableMixin#_bindTo}. + // @property {Function} toMany See {@link ~ObservableMixin#_bindToMany}. + // @property {module:utils/observablemixin~Observable} _observable The observable which initializes the binding. + // @property {Array} _bindProperties Array of `_observable` properties to be bound. + // @property {Array} _to Array of `to()` observable–properties (`{ observable: toObservable, properties: ...toProperties }`). + // @property {Map} _bindings Stores bindings to be kept in + // {@link ~ObservableMixin#_boundProperties}/{@link ~ObservableMixin#_boundObservables} + // initiated in this binding chain. + return { + to: bindTo, + toMany: bindToMany, + + _observable: this, + _bindProperties: bindProperties, + _to: [], + _bindings: bindings + }; + } - unbindProperties.forEach( propertyName => { - const binding = boundProperties.get( propertyName ); + public unbind( ...unbindProperties: ( keyof this & string )[] ): void { + // Nothing to do here if not inited yet. + if ( !( this[ observablePropertiesSymbol ] ) ) { + return; + } - // Nothing to do if the binding is not defined - if ( !binding ) { - return; + const boundProperties = this[ boundPropertiesSymbol ]!; + const boundObservables = this[ boundObservablesSymbol ]!; + + if ( unbindProperties.length ) { + if ( !isStringArray( unbindProperties ) ) { + /** + * Properties must be strings. + * + * @error observable-unbind-wrong-properties + */ + throw new CKEditorError( 'observable-unbind-wrong-properties', this ); } - binding.to.forEach( ( [ toObservable, toProperty ] ) => { - const toProperties = boundObservables.get( toObservable )!; - const toPropertyBindings = toProperties[ toProperty ]; - - toPropertyBindings.delete( binding ); + unbindProperties.forEach( propertyName => { + const binding = boundProperties.get( propertyName ); - if ( !toPropertyBindings.size ) { - delete toProperties[ toProperty ]; + // Nothing to do if the binding is not defined + if ( !binding ) { + return; } - if ( !Object.keys( toProperties ).length ) { - boundObservables.delete( toObservable ); - this.stopListening( toObservable, 'change' ); - } - } ); + binding.to.forEach( ( [ toObservable, toProperty ] ) => { + const toProperties = boundObservables.get( toObservable )!; + const toPropertyBindings = toProperties[ toProperty ]; - boundProperties.delete( propertyName ); - } ); - } else { - boundObservables.forEach( ( bindings, boundObservable ) => { - this.stopListening( boundObservable, 'change' ); - } ); + toPropertyBindings.delete( binding ); - boundObservables.clear(); - boundProperties.clear(); - } - }, + if ( !toPropertyBindings.size ) { + delete toProperties[ toProperty ]; + } - /** - * @inheritDoc - */ - decorate( this: Observable & { [ x: string ]: any }, methodName: string ): void { - initObservable( this ); + if ( !Object.keys( toProperties ).length ) { + boundObservables.delete( toObservable ); + this.stopListening( toObservable, 'change' ); + } + } ); - const originalMethod = this[ methodName ]; + boundProperties.delete( propertyName ); + } ); + } else { + boundObservables.forEach( ( bindings, boundObservable ) => { + this.stopListening( boundObservable, 'change' ); + } ); - if ( !originalMethod ) { - /** - * Cannot decorate an undefined method. - * - * @error observablemixin-cannot-decorate-undefined - * @param {Object} object The object which method should be decorated. - * @param {String} methodName Name of the method which does not exist. - */ - throw new CKEditorError( - 'observablemixin-cannot-decorate-undefined', - this, - { object: this, methodName } - ); + boundObservables.clear(); + boundProperties.clear(); + } } - this.on( methodName, ( evt, args ) => { - evt.return = originalMethod.apply( this, args ); - } ); + public decorate( this: this & { [ x: string ]: any }, methodName: keyof this & string ): void { + initObservable( this ); + + const originalMethod = this[ methodName ]; + + if ( !originalMethod ) { + /** + * Cannot decorate an undefined method. + * + * @error observablemixin-cannot-decorate-undefined + * @param {Object} object The object which method should be decorated. + * @param {String} methodName Name of the method which does not exist. + */ + throw new CKEditorError( + 'observablemixin-cannot-decorate-undefined', + this, + { object: this, methodName } + ); + } + + this.on( methodName, ( evt, args ) => { + evt.return = originalMethod.apply( this, args ); + } ); - this[ methodName ] = function( ...args: unknown[] ) { - return this.fire( methodName, args ); - }; + this[ methodName ] = function( ...args: unknown[] ) { + return this.fire( methodName, args ); + }; - this[ methodName ][ decoratedOriginal ] = originalMethod; + this[ methodName ][ decoratedOriginal ] = originalMethod; - if ( !this[ decoratedMethods ] ) { - this[ decoratedMethods ] = []; + if ( !this[ decoratedMethods ] ) { + this[ decoratedMethods ] = []; + } + + this[ decoratedMethods ]!.push( methodName ); } - this[ decoratedMethods ]!.push( methodName ); - }, - - on: Emitter.prototype.on, - once: Emitter.prototype.once, - fire: Emitter.prototype.fire, - listenTo: Emitter.prototype.listenTo, - off: Emitter.prototype.off, - stopListening: Emitter.prototype.stopListening, - delegate: Emitter.prototype.delegate, - stopDelegating: Emitter.prototype.stopDelegating -}; + // Override the EmitterMixin stopListening method to be able to clean (and restore) decorated methods. + // This is needed in case of: + // 1. Have x.foo() decorated. + // 2. Call x.stopListening() + // 3. Call x.foo(). Problem: nothing happens (the original foo() method is not executed) + public override stopListening( + this: ObservableInternal & { [ x: string ]: any }, + emitter?: Emitter, + event?: string, + callback?: Function + ): void { + // Removing all listeners so let's clean the decorated methods to the original state. + if ( !emitter && this[ decoratedMethods ] ) { + for ( const methodName of this[ decoratedMethods ]! ) { + this[ methodName ] = this[ methodName ][ decoratedOriginal ]; + } -( ObservableMixin as any )._addEventListener = ( Emitter.prototype as any )._addEventListener; -( ObservableMixin as any )._removeEventListener = ( Emitter.prototype as any )._removeEventListener; - -// Override the EmitterMixin stopListening method to be able to clean (and restore) decorated methods. -// This is needed in case of: -// 1. Have x.foo() decorated. -// 2. Call x.stopListening() -// 3. Call x.foo(). Problem: nothing happens (the original foo() method is not executed) -ObservableMixin.stopListening = function( - this: Observable & { [ x: string ]: any }, - emitter?, - event?, - callback? -): void { - // Removing all listeners so let's clean the decorated methods to the original state. - if ( !emitter && this[ decoratedMethods ] ) { - for ( const methodName of this[ decoratedMethods ]! ) { - this[ methodName ] = this[ methodName ][ decoratedOriginal ]; + delete this[ decoratedMethods ]; + } + + Emitter.prototype.stopListening.call( this, emitter, event, callback ); } - delete this[ decoratedMethods ]; + public [ observablePropertiesSymbol ]?: Map; + + public [ decoratedMethods ]?: string[]; + + public [ boundPropertiesSymbol ]?: Map; + + public [ boundObservablesSymbol]?: Map>>; } - Emitter.prototype.stopListening.call( this, emitter, event, callback ); -}; + return Mixin as any; +} -export default ObservableMixin; +export const Observable = ObservableMixin( Emitter ); + +ObservableMixin.mixinMethods = Observable.prototype; interface Binding { property: string; @@ -329,7 +324,7 @@ interface BindChainInternal { // // @private // @param {module:utils/observablemixin~ObservableMixin} observable -function initObservable( observable: T ): void { +function initObservable( observable: ObservableInternal ): void { // Do nothing if already inited. if ( observable[ observablePropertiesSymbol ] ) { return; @@ -597,7 +592,7 @@ function parseBindToArgs( ...args: ( Observable | string | Function )[] ) { // @param {Observable} toObservable A observable, which is a new component of `binding`. // @param {String} toPropertyName A name of `toObservable`'s property, a new component of the `binding`. function updateBoundObservables( - observable: Observable, + observable: ObservableInternal, binding: Binding, toObservable: Observable, toPropertyName: string @@ -678,7 +673,7 @@ function updateBindToBound( chain: BindChainInternal ): void { // @private // @param {Observable} observable A observable which property is to be updated. // @param {String} propertyName An property to be updated. -function updateBoundObservableProperty( observable: Observable, propertyName: string ): void { +function updateBoundObservableProperty( observable: ObservableInternal, propertyName: string ): void { const boundProperties = observable[ boundPropertiesSymbol ]!; const binding = boundProperties.get( propertyName )!; let propertyValue; @@ -709,7 +704,7 @@ function updateBoundObservableProperty( observable: Observable, propertyName: st // @private // @param {Observable} observable // @param {BindChain} chain The chain initialized by {@link Observable#bind}. -function attachBindToListeners( observable: Observable, toBindings: BindChainInternal[ '_to' ] ): void { +function attachBindToListeners( observable: ObservableInternal, toBindings: BindChainInternal[ '_to' ] ): void { toBindings.forEach( to => { const boundObservables = observable[ boundObservablesSymbol ]!; let bindings; @@ -957,17 +952,15 @@ export interface Observable extends Emitter { * @param {*} value The new property value. * @param {*} oldValue The previous property value. */ +} - /** @internal */ +interface ObservableInternal extends Observable { [ observablePropertiesSymbol ]?: Map; - /** @internal */ [ decoratedMethods ]?: string[]; - /** @internal */ [ boundPropertiesSymbol ]?: Map; - /** @internal */ [ boundObservablesSymbol]?: Map>>; } diff --git a/packages/ckeditor5-utils/tests/_utils-tests/utils.js b/packages/ckeditor5-utils/tests/_utils-tests/utils.js index 63453b257de..20d8a3dd590 100644 --- a/packages/ckeditor5-utils/tests/_utils-tests/utils.js +++ b/packages/ckeditor5-utils/tests/_utils-tests/utils.js @@ -4,7 +4,7 @@ */ import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; -import ObservableMixin from '../../src/observablemixin'; +import { Observable } from '../../src/observablemixin'; import { Emitter } from '../../src/emittermixin'; import { createObserver } from '../_utils/utils'; @@ -21,10 +21,10 @@ describe( 'utils - testUtils', () => { beforeEach( () => { observer = createObserver(); - observable = Object.create( ObservableMixin ); + observable = new Observable(); observable.set( { foo: 0, bar: 0 } ); - observable2 = Object.create( ObservableMixin ); + observable2 = new Observable(); observable2.set( { foo: 0, bar: 0 } ); } ); diff --git a/packages/ckeditor5-utils/tests/emittermixin.js b/packages/ckeditor5-utils/tests/emittermixin.js index 0d4022ad85d..3f78f74e666 100644 --- a/packages/ckeditor5-utils/tests/emittermixin.js +++ b/packages/ckeditor5-utils/tests/emittermixin.js @@ -18,21 +18,19 @@ describe( 'EmitterMixin', () => { listener = getEmitterInstance(); } ); - describe( 'create', () => { - it( 'should inherit from the given class', () => { - class TestClass { - constructor( value ) { - this.value = value; - } + it( 'should inherit from the given class', () => { + class TestClass { + constructor( value ) { + this.value = value; } + } - const EmitterClass = EmitterMixin( TestClass ); + const EmitterClass = EmitterMixin( TestClass ); - const emitter = new EmitterClass( 5 ); + const emitter = new EmitterClass( 5 ); - expect( emitter ).to.be.instanceOf( TestClass ); - expect( emitter.value ).to.equal( 5 ); - } ); + expect( emitter ).to.be.instanceOf( TestClass ); + expect( emitter.value ).to.equal( 5 ); } ); describe( 'fire', () => { diff --git a/packages/ckeditor5-utils/tests/observablemixin.js b/packages/ckeditor5-utils/tests/observablemixin.js index c009689a2ea..d1c63674902 100644 --- a/packages/ckeditor5-utils/tests/observablemixin.js +++ b/packages/ckeditor5-utils/tests/observablemixin.js @@ -3,45 +3,90 @@ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license */ +/* eslint-disable new-cap */ + import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; import { assertBinding, expectToThrowCKEditorError } from '../tests/_utils/utils'; -import ObservableMixin from '../src/observablemixin'; -import { Emitter } from '../src/emittermixin'; +import ObservableMixin, { Observable } from '../src/observablemixin'; +import EmitterMixin, { Emitter } from '../src/emittermixin'; import EventInfo from '../src/eventinfo'; -import mix from '../src/mix'; describe( 'ObservableMixin', () => { testUtils.createSinonSandbox(); it( 'exists', () => { - expect( ObservableMixin ).to.be.an( 'object' ); + expect( ObservableMixin ).to.be.a( 'function' ); } ); it( 'mixes in EmitterMixin', () => { - expect( ObservableMixin ).to.have.property( 'on', Emitter.prototype.on ); + expect( new Observable() ).to.have.property( 'on', Emitter.prototype.on ); } ); it( 'implements set, bind, and unbind methods', () => { - expect( ObservableMixin ).to.contain.keys( 'set', 'bind', 'unbind' ); + for ( const key of [ 'set', 'bind', 'unbind' ] ) { + expect( new Observable() ).to.have.property( key ); + } + } ); + + it( 'inherits any emitter directly', () => { + class TestClass { + constructor( value ) { + this.value = value; + } + } + + const EmitterClass = EmitterMixin( TestClass ); + const ObservableClass = ObservableMixin( EmitterClass ); + + const observable = new ObservableClass( 5 ); + + expect( observable ).to.be.instanceOf( TestClass ); + expect( observable.value ).to.equal( 5 ); + + for ( const key of [ 'set', 'bind', 'unbind' ] ) { + expect( observable ).to.have.property( key ); + } + } ); + + it( 'inherits any emitter indirectly', () => { + class TestClass extends Emitter { + constructor( value ) { + super(); + + this.value = value; + } + } + + const ObservableClass = ObservableMixin( TestClass ); + + const observable = new ObservableClass( 5 ); + + expect( observable ).to.be.instanceOf( TestClass ); + expect( observable.value ).to.equal( 5 ); + + for ( const key of [ 'set', 'bind', 'unbind' ] ) { + expect( observable ).to.have.property( key ); + } } ); } ); describe( 'Observable', () => { testUtils.createSinonSandbox(); - class Observable { + class BaseObservable extends Observable { constructor( properties ) { + super(); + if ( properties ) { this.set( properties ); } } } - mix( Observable, ObservableMixin ); let Car, car; beforeEach( () => { - Car = class extends Observable {}; + Car = class extends BaseObservable {}; car = new Car( { color: 'red', @@ -227,7 +272,7 @@ describe( 'Observable', () => { } ); it( 'should throw when overriding already existing property (in the prototype)', () => { - class Car extends Observable { + class Car extends BaseObservable { method() {} } @@ -300,7 +345,7 @@ describe( 'Observable', () => { describe( 'to()', () => { it( 'should not chain', () => { expect( - car.bind( 'color' ).to( new Observable( { color: 'red' } ) ) + car.bind( 'color' ).to( new BaseObservable( { color: 'red' } ) ) ).to.be.undefined; } ); @@ -807,13 +852,13 @@ describe( 'Observable', () => { let Wheel; beforeEach( () => { - Wheel = class extends Observable { + Wheel = class extends BaseObservable { }; } ); it( 'should not chain', () => { expect( - car.bind( 'color' ).toMany( [ new Observable( { color: 'red' } ) ], 'color', () => {} ) + car.bind( 'color' ).toMany( [ new BaseObservable( { color: 'red' } ) ], 'color', () => {} ) ).to.be.undefined; } ); @@ -863,13 +908,13 @@ describe( 'Observable', () => { describe( 'unbind()', () => { it( 'should not fail when unbinding a fresh observable', () => { - const observable = new Observable(); + const observable = new BaseObservable(); observable.unbind(); } ); it( 'should not fail when unbinding property that is not bound', () => { - const observable = new Observable(); + const observable = new BaseObservable(); observable.bind( 'foo' ).to( car, 'color' ); @@ -953,7 +998,7 @@ describe( 'Observable', () => { it( 'makes the method fire an event', () => { const spy = sinon.spy(); - class Foo extends Observable { + class Foo extends BaseObservable { method() {} } @@ -972,7 +1017,7 @@ describe( 'Observable', () => { it( 'executes the original method in a listener with the default priority', () => { const calls = []; - class Foo extends Observable { + class Foo extends BaseObservable { method() { calls.push( 'original' ); } @@ -991,7 +1036,7 @@ describe( 'Observable', () => { } ); it( 'supports overriding return values', () => { - class Foo extends Observable { + class Foo extends BaseObservable { method() { return 1; } @@ -1011,7 +1056,7 @@ describe( 'Observable', () => { } ); it( 'supports overriding arguments', () => { - class Foo extends Observable { + class Foo extends BaseObservable { method( a ) { expect( a ).to.equal( 2 ); } @@ -1029,7 +1074,7 @@ describe( 'Observable', () => { } ); it( 'supports stopping the event (which prevents execution of the original method)', () => { - class Foo extends Observable { + class Foo extends BaseObservable { method() { throw new Error( 'this should not be executed' ); } @@ -1047,7 +1092,7 @@ describe( 'Observable', () => { } ); it( 'throws when trying to decorate non existing method', () => { - class Foo extends Observable {} + class Foo extends BaseObservable {} const foo = new Foo(); @@ -1060,7 +1105,7 @@ describe( 'Observable', () => { const spyFoo = sinon.spy(); const spyBar = sinon.spy(); - class Foo extends Observable { + class Foo extends BaseObservable { methodFoo() {} methodBar() {} } @@ -1084,7 +1129,7 @@ describe( 'Observable', () => { } ); it( 'should reverts decorated methods to the original method on stopListening for all events', () => { - class Foo extends Observable { + class Foo extends BaseObservable { method() { } } @@ -1102,7 +1147,7 @@ describe( 'Observable', () => { } ); it( 'should not revert decorated methods to the original method on stopListening for specific emitter', () => { - class Foo extends Observable { + class Foo extends BaseObservable { method() { } } From 91710d4c3112ea5a27d8a2bf516d3ad7556396f3 Mon Sep 17 00:00:00 2001 From: Arkadiusz Filipczak Date: Fri, 22 Jul 2022 09:21:45 +0200 Subject: [PATCH 3/6] Changed mixin style: DomEmitterMixin --- .../ckeditor5-utils/src/dom/emittermixin.ts | 243 +++++++++--------- packages/ckeditor5-utils/src/emittermixin.ts | 9 +- packages/ckeditor5-utils/src/mix.ts | 10 +- .../ckeditor5-utils/src/observablemixin.ts | 10 +- .../ckeditor5-utils/tests/dom/emittermixin.js | 45 +++- 5 files changed, 187 insertions(+), 130 deletions(-) diff --git a/packages/ckeditor5-utils/src/dom/emittermixin.ts b/packages/ckeditor5-utils/src/dom/emittermixin.ts index 970aaf37392..fca17358d2a 100644 --- a/packages/ckeditor5-utils/src/dom/emittermixin.ts +++ b/packages/ckeditor5-utils/src/dom/emittermixin.ts @@ -3,12 +3,13 @@ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license */ +/* eslint-disable new-cap */ + /** * @module utils/dom/emittermixin */ import { - default as EmitterMixin, _getEmitterListenedTo, _setEmitterId, Emitter as BaseEmitter, @@ -19,9 +20,7 @@ import { import uid from '../uid'; import isNode from './isnode'; import isWindow from './iswindow'; -import { extend } from 'lodash-es'; import type EventInfo from '../eventinfo'; -import mix from '../mix'; /** * Mixin that injects the DOM events API into its host. It provides the API @@ -45,126 +44,115 @@ import mix from '../mix'; * @mixes module:utils/emittermixin~EmitterMixin * @implements module:utils/dom/emittermixin~Emitter */ -const DomEmitterMixin: Emitter = extend( {}, EmitterMixin.mixinMethods, { - /** - * Registers a callback function to be executed when an event is fired in a specific Emitter or DOM Node. - * It is backwards compatible with {@link module:utils/emittermixin~EmitterMixin#listenTo}. - * - * @param {module:utils/emittermixin~Emitter|Node|Window} emitter The object that fires the event. - * @param {String} event The name of the event. - * @param {Function} callback The function to be called on event. - * @param {Object} [options={}] Additional options. - * @param {module:utils/priorities~PriorityString|Number} [options.priority='normal'] The priority of this event callback. The higher - * the priority value the sooner the callback will be fired. Events having the same priority are called in the - * order they were added. - * @param {Boolean} [options.useCapture=false] Indicates that events of this type will be dispatched to the registered - * listener before being dispatched to any EventTarget beneath it in the DOM tree. - * @param {Boolean} [options.usePassive=false] Indicates that the function specified by listener will never call preventDefault() - * and prevents blocking browser's main thread by this event handler. - */ - listenTo( - emitter: BaseEmitter | Node | Window, - event: string, - callback: ( ev: EventInfo, ...args: any[] ) => void, - options: CallbackOptions & { readonly useCapture?: boolean; readonly usePassive?: boolean } = {} - ): void { - // Check if emitter is an instance of DOM Node. If so, use corresponding ProxyEmitter (or create one if not existing). - if ( isNode( emitter ) || isWindow( emitter ) ) { - const proxyOptions = { - capture: !!options.useCapture, - passive: !!options.usePassive - }; - - const proxyEmitter = this._getProxyEmitter( emitter, proxyOptions ) || new ProxyEmitter( emitter, proxyOptions ); - - this.listenTo( proxyEmitter, event, callback, options ); - } else { - // Execute parent class method with Emitter (or ProxyEmitter) instance. - BaseEmitter.prototype.listenTo.call( this, emitter, event, callback, options ); +export default function DomEmitterMixin BaseEmitter>( + base: Base +): { + new( ...args: ConstructorParameters ): InstanceType & Emitter; + prototype: InstanceType & Emitter; +} { + class Mixin extends base implements Emitter { + public override listenTo( + emitter: Node | Window, + event: K, + callback: ( this: this, ev: EventInfo, event: HTMLElementEventMap[ K ] ) => void, + options?: CallbackOptions & { readonly useCapture?: boolean; readonly usePassive?: boolean } + ): void; + public override listenTo( + emitter: BaseEmitter, + event: TEvent[ 'name' ], + callback: GetCallback, + options?: CallbackOptions + ): void; + public override listenTo( + emitter: BaseEmitter | Node | Window, + event: string, + callback: ( ev: EventInfo, ...args: any[] ) => void, + options: CallbackOptions & { readonly useCapture?: boolean; readonly usePassive?: boolean } = {} + ): void { + // Check if emitter is an instance of DOM Node. If so, use corresponding ProxyEmitter (or create one if not existing). + if ( isNode( emitter ) || isWindow( emitter ) ) { + const proxyOptions = { + capture: !!options.useCapture, + passive: !!options.usePassive + }; + + const proxyEmitter = this._getProxyEmitter( emitter, proxyOptions ) || new ProxyEmitter( emitter, proxyOptions ); + + this.listenTo( proxyEmitter, event, callback, options ); + } else { + // Execute parent class method with Emitter (or ProxyEmitter) instance. + BaseEmitter.prototype.listenTo.call( this, emitter, event, callback, options ); + } } - }, - - /** - * Stops listening for events. It can be used at different levels: - * It is backwards compatible with {@link module:utils/emittermixin~EmitterMixin#listenTo}. - * - * * To stop listening to a specific callback. - * * To stop listening to a specific event. - * * To stop listening to all events fired by a specific object. - * * To stop listening to all events fired by all object. - * - * @param {module:utils/emittermixin~Emitter|Node|Window} [emitter] The object to stop listening to. - * If omitted, stops it for all objects. - * @param {String} [event] (Requires the `emitter`) The name of the event to stop listening to. If omitted, stops it - * for all events from `emitter`. - * @param {Function} [callback] (Requires the `event`) The function to be removed from the call list for the given - * `event`. - */ - stopListening( - emitter?: BaseEmitter | Node | Window, - event?: string, - callback?: Function - ): void { - // Check if the emitter is an instance of DOM Node. If so, forward the call to the corresponding ProxyEmitters. - if ( isNode( emitter ) || isWindow( emitter ) ) { - const proxyEmitters = this._getAllProxyEmitters( emitter ); - for ( const proxy of proxyEmitters ) { - this.stopListening( proxy, event, callback ); + public override stopListening( + emitter?: BaseEmitter | Node | Window, + event?: string, + callback?: Function + ): void { + // Check if the emitter is an instance of DOM Node. If so, forward the call to the corresponding ProxyEmitters. + if ( isNode( emitter ) || isWindow( emitter ) ) { + const proxyEmitters = this._getAllProxyEmitters( emitter ); + + for ( const proxy of proxyEmitters ) { + this.stopListening( proxy, event, callback ); + } + } else { + // Execute parent class method with Emitter (or ProxyEmitter) instance. + BaseEmitter.prototype.stopListening.call( this, emitter, event, callback ); } - } else { - // Execute parent class method with Emitter (or ProxyEmitter) instance. - BaseEmitter.prototype.stopListening.call( this, emitter, event, callback ); } - }, - /** - * Retrieves ProxyEmitter instance for given DOM Node residing in this Host and given options. - * - * @private - * @param {Node|Window} node DOM Node of the ProxyEmitter. - * @param {Object} [options] Additional options. - * @param {Boolean} [options.useCapture=false] Indicates that events of this type will be dispatched to the registered - * listener before being dispatched to any EventTarget beneath it in the DOM tree. - * @param {Boolean} [options.usePassive=false] Indicates that the function specified by listener will never call preventDefault() - * and prevents blocking browser's main thread by this event handler. - * @returns {module:utils/dom/emittermixin~ProxyEmitter|null} ProxyEmitter instance bound to the DOM Node. - */ - _getProxyEmitter( - node: Node | Window, - options: { capture: boolean; passive: boolean } - ): BaseEmitter | null { - return _getEmitterListenedTo( this as any, getProxyEmitterId( node, options ) ); - }, + /** + * Retrieves ProxyEmitter instance for given DOM Node residing in this Host and given options. + * + * @private + * @param {Node|Window} node DOM Node of the ProxyEmitter. + * @param {Object} [options] Additional options. + * @param {Boolean} [options.useCapture=false] Indicates that events of this type will be dispatched to the registered + * listener before being dispatched to any EventTarget beneath it in the DOM tree. + * @param {Boolean} [options.usePassive=false] Indicates that the function specified by listener will never call preventDefault() + * and prevents blocking browser's main thread by this event handler. + * @returns {module:utils/dom/emittermixin~ProxyEmitter|null} ProxyEmitter instance bound to the DOM Node. + */ + private _getProxyEmitter( + node: Node | Window, + options: { capture: boolean; passive: boolean } + ): BaseEmitter | null { + return _getEmitterListenedTo( this, getProxyEmitterId( node, options ) ); + } - /** - * Retrieves all the ProxyEmitter instances for given DOM Node residing in this Host. - * - * @private - * @param {Node|Window} node DOM Node of the ProxyEmitter. - * @returns {Array.} - */ - _getAllProxyEmitters( node: Node | Window ): ProxyEmitter[] { - return [ - { capture: false, passive: false }, - { capture: false, passive: true }, - { capture: true, passive: false }, - { capture: true, passive: true } - ].map( options => this._getProxyEmitter( node, options ) ).filter( proxy => !!proxy ) as any; - }, - - on: BaseEmitter.prototype.on, - once: BaseEmitter.prototype.once, - fire: BaseEmitter.prototype.fire, - off: BaseEmitter.prototype.off, - delegate: BaseEmitter.prototype.delegate, - stopDelegating: BaseEmitter.prototype.stopDelegating -} ); + /** + * Retrieves all the ProxyEmitter instances for given DOM Node residing in this Host. + * + * @private + * @param {Node|Window} node DOM Node of the ProxyEmitter. + * @returns {Array.} + */ + private _getAllProxyEmitters( node: Node | Window ): ProxyEmitter[] { + return [ + { capture: false, passive: false }, + { capture: false, passive: true }, + { capture: true, passive: false }, + { capture: true, passive: true } + ].map( options => this._getProxyEmitter( node, options ) ).filter( proxy => !!proxy ) as any; + } + } + + return Mixin as any; +} -( DomEmitterMixin as any )._addEventListener = ( BaseEmitter.prototype as any )._addEventListener; -( DomEmitterMixin as any )._removeEventListener = ( BaseEmitter.prototype as any )._removeEventListener; +export const Emitter = DomEmitterMixin( BaseEmitter ); -export default DomEmitterMixin; +// Backward compatibility with `mix` +( [ + '_getProxyEmitter', '_getAllProxyEmitters', + 'on', 'once', 'off', 'listenTo', + 'stopListening', 'fire', 'delegate', 'stopDelegating', + '_addEventListener', '_removeEventListener' +] ).forEach( key => { + ( DomEmitterMixin as any )[ key ] = ( Emitter.prototype as any )[ key ]; +} ); /** * Creates a ProxyEmitter instance. Such an instance is a bridge between a DOM Node firing events @@ -198,7 +186,7 @@ export default DomEmitterMixin; * @implements module:utils/dom/emittermixin~Emitter * @private */ -class ProxyEmitter { +class ProxyEmitter extends BaseEmitter { private readonly _domNode: Node | Window; private readonly _options: { capture: boolean; passive: boolean }; @@ -214,6 +202,8 @@ class ProxyEmitter { node: Node | Window, options: { capture: boolean; passive: boolean } ) { + super(); + // Set emitter ID to match DOM Node "expando" property. _setEmitterId( this, getProxyEmitterId( node, options ) ); @@ -352,10 +342,6 @@ class ProxyEmitter { } } -mix( ProxyEmitter, EmitterMixin ); - -interface ProxyEmitter extends BaseEmitter {} - // Gets an unique DOM Node identifier. The identifier will be set if not defined. // // @private @@ -391,6 +377,23 @@ function getProxyEmitterId( node: Node | Window, options: { [ option: string ]: * @interface Emitter */ export interface Emitter extends BaseEmitter { + + /** + * Registers a callback function to be executed when an event is fired in a specific Emitter or DOM Node. + * It is backwards compatible with {@link module:utils/emittermixin~EmitterMixin#listenTo}. + * + * @param {module:utils/emittermixin~Emitter|Node|Window} emitter The object that fires the event. + * @param {String} event The name of the event. + * @param {Function} callback The function to be called on event. + * @param {Object} [options={}] Additional options. + * @param {module:utils/priorities~PriorityString|Number} [options.priority='normal'] The priority of this event callback. The higher + * the priority value the sooner the callback will be fired. Events having the same priority are called in the + * order they were added. + * @param {Boolean} [options.useCapture=false] Indicates that events of this type will be dispatched to the registered + * listener before being dispatched to any EventTarget beneath it in the DOM tree. + * @param {Boolean} [options.usePassive=false] Indicates that the function specified by listener will never call preventDefault() + * and prevents blocking browser's main thread by this event handler. + */ listenTo( emitter: Node | Window, event: K, diff --git a/packages/ckeditor5-utils/src/emittermixin.ts b/packages/ckeditor5-utils/src/emittermixin.ts index f8b99b8e76e..0d12f5a7470 100644 --- a/packages/ckeditor5-utils/src/emittermixin.ts +++ b/packages/ckeditor5-utils/src/emittermixin.ts @@ -340,7 +340,14 @@ export default function EmitterMixin object> export const Emitter = EmitterMixin( Object ); -EmitterMixin.mixinMethods = Emitter.prototype; +// Backward compatibility with `mix` +( [ + 'on', 'once', 'off', 'listenTo', + 'stopListening', 'fire', 'delegate', 'stopDelegating', + '_addEventListener', '_removeEventListener' +] ).forEach( key => { + ( EmitterMixin as any )[ key ] = ( Emitter.prototype as any )[ key ]; +} ); /** * Emitter/listener interface. diff --git a/packages/ckeditor5-utils/src/mix.ts b/packages/ckeditor5-utils/src/mix.ts index 80388f8fa6d..3caef94cdbe 100644 --- a/packages/ckeditor5-utils/src/mix.ts +++ b/packages/ckeditor5-utils/src/mix.ts @@ -30,12 +30,8 @@ * @param {Function} [baseClass] Class which prototype will be extended. * @param {Object} [...mixins] Objects from which to get properties. */ -export default function mix( baseClass: Function, ...mixins: any[] ): void { +export default function mix( baseClass: Function, ...mixins: object[] ): void { mixins.forEach( mixin => { - if ( 'mixinMethods' in mixin ) { - mixin = mixin.mixinMethods; - } - const propertyNames: ( string | symbol )[] = Object.getOwnPropertyNames( mixin ); const propertySymbols = Object.getOwnPropertySymbols( mixin ); @@ -44,6 +40,10 @@ export default function mix( baseClass: Function, ...mixins: any[] ): void { return; } + if ( typeof mixin == 'function' && ( key == 'length' || key == 'name' || key == 'prototype' ) ) { + return; + } + const sourceDescriptor = Object.getOwnPropertyDescriptor( mixin, key )!; sourceDescriptor.enumerable = false; diff --git a/packages/ckeditor5-utils/src/observablemixin.ts b/packages/ckeditor5-utils/src/observablemixin.ts index 64d06f91309..90d4c693b4b 100644 --- a/packages/ckeditor5-utils/src/observablemixin.ts +++ b/packages/ckeditor5-utils/src/observablemixin.ts @@ -301,7 +301,15 @@ export default function ObservableMixin Em export const Observable = ObservableMixin( Emitter ); -ObservableMixin.mixinMethods = Observable.prototype; +// Backward compatibility with `mix` +( [ + 'set', 'bind', 'unbind', 'decorate', + 'on', 'once', 'off', 'listenTo', + 'stopListening', 'fire', 'delegate', 'stopDelegating', + '_addEventListener', '_removeEventListener' +] ).forEach( key => { + ( ObservableMixin as any )[ key ] = ( Observable.prototype as any )[ key ]; +} ); interface Binding { property: string; diff --git a/packages/ckeditor5-utils/tests/dom/emittermixin.js b/packages/ckeditor5-utils/tests/dom/emittermixin.js index 8907896b200..b133cc56716 100644 --- a/packages/ckeditor5-utils/tests/dom/emittermixin.js +++ b/packages/ckeditor5-utils/tests/dom/emittermixin.js @@ -3,11 +3,13 @@ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license */ +/* eslint-disable new-cap */ + /* globals document, window, Event, MouseEvent */ import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; -import DomEmitterMixin from '../../src/dom/emittermixin'; -import { Emitter } from '../../src/emittermixin'; +import DomEmitterMixin, { Emitter as DomEmitter } from '../../src/dom/emittermixin'; +import EmitterMixin, { Emitter } from '../../src/emittermixin'; describe( 'DomEmitterMixin', () => { let emitter, domEmitter, node; @@ -16,7 +18,7 @@ describe( 'DomEmitterMixin', () => { beforeEach( () => { emitter = new Emitter(); - domEmitter = Object.create( DomEmitterMixin ); + domEmitter = new DomEmitter(); node = document.createElement( 'div' ); } ); @@ -24,6 +26,43 @@ describe( 'DomEmitterMixin', () => { domEmitter.stopListening(); } ); + it( 'mixes in EmitterMixin', () => { + expect( new DomEmitter() ).to.have.property( 'on', Emitter.prototype.on ); + } ); + + it( 'inherits any emitter directly', () => { + class TestClass { + constructor( value ) { + this.value = value; + } + } + + const EmitterClass = EmitterMixin( TestClass ); + const ObservableClass = DomEmitterMixin( EmitterClass ); + + const observable = new ObservableClass( 5 ); + + expect( observable ).to.be.instanceOf( TestClass ); + expect( observable.value ).to.equal( 5 ); + } ); + + it( 'inherits any emitter indirectly', () => { + class TestClass extends Emitter { + constructor( value ) { + super(); + + this.value = value; + } + } + + const ObservableClass = DomEmitterMixin( TestClass ); + + const observable = new ObservableClass( 5 ); + + expect( observable ).to.be.instanceOf( TestClass ); + expect( observable.value ).to.equal( 5 ); + } ); + describe( 'listenTo', () => { it( 'should listen to EmitterMixin events', () => { const spy = testUtils.sinon.spy(); From 76ad905d46c25d8f3607a03a781bd295adbd9919 Mon Sep 17 00:00:00 2001 From: Arkadiusz Filipczak Date: Fri, 22 Jul 2022 09:31:54 +0200 Subject: [PATCH 4/6] Changed mixin style: Apply new pattern --- packages/ckeditor5-utils/src/collection.ts | 13 ++++--------- .../ckeditor5-utils/src/dom/emittermixin.ts | 4 ++-- packages/ckeditor5-utils/src/focustracker.ts | 19 +++++++------------ packages/ckeditor5-utils/src/mix.ts | 1 + 4 files changed, 14 insertions(+), 23 deletions(-) diff --git a/packages/ckeditor5-utils/src/collection.ts b/packages/ckeditor5-utils/src/collection.ts index 810ebaaf1b2..e647684fed6 100644 --- a/packages/ckeditor5-utils/src/collection.ts +++ b/packages/ckeditor5-utils/src/collection.ts @@ -7,11 +7,10 @@ * @module utils/collection */ -import EmitterMixin, { type Emitter } from './emittermixin'; +import { Emitter } from './emittermixin'; import CKEditorError from './ckeditorerror'; import uid from './uid'; import isIterable from './isiterable'; -import mix from './mix'; /** * Collections are ordered sets of objects. Items in the collection can be retrieved by their indexes @@ -25,7 +24,7 @@ import mix from './mix'; * * @mixes module:utils/emittermixin~EmitterMixin */ -class Collection implements Iterable { +export default class Collection extends Emitter implements Iterable { /** * The internal list of items in the collection. * @@ -131,6 +130,8 @@ class Collection im * Items that do not have such a property will be assigned one when added to the collection. */ constructor( initialItemsOrOptions: Iterable | { readonly idProperty?: I } = {}, options: { readonly idProperty?: I } = {} ) { + super(); + const hasInitialItems = isIterable( initialItemsOrOptions ); if ( !hasInitialItems ) { @@ -781,12 +782,6 @@ class Collection im */ } -mix( Collection, EmitterMixin ); - -interface Collection extends Emitter {} - -export default Collection; - export type AddEvent = { name: 'add'; args: [ item: T, index: number ]; diff --git a/packages/ckeditor5-utils/src/dom/emittermixin.ts b/packages/ckeditor5-utils/src/dom/emittermixin.ts index fca17358d2a..3744a911639 100644 --- a/packages/ckeditor5-utils/src/dom/emittermixin.ts +++ b/packages/ckeditor5-utils/src/dom/emittermixin.ts @@ -31,9 +31,9 @@ import type EventInfo from '../eventinfo'; * * import mix from '../utils/mix.js'; * import DomEmitterMixin from '../utils/dom/emittermixin.js'; + * import { Emitter } from '../utils/emittermixin.js'; * - * class SomeView {} - * mix( SomeView, DomEmitterMixin ); + * class SomeView extends DomEmitterMixin( Emitter ) {} * * const view = new SomeView(); * view.listenTo( domElement, ( evt, domEvt ) => { diff --git a/packages/ckeditor5-utils/src/focustracker.ts b/packages/ckeditor5-utils/src/focustracker.ts index 92621405699..fcdb54ad47f 100644 --- a/packages/ckeditor5-utils/src/focustracker.ts +++ b/packages/ckeditor5-utils/src/focustracker.ts @@ -5,14 +5,15 @@ /* global setTimeout, clearTimeout */ +/* eslint-disable new-cap */ + /** * @module utils/focustracker */ -import DomEmitterMixin, { type Emitter as DomEmitter } from './dom/emittermixin'; -import ObservableMixin, { type Observable } from './observablemixin'; +import DomEmitterMixin from './dom/emittermixin'; +import { Observable } from './observablemixin'; import CKEditorError from './ckeditorerror'; -import mix from './mix'; /** * Allows observing a group of `Element`s whether at least one of them is focused. @@ -29,7 +30,7 @@ import mix from './mix'; * @mixes module:utils/dom/emittermixin~EmitterMixin * @mixes module:utils/observablemixin~ObservableMixin */ -class FocusTracker { +export default class FocusTracker extends DomEmitterMixin( Observable ) { /** * True when one of the registered elements is focused. * @@ -69,6 +70,8 @@ class FocusTracker { private _nextEventLoopTimeout: ReturnType | null; constructor() { + super(); + this.set( 'isFocused', false ); this.set( 'focusedElement', null ); @@ -150,11 +153,3 @@ class FocusTracker { }, 0 ); } } - -mix( FocusTracker, DomEmitterMixin ); -mix( FocusTracker, ObservableMixin ); - -type ObservableDomEmitter = DomEmitter & Observable; -interface FocusTracker extends ObservableDomEmitter {} - -export default FocusTracker; diff --git a/packages/ckeditor5-utils/src/mix.ts b/packages/ckeditor5-utils/src/mix.ts index 3caef94cdbe..50d687de520 100644 --- a/packages/ckeditor5-utils/src/mix.ts +++ b/packages/ckeditor5-utils/src/mix.ts @@ -27,6 +27,7 @@ * * Note: Properties which already exist in the base class will not be overriden. * + * @depreciated Use mixin pattern, see: https://www.typescriptlang.org/docs/handbook/mixins.html. * @param {Function} [baseClass] Class which prototype will be extended. * @param {Object} [...mixins] Objects from which to get properties. */ From c7380f2e287bce995ad2d5ef906c7828fdc83e2c Mon Sep 17 00:00:00 2001 From: Arkadiusz Filipczak Date: Mon, 25 Jul 2022 09:45:33 +0200 Subject: [PATCH 5/6] Changed mixin style: fixed abstract class problem --- .../ckeditor5-utils/src/dom/emittermixin.ts | 4 ++-- packages/ckeditor5-utils/src/emittermixin.ts | 4 ++-- packages/ckeditor5-utils/src/eventinfo.ts | 2 +- .../ckeditor5-utils/src/observablemixin.ts | 20 +++++++++++++++++-- 4 files changed, 23 insertions(+), 7 deletions(-) diff --git a/packages/ckeditor5-utils/src/dom/emittermixin.ts b/packages/ckeditor5-utils/src/dom/emittermixin.ts index 3744a911639..696d2b919f1 100644 --- a/packages/ckeditor5-utils/src/dom/emittermixin.ts +++ b/packages/ckeditor5-utils/src/dom/emittermixin.ts @@ -44,13 +44,13 @@ import type EventInfo from '../eventinfo'; * @mixes module:utils/emittermixin~EmitterMixin * @implements module:utils/dom/emittermixin~Emitter */ -export default function DomEmitterMixin BaseEmitter>( +export default function DomEmitterMixin BaseEmitter>( base: Base ): { new( ...args: ConstructorParameters ): InstanceType & Emitter; prototype: InstanceType & Emitter; } { - class Mixin extends base implements Emitter { + abstract class Mixin extends base implements Emitter { public override listenTo( emitter: Node | Window, event: K, diff --git a/packages/ckeditor5-utils/src/emittermixin.ts b/packages/ckeditor5-utils/src/emittermixin.ts index 0d12f5a7470..eb98626f121 100644 --- a/packages/ckeditor5-utils/src/emittermixin.ts +++ b/packages/ckeditor5-utils/src/emittermixin.ts @@ -33,13 +33,13 @@ const _delegations = Symbol( 'delegations' ); * @mixin EmitterMixin * @implements module:utils/emittermixin~Emitter */ -export default function EmitterMixin object>( +export default function EmitterMixin object>( base: Base ): { new( ...args: ConstructorParameters ): InstanceType & Emitter; prototype: InstanceType & Emitter; } { - class Mixin extends base implements EmitterInternal { + abstract class Mixin extends base implements EmitterInternal { public on( event: TEvent[ 'name' ], callback: GetCallback, diff --git a/packages/ckeditor5-utils/src/eventinfo.ts b/packages/ckeditor5-utils/src/eventinfo.ts index b8f9169c243..268e026fdb5 100644 --- a/packages/ckeditor5-utils/src/eventinfo.ts +++ b/packages/ckeditor5-utils/src/eventinfo.ts @@ -68,7 +68,7 @@ export default class EventInfo * * @member #return */ - public return: TReturn | undefined;; + public return: TReturn | undefined; /** * @param {Object} source The emitter. diff --git a/packages/ckeditor5-utils/src/observablemixin.ts b/packages/ckeditor5-utils/src/observablemixin.ts index 90d4c693b4b..889216fb6ce 100644 --- a/packages/ckeditor5-utils/src/observablemixin.ts +++ b/packages/ckeditor5-utils/src/observablemixin.ts @@ -33,13 +33,13 @@ const decoratedOriginal = Symbol( 'decoratedOriginal' ); * @mixes module:utils/emittermixin~EmitterMixin * @implements module:utils/observablemixin~Observable */ -export default function ObservableMixin Emitter>( +export default function ObservableMixin Emitter>( base: Base ): { new( ...args: ConstructorParameters ): InstanceType & Observable; prototype: InstanceType & Observable; } { - class Mixin extends base implements ObservableInternal { + abstract class Mixin extends base implements ObservableInternal { public set( name: string | { [ name: string ]: unknown }, value?: unknown ): void { // If the first parameter is an Object, iterate over its properties. if ( isObject( name ) ) { @@ -760,6 +760,13 @@ export interface Observable extends Emitter { * has a property with the given property name. This prevents from mistakenly overriding existing * properties and methods, but means that `foo.set( 'bar', 1 )` may be slightly slower than `foo.bar = 1`. * + * In TypeScript, those properties should be declared in class using `declare` keyword. In example: + * + * public declare myProp: number; + * constructor() { + * this.set( 'myProp', 2 ); + * } + * * @method #set * @param {String|Object} name The property's name or object with `name=>value` pairs. * @param {*} [value] The property's value (if `name` was passed in the first parameter). @@ -983,6 +990,15 @@ export type SetEvent = { return: TValue; }; +export type DecoratedMethodEvent< + TObservable extends Observable & { [ N in TName ]: ( ...args: any[] ) => any }, + TName extends keyof TObservable & string +> = { + name: TName; + args: [ Parameters ]; + return: ReturnType; +}; + interface SingleBindChain { toMany( observables: readonly O[], From 6ea6c44fa94d690d4351c1d4ea6b599c33812de2 Mon Sep 17 00:00:00 2001 From: Arkadiusz Filipczak Date: Thu, 28 Jul 2022 12:19:45 +0200 Subject: [PATCH 6/6] Mixin style: fix after code review --- .../ckeditor5-utils/src/dom/emittermixin.ts | 16 ++++++++ packages/ckeditor5-utils/tests/mix.js | 41 +++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/packages/ckeditor5-utils/src/dom/emittermixin.ts b/packages/ckeditor5-utils/src/dom/emittermixin.ts index 696d2b919f1..b0d81568f84 100644 --- a/packages/ckeditor5-utils/src/dom/emittermixin.ts +++ b/packages/ckeditor5-utils/src/dom/emittermixin.ts @@ -407,5 +407,21 @@ export interface Emitter extends BaseEmitter { options?: CallbackOptions ): void; + /** + * Stops listening for events. It can be used at different levels: + * It is backwards compatible with {@link module:utils/emittermixin~EmitterMixin#listenTo}. + * + * * To stop listening to a specific callback. + * * To stop listening to a specific event. + * * To stop listening to all events fired by a specific object. + * * To stop listening to all events fired by all object. + * + * @param {module:utils/emittermixin~Emitter|Node|Window} [emitter] The object to stop listening to. + * If omitted, stops it for all objects. + * @param {String} [event] (Requires the `emitter`) The name of the event to stop listening to. If omitted, stops it + * for all events from `emitter`. + * @param {Function} [callback] (Requires the `event`) The function to be removed from the call list for the given + * `event`. + */ stopListening( emitter?: BaseEmitter | Node | Window, event?: string, callback?: Function ): void; } diff --git a/packages/ckeditor5-utils/tests/mix.js b/packages/ckeditor5-utils/tests/mix.js index 4a64ba227d4..2e5c5235db1 100644 --- a/packages/ckeditor5-utils/tests/mix.js +++ b/packages/ckeditor5-utils/tests/mix.js @@ -149,5 +149,46 @@ describe( 'utils', () => { expect( bar.a() ).to.equal( 'foo' ); expect( bar.b() ).to.equal( 'b' ); } ); + + it( 'does not copy function built-in properties', () => { + class Foo {} + + function mixin() {} + + mixin.a = function() { + return 'a'; + }; + + mix( Foo, mixin ); + + const foo = new Foo(); + + expect( foo.a() ).to.equal( 'a' ); + expect( foo ).to.not.have.property( 'name' ); + expect( foo ).to.not.have.property( 'prototype' ); + expect( foo ).to.not.have.property( 'length' ); + } ); + + it( 'does copy function built-in properties if the mixin is not a function at all', () => { + class Foo {} + + const prototype = {}; + + const mixin = { + a() { return 'a'; }, + name: 'mixin', + length: 0, + prototype + }; + + mix( Foo, mixin ); + + const foo = new Foo(); + + expect( foo.a() ).to.equal( 'a' ); + expect( foo ).to.have.property( 'name' ).that.is.equal( 'mixin' ); + expect( foo ).to.have.property( 'prototype' ).that.is.equal( prototype ); + expect( foo ).to.have.property( 'length' ).that.is.equal( 0 ); + } ); } ); } );