From 6fe5b421c679449be123f661ff4f18995cfdba99 Mon Sep 17 00:00:00 2001 From: Kris Selden Date: Thu, 9 Aug 2018 13:04:11 -0700 Subject: [PATCH 1/2] Add tests for removeListener and removeObserver before prototype mixin has been applied. --- .../system/object/es-compatibility-test.js | 91 ++++++++++++++++++- 1 file changed, 90 insertions(+), 1 deletion(-) diff --git a/packages/ember-runtime/tests/system/object/es-compatibility-test.js b/packages/ember-runtime/tests/system/object/es-compatibility-test.js index bc4bbcdd455..facc0fed0d8 100644 --- a/packages/ember-runtime/tests/system/object/es-compatibility-test.js +++ b/packages/ember-runtime/tests/system/object/es-compatibility-test.js @@ -1,5 +1,16 @@ import EmberObject from '../../../lib/system/object'; -import { Mixin, defineProperty, computed, addObserver, addListener, sendEvent } from 'ember-metal'; +import { + Mixin, + defineProperty, + computed, + observer, + on, + addObserver, + removeObserver, + addListener, + removeListener, + sendEvent, +} from 'ember-metal'; import { moduleFor, AbstractTestCase } from 'internal-test-helpers'; moduleFor( @@ -297,6 +308,84 @@ moduleFor( SubEmberObject.metaForProperty('foo'); } + '@test observes / removeObserver on / removeListener interop'(assert) { + let fooDidChangeBase = 0; + let fooDidChangeA = 0; + let fooDidChangeB = 0; + let someEventBase = 0; + let someEventA = 0; + let someEventB = 0; + class A extends EmberObject.extend({ + fooDidChange: observer('foo', function() { + fooDidChangeBase++; + }), + + onSomeEvent: on('someEvent', function() { + someEventBase++; + }), + }) { + init() { + super.init(); + this.foo = 'bar'; + } + + fooDidChange() { + super.fooDidChange(); + fooDidChangeA++; + } + + onSomeEvent() { + super.onSomeEvent(); + someEventA++; + } + } + + class B extends A { + fooDidChange() { + super.fooDidChange(); + fooDidChangeB++; + } + + onSomeEvent() { + super.onSomeEvent(); + someEventB++; + } + } + + removeObserver(B.prototype, 'foo', null, 'fooDidChange'); + removeListener(B.prototype, 'someEvent', null, 'onSomeEvent'); + + assert.equal(fooDidChangeBase, 0); + assert.equal(fooDidChangeA, 0); + assert.equal(fooDidChangeB, 0); + + assert.equal(someEventBase, 0); + assert.equal(someEventA, 0); + assert.equal(someEventB, 0); + + let a = new A(); + a.set('foo', 'something'); + assert.equal(fooDidChangeBase, 1); + assert.equal(fooDidChangeA, 1); + assert.equal(fooDidChangeB, 0); + + sendEvent(a, 'someEvent'); + assert.equal(someEventBase, 1); + assert.equal(someEventA, 1); + assert.equal(someEventB, 0); + + let b = new B(); + b.set('foo', 'something'); + assert.equal(fooDidChangeBase, 1); + assert.equal(fooDidChangeA, 1); + assert.equal(fooDidChangeB, 0); + + sendEvent(b, 'someEvent'); + assert.equal(someEventBase, 1); + assert.equal(someEventA, 1); + assert.equal(someEventB, 0); + } + '@test super and _super interop between old and new methods'(assert) { let calls = []; let changes = []; From d0d61acce64d8e616a2e0fe089deace75355848e Mon Sep 17 00:00:00 2001 From: Kris Selden Date: Thu, 9 Aug 2018 15:37:36 -0700 Subject: [PATCH 2/2] Fix removeListener inheritance by making it lazier. --- packages/ember-meta/lib/meta.ts | 181 ++++++++++++---------- packages/ember-metal/tests/events_test.js | 9 +- 2 files changed, 102 insertions(+), 88 deletions(-) diff --git a/packages/ember-meta/lib/meta.ts b/packages/ember-meta/lib/meta.ts index a2e707cc338..4040f92741c 100644 --- a/packages/ember-meta/lib/meta.ts +++ b/packages/ember-meta/lib/meta.ts @@ -34,10 +34,24 @@ if (DEBUG) { export const UNDEFINED = symbol('undefined'); +function ALL() {} + // FLAGS -const SOURCE_DESTROYING = 1 << 1; -const SOURCE_DESTROYED = 1 << 2; -const META_DESTROYED = 1 << 3; +const enum MetaFlags { + NONE = 0, + SOURCE_DESTROYING = 1 << 0, + SOURCE_DESTROYED = 1 << 1, + META_DESTROYED = 1 << 2, +} + +const enum ListenerFlags { + NONE = 0, + REMOVE = 1 << 0, + ONCE = 1 << 1, +} + +type Listeners = Array; +type PublicListeners = Array; export class Meta { _descriptors: any | undefined; @@ -48,12 +62,11 @@ export class Meta { _chains: any | undefined; _tag: Tag | undefined; _tags: any | undefined; - _flags: number; + _flags: MetaFlags; source: object; proto: object | undefined; _parent: Meta | undefined | null; - _listeners: any | undefined; - _listenersFinalized: boolean; + private _listeners: Listeners | undefined; // DEBUG _values: any | undefined; @@ -79,7 +92,7 @@ export class Meta { // initial value for all flags right now is false // see FLAGS const for detailed list of flags used - this._flags = 0; + this._flags = MetaFlags.NONE; // used only internally this.source = obj; @@ -89,7 +102,6 @@ export class Meta { this.proto = obj.constructor === undefined ? undefined : obj.constructor.prototype; this._listeners = undefined; - this._listenersFinalized = false; } get parent() { @@ -119,27 +131,27 @@ export class Meta { } isSourceDestroying() { - return this._hasFlag(SOURCE_DESTROYING); + return this._hasFlag(MetaFlags.SOURCE_DESTROYING); } setSourceDestroying() { - this._flags |= SOURCE_DESTROYING; + this._flags |= MetaFlags.SOURCE_DESTROYING; } isSourceDestroyed() { - return this._hasFlag(SOURCE_DESTROYED); + return this._hasFlag(MetaFlags.SOURCE_DESTROYED); } setSourceDestroyed() { - this._flags |= SOURCE_DESTROYED; + this._flags |= MetaFlags.SOURCE_DESTROYED; } isMetaDestroyed() { - return this._hasFlag(META_DESTROYED); + return this._hasFlag(MetaFlags.META_DESTROYED); } setMetaDestroyed() { - this._flags |= META_DESTROYED; + this._flags |= MetaFlags.META_DESTROYED; } _hasFlag(flag: number) { @@ -440,82 +452,65 @@ export class Meta { method: Function | string, once: boolean ) { - if (this._listeners === undefined) { - this._listeners = []; - } - this._listeners.push(eventName, target, method, once); + this._addToListeners(eventName, target, method, once ? ListenerFlags.ONCE : ListenerFlags.NONE); } - _finalizeListeners() { - if (this._listenersFinalized) { - return; - } - if (this._listeners === undefined) { - this._listeners = []; - } - let pointer = this.parent; - while (pointer !== null) { - let listeners = pointer._listeners; - if (listeners !== undefined) { - this._listeners = this._listeners.concat(listeners); - } - if (pointer._listenersFinalized) { - break; - } - pointer = pointer.parent; + private _addToListeners( + eventName: string, + target: object | null, + method: Function | string, + flags: ListenerFlags + ) { + let { _listeners: listeners } = this; + if (listeners === undefined) { + listeners = this._listeners = []; } - this._listenersFinalized = true; + listeners.push(eventName, target, method, flags); } - removeFromListeners(eventName: string, target: any, method: Function | string): void { - let pointer: Meta | null = this; - while (pointer !== null) { - let listeners = pointer._listeners; - if (listeners !== undefined) { - for (let index = listeners.length - 4; index >= 0; index -= 4) { - if ( - listeners[index] === eventName && - (!method || (listeners[index + 1] === target && listeners[index + 2] === method)) - ) { - if (pointer === this) { - listeners.splice(index, 4); // we are modifying our own list, so we edit directly - } else { - // we are trying to remove an inherited listener, so we do - // just-in-time copying to detach our own listeners from - // our inheritance chain. - this._finalizeListeners(); - return this.removeFromListeners(eventName, target, method); - } - } - } - } - if (pointer._listenersFinalized) { - break; - } - pointer = pointer.parent; - } + removeFromListeners(eventName: string, target: any, method?: Function | string): void { + this._addToListeners( + eventName, + target, + method === undefined ? ALL : method, + ListenerFlags.REMOVE + ); } - matchingListeners(eventName: string) { - let pointer: Meta | null = this; - // fix type - let result: any[] | undefined; - while (pointer !== null) { - let listeners = pointer._listeners; - if (listeners !== undefined) { - for (let index = 0; index < listeners.length; index += 4) { - if (listeners[index] === eventName) { - result = result || []; - pushUniqueListener(result, listeners, index); + matchingListeners(eventName: string): PublicListeners | undefined { + let { parent } = this; + let result = parent === null ? undefined : parent.matchingListeners(eventName); + let { _listeners: listeners } = this; + + if (listeners !== undefined) { + for (let index = 0; index < listeners.length; index += 4) { + if (listeners[index] === eventName) { + let target = listeners[index + 1] as object | null; + let method = listeners[index + 2] as Function | string; + let flags = listeners[index + 3] as ListenerFlags; + if ((flags & ListenerFlags.REMOVE) === ListenerFlags.REMOVE) { + if (result !== undefined) { + if (method === ALL) { + result = undefined; + } else { + removeListener(result, target, method); + } + } + } else { + if (result === undefined) { + result = [] as PublicListeners; + } + pushUniqueListener( + result, + target, + method, + (flags & ListenerFlags.ONCE) === ListenerFlags.ONCE + ); } } } - if (pointer._listenersFinalized) { - break; - } - pointer = pointer.parent; } - return result; + return result !== undefined && result.length === 0 ? undefined : result; } } @@ -811,13 +806,29 @@ export { counters }; allocations, without even bothering to do deduplication -- we can save that for dispatch time, if an event actually happens. */ -function pushUniqueListener(destination: any[], source: any[], index: number) { - let target = source[index + 1]; - let method = source[index + 2]; - for (let destinationIndex = 0; destinationIndex < destination.length; destinationIndex += 3) { - if (destination[destinationIndex] === target && destination[destinationIndex + 1] === method) { +function pushUniqueListener( + destination: PublicListeners, + target: object | null, + method: Function | string, + once: boolean +) { + for (let index = 0; index < destination.length; index += 3) { + if (destination[index] === target && destination[index + 1] === method) { return; } } - destination.push(target, method, source[index + 3]); + destination.push(target, method, once); +} + +function removeListener( + destination: PublicListeners, + target: object | null, + method: Function | string +) { + for (let index = 0; index < destination.length; index += 3) { + if (destination[index] === target && destination[index + 1] === method) { + destination.splice(index, 3); + break; + } + } } diff --git a/packages/ember-metal/tests/events_test.js b/packages/ember-metal/tests/events_test.js index 47553698171..624a32d2915 100644 --- a/packages/ember-metal/tests/events_test.js +++ b/packages/ember-metal/tests/events_test.js @@ -26,15 +26,18 @@ moduleFor( } ['@test listeners should be inherited'](assert) { - let obj = {}; + class A {} + class B extends A {} + let count = 0; let F = function() { count++; }; - addListener(obj, 'event!', F); + addListener(A.prototype, 'event!', F); - let obj2 = Object.create(obj); + let obj = new A(); + let obj2 = new B(); assert.equal(count, 0, 'nothing yet');