From 424db11bf2fb6fcc06124f89e4e5c7bf1b8b3fa2 Mon Sep 17 00:00:00 2001 From: Yun Lai Date: Sat, 31 Oct 2015 16:26:09 +1100 Subject: [PATCH 1/8] still something wrong --- package.json | 2 +- src/reactor.js | 19 +++---- src/reactor/fns.js | 125 +++++++++++++++++++++++++++-------------- src/reactor/records.js | 8 ++- 4 files changed, 98 insertions(+), 56 deletions(-) diff --git a/package.json b/package.json index 0cf02f5..123671e 100644 --- a/package.json +++ b/package.json @@ -2,7 +2,7 @@ "name": "nuclear-js", "version": "1.1.2", "description": "Immutable, reactive Flux architecture. UI Agnostic.", - "main": "dist/nuclear.js", + "main": "src/main.js", "scripts": { "test": "grunt ci" }, diff --git a/src/reactor.js b/src/reactor.js index b31cb21..6e0cf49 100644 --- a/src/reactor.js +++ b/src/reactor.js @@ -195,7 +195,7 @@ class Reactor { return } - let observerIdsToNotify = Immutable.Set().withMutations(set => { + let gettersToNotify = Immutable.Set().withMutations(set => { // notify all observers set.union(this.observerState.get('any')) @@ -208,15 +208,8 @@ class Reactor { }) }) - 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((getterId) => { + const getter = this.observerState.get('getters')[getterId]; const prevEvaluateResult = fns.evaluate(this.prevReactorState, getter) const currEvaluateResult = fns.evaluate(this.reactorState, getter) @@ -225,7 +218,11 @@ class Reactor { const currValue = currEvaluateResult.result if (!Immutable.is(prevValue, currValue)) { - handler.call(null, currValue) + const handlers = this.observerState.getIn(['gettersMap', getterId]) + .map(observerId => this.observerState.getIn(['observersMap', observerId, 'handler'])) + // don't notify here in the case a handler called unobserve on another observer + + handlers.forEach(handler => handler.call(null, currValue)) } }) diff --git a/src/reactor/fns.js b/src/reactor/fns.js index 4afea7d..ce49937 100644 --- a/src/reactor/fns.js +++ b/src/reactor/fns.js @@ -156,45 +156,73 @@ exports.loadState = function(reactorState, state) { */ exports.addObserver = function(observerState, getter, handler) { // use the passed in getter as the key so we can rely on a byreference call for unobserve - const getterKey = getter - if (isKeyPath(getter)) { - getter = fromKeyPath(getter) - } + try { + const getterKey = getter + if (isKeyPath(getter)) { + getter = fromKeyPath(getter) + } - const currId = observerState.get('nextId') - const storeDeps = getStoreDeps(getter) - const entry = Immutable.Map({ - id: currId, - storeDeps: storeDeps, - getterKey: getterKey, - getter: getter, - handler: handler, - }) + const currId = observerState.get('nextId') + const storeDeps = getStoreDeps(getter) + const entry = Immutable.Map({ + id: currId, + storeDeps: storeDeps, + getterKey: getterKey, + getter: getter, + handler: handler, + }) - let updatedObserverState - 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)) - } else { - updatedObserverState = observerState.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)) + let updatedObserverState + + let existingGetters = observerState.get('getters'); + + let getterId = existingGetters.indexOf(getter); + + if (getterId < 0) { + existingGetters.push(getter); + getterId = existingGetters.length - 1; + } + //update getterMap + + let observerIdsForGetter = observerState.getIn(['gettersMap', getterId]) + + if (!observerIdsForGetter) { + observerIdsForGetter = Immutable.Set([]) + } + + observerIdsForGetter = observerIdsForGetter.add(currId); + + updatedObserverState = observerState.setIn(['gettersMap', getterId], observerIdsForGetter); + + if (storeDeps.size === 0) { + // no storeDeps means the observer is dependent on any of the state changing + + updatedObserverState = updatedObserverState.updateIn(['any'], getters => getters.add(getterId)) + } else { + updatedObserverState = updatedObserverState.withMutations(map => { + storeDeps.forEach(storeId => { + let path = ['stores', storeId] + if (!map.hasIn(path)) { + map.setIn(path, Immutable.Set([])) + } + map.updateIn(['stores', storeId], getters => getters.add(getterId)) + }) }) - }) - } + } - updatedObserverState = updatedObserverState - .set('nextId', currId + 1) - .setIn(['observersMap', currId], entry) + updatedObserverState = updatedObserverState + .set('nextId', currId + 1) + .setIn(['observersMap', currId], entry) - return { - observerState: updatedObserverState, - entry: entry, + return { + observerState: updatedObserverState, + entry: entry, + } + } catch (e) { + debugger; } + + } /** @@ -239,18 +267,31 @@ exports.removeObserver = function(observerState, getter, handler) { */ exports.removeObserverByEntry = function(observerState, entry) { return observerState.withMutations(map => { - const id = entry.get('id') - const storeDeps = entry.get('storeDeps') + try { + 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 => observers.remove(id)) - }) + const existingGetters = observerState.get('getters'); + + const getterId = existingGetters.indexOf(getter); + + //cleaning the gettersMap + map.updateIn(['gettersMap', getterId], observerIds => observerIds.remove(id)); + + if (storeDeps.size === 0 && map.getIn(['gettersMap', getterId]).size === 0) { + map.update('any', anyGetters => anyGetters.remove(getterId)) + } else { + storeDeps.forEach(storeId => { + map.updateIn(['stores', storeId], getters => getters.remove(getterId)) + }) + } + + map.removeIn(['observersMap', id]) + } catch (e) { + debugger; } - map.removeIn(['observersMap', id]) }) } diff --git a/src/reactor/records.js b/src/reactor/records.js index 33aaff9..4018941 100644 --- a/src/reactor/records.js +++ b/src/reactor/records.js @@ -12,11 +12,15 @@ 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({}), + getters: [], + + gettersMap: Immutable.Map({}), + observersMap: Immutable.Map({}), nextId: 1, From c1dc8c4962a449d24fa88a69e3cb884f274870a9 Mon Sep 17 00:00:00 2001 From: Yun Lai Date: Sun, 1 Nov 2015 00:05:44 +1100 Subject: [PATCH 2/8] fixed the problem. now can use to compare with the previous one --- src/reactor.js | 6 ++-- src/reactor/fns.js | 67 ++++++++++++++++-------------------------- src/reactor/records.js | 2 -- 3 files changed, 27 insertions(+), 48 deletions(-) diff --git a/src/reactor.js b/src/reactor.js index 6e0cf49..224f8d4 100644 --- a/src/reactor.js +++ b/src/reactor.js @@ -208,9 +208,7 @@ class Reactor { }) }) - gettersToNotify.forEach((getterId) => { - const getter = this.observerState.get('getters')[getterId]; - + gettersToNotify.forEach(getter => { const prevEvaluateResult = fns.evaluate(this.prevReactorState, getter) const currEvaluateResult = fns.evaluate(this.reactorState, getter) @@ -218,7 +216,7 @@ class Reactor { const currValue = currEvaluateResult.result if (!Immutable.is(prevValue, currValue)) { - const handlers = this.observerState.getIn(['gettersMap', getterId]) + const handlers = this.observerState.getIn(['gettersMap', getter]) .map(observerId => this.observerState.getIn(['observersMap', observerId, 'handler'])) // don't notify here in the case a handler called unobserve on another observer diff --git a/src/reactor/fns.js b/src/reactor/fns.js index ce49937..f4f9baf 100644 --- a/src/reactor/fns.js +++ b/src/reactor/fns.js @@ -172,40 +172,26 @@ exports.addObserver = function(observerState, getter, handler) { handler: handler, }) - let updatedObserverState - - let existingGetters = observerState.get('getters'); - - let getterId = existingGetters.indexOf(getter); - - if (getterId < 0) { - existingGetters.push(getter); - getterId = existingGetters.length - 1; - } - //update getterMap - - let observerIdsForGetter = observerState.getIn(['gettersMap', getterId]) - - if (!observerIdsForGetter) { - observerIdsForGetter = Immutable.Set([]) - } - - observerIdsForGetter = observerIdsForGetter.add(currId); - - updatedObserverState = observerState.setIn(['gettersMap', getterId], observerIdsForGetter); + let updatedObserverState = observerState.updateIn(['gettersMap', getter] + , observerIds => + observerIds + ? observerIds.add(currId) + : Immutable.Set([]).add(currId) + ) if (storeDeps.size === 0) { // no storeDeps means the observer is dependent on any of the state changing - updatedObserverState = updatedObserverState.updateIn(['any'], getters => getters.add(getterId)) + updatedObserverState = updatedObserverState.updateIn(['any'], getters => getters.add(getter)) } else { updatedObserverState = updatedObserverState.withMutations(map => { storeDeps.forEach(storeId => { - let path = ['stores', storeId] - if (!map.hasIn(path)) { - map.setIn(path, Immutable.Set([])) - } - map.updateIn(['stores', storeId], getters => getters.add(getterId)) + map.updateIn(['stores', storeId] + , getters => + getters + ? getters.add(getter) + : Immutable.Set([]).add(getter) + ) }) }) } @@ -267,31 +253,28 @@ exports.removeObserver = function(observerState, getter, handler) { */ exports.removeObserverByEntry = function(observerState, entry) { return observerState.withMutations(map => { - try { - const id = entry.get('id') - const getter = entry.get('getter') - const storeDeps = entry.get('storeDeps') - - const existingGetters = observerState.get('getters'); + const id = entry.get('id') + const getter = entry.get('getter') + const storeDeps = entry.get('storeDeps') - const getterId = existingGetters.indexOf(getter); + map.updateIn(['gettersMap', getter], observerIds => observerIds.remove(id)); - //cleaning the gettersMap - map.updateIn(['gettersMap', getterId], observerIds => observerIds.remove(id)); + if (map.getIn(['gettersMap', getter]).size <= 0) { - if (storeDeps.size === 0 && map.getIn(['gettersMap', getterId]).size === 0) { - map.update('any', anyGetters => anyGetters.remove(getterId)) + 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 => getters.remove(getterId)) + map.updateIn(['stores', storeId] + , getters => getters.remove(getter) ) }) } - map.removeIn(['observersMap', id]) - } catch (e) { - debugger; } + map.removeIn(['observersMap', id]) + }) } diff --git a/src/reactor/records.js b/src/reactor/records.js index 4018941..fe5903f 100644 --- a/src/reactor/records.js +++ b/src/reactor/records.js @@ -17,8 +17,6 @@ const ObserverState = Immutable.Record({ // getters registered to specific store changes stores: Immutable.Map({}), - getters: [], - gettersMap: Immutable.Map({}), observersMap: Immutable.Map({}), From f77202cc68814c5b9e99d68dea55b4b3506e598d Mon Sep 17 00:00:00 2001 From: Yun Lai Date: Sun, 1 Nov 2015 16:53:40 +1100 Subject: [PATCH 3/8] caching fix for notify. --- src/reactor.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/reactor.js b/src/reactor.js index 224f8d4..ce05b0e 100644 --- a/src/reactor.js +++ b/src/reactor.js @@ -212,6 +212,9 @@ class Reactor { const prevEvaluateResult = fns.evaluate(this.prevReactorState, getter) const currEvaluateResult = fns.evaluate(this.reactorState, getter) + this.prevReactorState = prevEvaluateResult.reactorState + this.reactorState = currEvaluateResult.reactorState + const prevValue = prevEvaluateResult.result const currValue = currEvaluateResult.result From d38b04e2fe29736ca0ad2bcaf73d440cc0514c2b Mon Sep 17 00:00:00 2001 From: Yun Lai Date: Sun, 1 Nov 2015 16:58:44 +1100 Subject: [PATCH 4/8] restore the package.json --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 123671e..0cf02f5 100644 --- a/package.json +++ b/package.json @@ -2,7 +2,7 @@ "name": "nuclear-js", "version": "1.1.2", "description": "Immutable, reactive Flux architecture. UI Agnostic.", - "main": "src/main.js", + "main": "dist/nuclear.js", "scripts": { "test": "grunt ci" }, From fe88c19d36268132c6cf3fe2c3744d461db8a970 Mon Sep 17 00:00:00 2001 From: Yun Lai Date: Sun, 1 Nov 2015 17:13:18 +1100 Subject: [PATCH 5/8] remove the testing bit. --- src/reactor/fns.js | 85 ++++++++++++++++++++++------------------------ 1 file changed, 40 insertions(+), 45 deletions(-) diff --git a/src/reactor/fns.js b/src/reactor/fns.js index f4f9baf..b25f390 100644 --- a/src/reactor/fns.js +++ b/src/reactor/fns.js @@ -156,59 +156,54 @@ exports.loadState = function(reactorState, state) { */ exports.addObserver = function(observerState, getter, handler) { // use the passed in getter as the key so we can rely on a byreference call for unobserve - try { - const getterKey = getter - if (isKeyPath(getter)) { - getter = fromKeyPath(getter) - } - - const currId = observerState.get('nextId') - const storeDeps = getStoreDeps(getter) - const entry = Immutable.Map({ - id: currId, - storeDeps: storeDeps, - getterKey: getterKey, - getter: getter, - handler: handler, - }) + const getterKey = getter + if (isKeyPath(getter)) { + getter = fromKeyPath(getter) + } - let updatedObserverState = observerState.updateIn(['gettersMap', getter] - , observerIds => - observerIds - ? observerIds.add(currId) - : Immutable.Set([]).add(currId) - ) + const currId = observerState.get('nextId') + const storeDeps = getStoreDeps(getter) + const entry = Immutable.Map({ + id: currId, + storeDeps: storeDeps, + getterKey: getterKey, + getter: getter, + handler: handler, + }) - if (storeDeps.size === 0) { - // no storeDeps means the observer is dependent on any of the state changing + let updatedObserverState = observerState.updateIn(['gettersMap', getter] + , observerIds => + observerIds + ? observerIds.add(currId) + : Immutable.Set([]).add(currId) + ) - updatedObserverState = updatedObserverState.updateIn(['any'], getters => getters.add(getter)) - } else { - updatedObserverState = updatedObserverState.withMutations(map => { - storeDeps.forEach(storeId => { - map.updateIn(['stores', storeId] - , getters => - getters - ? getters.add(getter) - : Immutable.Set([]).add(getter) - ) - }) + if (storeDeps.size === 0) { + // no storeDeps means the observer is dependent on any of the state changing + + updatedObserverState = updatedObserverState.updateIn(['any'], getters => getters.add(getter)) + } else { + updatedObserverState = updatedObserverState.withMutations(map => { + storeDeps.forEach(storeId => { + map.updateIn(['stores', storeId] + , getters => + getters + ? getters.add(getter) + : Immutable.Set([]).add(getter) + ) }) - } + }) + } - updatedObserverState = updatedObserverState - .set('nextId', currId + 1) - .setIn(['observersMap', currId], entry) + updatedObserverState = updatedObserverState + .set('nextId', currId + 1) + .setIn(['observersMap', currId], entry) - return { - observerState: updatedObserverState, - entry: entry, - } - } catch (e) { - debugger; + return { + observerState: updatedObserverState, + entry: entry, } - } /** From b75d528224ebcb1276adb251ebc56cef92fc036d Mon Sep 17 00:00:00 2001 From: Yun Lai Date: Mon, 2 Nov 2015 07:43:02 +1100 Subject: [PATCH 6/8] tests passed --- tests/reactor-fns-tests.js | 38 ++++++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/tests/reactor-fns-tests.js b/tests/reactor-fns-tests.js index 8d8bf08..de1950f 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, From eb162a1cecf3bbd6f0bc3e16cf9faafe3541c0d8 Mon Sep 17 00:00:00 2001 From: Yun Lai Date: Mon, 2 Nov 2015 07:44:37 +1100 Subject: [PATCH 7/8] solve the formatting issue --- src/reactor/fns.js | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/reactor/fns.js b/src/reactor/fns.js index b25f390..4ded614 100644 --- a/src/reactor/fns.js +++ b/src/reactor/fns.js @@ -171,12 +171,9 @@ exports.addObserver = function(observerState, getter, handler) { handler: handler, }) - let updatedObserverState = observerState.updateIn(['gettersMap', getter] - , observerIds => - observerIds - ? observerIds.add(currId) - : Immutable.Set([]).add(currId) - ) + 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 @@ -185,12 +182,9 @@ exports.addObserver = function(observerState, getter, handler) { } else { updatedObserverState = updatedObserverState.withMutations(map => { storeDeps.forEach(storeId => { - map.updateIn(['stores', storeId] - , getters => - getters - ? getters.add(getter) - : Immutable.Set([]).add(getter) - ) + map.updateIn(['stores', storeId], getters => { + return getters ? getters.add(getter) : Immutable.Set.of(getter) + }) }) }) } From 22bedfba0b9ee481487489908e53503ecbc9842e Mon Sep 17 00:00:00 2001 From: Yun Lai Date: Mon, 2 Nov 2015 08:38:52 +1100 Subject: [PATCH 8/8] cover the test case where removing a getter with handler, if there is still handler for the getter, do not remove the reference in store. --- tests/reactor-fns-tests.js | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/reactor-fns-tests.js b/tests/reactor-fns-tests.js index de1950f..959e825 100644 --- a/tests/reactor-fns-tests.js +++ b/tests/reactor-fns-tests.js @@ -518,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*/