Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

grouping observers by getters in addObserver #183

Open
wants to merge 9 commits into
base: jordan/refactor-everything!
Choose a base branch
from
19 changes: 7 additions & 12 deletions src/reactor.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'))

Expand All @@ -208,16 +208,7 @@ 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(getter => {
const prevEvaluateResult = fns.evaluate(this.prevReactorState, getter)
const currEvaluateResult = fns.evaluate(this.reactorState, getter)

Expand All @@ -228,7 +219,11 @@ class Reactor {
const currValue = currEvaluateResult.result

if (!Immutable.is(prevValue, currValue)) {
handler.call(null, currValue)
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

handlers.forEach(handler => handler.call(null, currValue))
}
})

Expand Down
51 changes: 32 additions & 19 deletions src/reactor/fns.js
Original file line number Diff line number Diff line change
Expand Up @@ -171,18 +171,26 @@ exports.addObserver = function(observerState, getter, handler) {
handler: handler,
})

let updatedObserverState
let updatedObserverState = observerState.updateIn(['gettersMap', getter]
Copy link
Contributor

Choose a reason for hiding this comment

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

please use the style:

let updatedObserverState = observerState.updateIn(['gettersMap', getter], observerIds => {
  return observerIds ? observerIds.add(currId) : Immutable.Set.of(currId)
})

, 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 = 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 =>
getters
? getters.add(getter)
: Immutable.Set([]).add(getter)
)
})
})
}
Expand All @@ -195,6 +203,7 @@ exports.addObserver = function(observerState, getter, handler) {
observerState: updatedObserverState,
entry: entry,
}

}

/**
Expand Down Expand Up @@ -240,23 +249,27 @@ 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
map.updateIn(['gettersMap', getter], observerIds => observerIds.remove(id));

if (map.getIn(['gettersMap', getter]).size <= 0) {

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]
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont think this is correct.

If we observe add two observer entries with the same getter and then unobserve one it will unobserve all entries with that getter.

Copy link
Author

Choose a reason for hiding this comment

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

that's why it's under the condition, only removed when there's no entries listening to that getter

if (map.getIn(['gettersMap', getter]).size <= 0) 

Copy link
Author

Choose a reason for hiding this comment

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

I can add a test case to cover it if you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! This looks good.

, getters => getters.remove(getter) )
})
})
}

}

map.removeIn(['observersMap', id])

})
}

Expand Down
6 changes: 4 additions & 2 deletions src/reactor/records.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down