diff --git a/src/reactor.js b/src/reactor.js index 0efd995..74d6c47 100644 --- a/src/reactor.js +++ b/src/reactor.js @@ -195,29 +195,19 @@ class Reactor { return } - let observerIdsToNotify = Immutable.Set().withMutations(set => { + let gettersToNotify = Immutable.Set().withMutations(set => { // notify all observers set.union(this.observerState.get('any')) dirtyStores.forEach(id => { const entries = this.observerState.getIn(['stores', id]) - if (!entries) { - return + if (entries) { + set.union(entries) } - set.union(entries) }) }) - observerIdsToNotify.forEach((observerId) => { - const entry = this.observerState.getIn(['observersMap', observerId]) - if (!entry) { - // don't notify here in the case a handler called unobserve on another observer - return - } - - const getter = entry.get('getter') - const handler = entry.get('handler') - + gettersToNotify.forEach(getter => { const prevEvaluateResult = fns.evaluate(this.prevReactorState, getter) const currEvaluateResult = fns.evaluate(this.reactorState, getter) @@ -228,7 +218,14 @@ class Reactor { const currValue = currEvaluateResult.result if (!Immutable.is(prevValue, currValue)) { - handler.call(null, currValue) + const observerIds = this.observerState.getIn(['gettersMap', getter], []) + observerIds.forEach(observerId => { + const handler = this.observerState.getIn(['observersMap', observerId, 'handler']) + // don't notify here in the case a handler called unobserve on another observer + if (handler) { + handler.call(null, currValue) + } + }) } }) diff --git a/src/reactor/fns.js b/src/reactor/fns.js index 05f0fe5..6fe3a92 100644 --- a/src/reactor/fns.js +++ b/src/reactor/fns.js @@ -171,18 +171,19 @@ exports.addObserver = function(observerState, getter, handler) { handler: handler, }) - let updatedObserverState + let updatedObserverState = observerState.updateIn(['gettersMap', getter], observerIds => { + return observerIds ? observerIds.add(currId) : Immutable.Set.of(currId) + }) + if (storeDeps.size === 0) { // no storeDeps means the observer is dependent on any of the state changing - updatedObserverState = observerState.update('any', observerIds => observerIds.add(currId)) + updatedObserverState = updatedObserverState.updateIn(['any'], getters => getters.add(getter)) } else { - updatedObserverState = observerState.withMutations(map => { + updatedObserverState = updatedObserverState.withMutations(map => { storeDeps.forEach(storeId => { - let path = ['stores', storeId] - if (!map.hasIn(path)) { - map.setIn(path, Immutable.Set([])) - } - map.updateIn(['stores', storeId], observerIds => observerIds.add(currId)) + map.updateIn(['stores', storeId], getters => { + return getters ? getters.add(getter) : Immutable.Set.of(getter) + }) }) }) } @@ -195,6 +196,7 @@ exports.addObserver = function(observerState, getter, handler) { observerState: updatedObserverState, entry: entry, } + } /** @@ -240,20 +242,34 @@ exports.removeObserver = function(observerState, getter, handler) { exports.removeObserverByEntry = function(observerState, entry) { return observerState.withMutations(map => { const id = entry.get('id') + const getter = entry.get('getter') const storeDeps = entry.get('storeDeps') - if (storeDeps.size === 0) { - map.update('any', anyObsevers => anyObsevers.remove(id)) - } else { - storeDeps.forEach(storeId => { - map.updateIn(['stores', storeId], observers => { - if (observers) { - // check for observers being present because reactor.reset() can be called before an unwatch fn - return observers.remove(id) - } - return observers + const observerIds = map.getIn(['gettersMap', getter]) + if (!observerIds) { + // getter doesn't exist if reactor.reset() is called before the unwatchFn() + return + } + const updatedObserverIds = observerIds.remove(id) + map.setIn(['gettersMap', getter], updatedObserverIds) + + if (updatedObserverIds.size === 0) { + // all observers have been removed for this getter, remove other entries + if (storeDeps.size === 0) { + // no storeDeps means the observer is dependent on any of the state changing + map.update('any', getters => getters.remove(getter)); + } else { + storeDeps.forEach(storeId => { + map.updateIn(['stores', storeId], getters => { + if (getters) { + // check to make sure the getters Set exists for this store, + // in the case of reactor.reset() is called before the unwatchFn() + return getters.remove(getter) + } + return getters + }) }) - }) + } } map.removeIn(['observersMap', id]) diff --git a/src/reactor/records.js b/src/reactor/records.js index 33aaff9..fe5903f 100644 --- a/src/reactor/records.js +++ b/src/reactor/records.js @@ -12,11 +12,13 @@ const ReactorState = Immutable.Record({ }) const ObserverState = Immutable.Record({ - // observers registered to any store change + // getters registered to any store change any: Immutable.Set([]), - // observers registered to specific store changes + // getters registered to specific store changes stores: Immutable.Map({}), + gettersMap: Immutable.Map({}), + observersMap: Immutable.Map({}), nextId: 1, diff --git a/tests/reactor-fns-tests.js b/tests/reactor-fns-tests.js index 8d8bf08..959e825 100644 --- a/tests/reactor-fns-tests.js +++ b/tests/reactor-fns-tests.js @@ -321,12 +321,12 @@ describe('reactor fns', () => { entry = result.entry }) - it('should update the "any" observers', () => { - const expected = Set.of(1) + it('should update the "any" with getter reference', () => { + const expected = Set.of(getter) const result = nextObserverState.get('any') expect(is(expected, result)).toBe(true) }) - it('should not update the "store" observers', () => { + it('should not update the "store" with getter reference', () => { const expected = Map({}) const result = nextObserverState.get('stores') expect(is(expected, result)).toBe(true) @@ -336,6 +336,11 @@ describe('reactor fns', () => { const result = nextObserverState.get('nextId') expect(is(expected, result)).toBe(true) }) + it('should update the gettersMap with getter as ref, id as value', () => { + const expected = Set.of(1) + const result = nextObserverState.getIn(['gettersMap', getter]) + expect(is(expected, result)).toBe(true) + }) it('should update the observerMap', () => { const expected = Map([ [1, Map({ @@ -375,20 +380,25 @@ describe('reactor fns', () => { nextObserverState = result.observerState entry = result.entry }) - it('should not update the "any" observers', () => { + it('should not update the "any" getters', () => { const expected = Set.of() const result = nextObserverState.get('any') expect(is(expected, result)).toBe(true) }) - it('should not update the "store" observers', () => { + it('should update the "store" with getter reference', () => { const expected = Map({ - store1: Set.of(1), - store2: Set.of(1), + store1: Set.of(getter), + store2: Set.of(getter), }) const result = nextObserverState.get('stores') expect(is(expected, result)).toBe(true) }) + it('should update the gettersMap with getter as ref, id as value', () => { + const expected = Set.of(1) + const result = nextObserverState.getIn(['gettersMap', getter]) + expect(is(expected, result)).toBe(true) + }) it('should increment the nextId', () => { const expected = 2 const result = nextObserverState.get('nextId') @@ -448,12 +458,16 @@ describe('reactor fns', () => { it('should return a new ObserverState with all entries containing the getter removed', () => { nextObserverState = fns.removeObserver(initialObserverState, getter1) const expected = Map({ - any: Set.of(3), + any: Set.of(getter2), stores: Map({ store1: Set(), store2: Set(), }), nextId: 4, + gettersMap: Map([ + [getter1, Set()], + [getter2, Set.of(3)] + ]), observersMap: Map([ [3, Map({ id: 3, @@ -475,10 +489,14 @@ describe('reactor fns', () => { const expected = Map({ any: Set(), stores: Map({ - store1: Set.of(1, 2), - store2: Set.of(1, 2), + store1: Set.of(getter1), + store2: Set.of(getter1), }), nextId: 4, + gettersMap: Map([ + [getter1, Set.of(1, 2)], + [getter2, Set()] + ]), observersMap: Map([ [1, Map({ id: 1, @@ -500,6 +518,40 @@ describe('reactor fns', () => { expect(is(expected, result)).toBe(true) }) }) + + it('should not remove the getter reference in store when there is still listeners for the getter', () => { + nextObserverState = fns.removeObserver(initialObserverState, getter1, handler2) + const expected = Map({ + any: Set.of(getter2), + stores: Map({ + store1: Set.of(getter1), + store2: Set.of(getter1), + }), + nextId: 4, + gettersMap: Map([ + [getter1, Set.of(1)], + [getter2, Set.of(3)] + ]), + observersMap: Map([ + [1, Map({ + id: 1, + storeDeps: Set.of('store1', 'store2'), + getterKey: getter1, + getter: getter1, + handler: handler1, + })], + [3, Map({ + id: 3, + storeDeps: Set(), + getterKey: getter2, + getter: getter2, + handler: handler3, + })] + ]) + }) + const result = nextObserverState + expect(is(expected, result)).toBe(true) + }) }) }) /*eslint-enable one-var, comma-dangle*/