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.

Sign up for GitHub

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

avoid useless calls to listeners #1626

Closed
wants to merge 7 commits into from
Closed

avoid useless calls to listeners #1626

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2204c6e
avoid useless calls to listeners when they've already been notified b…
splendido Apr 16, 2016
4dd056c
consider useless notification pruning during nested dispatch
splendido Apr 17, 2016
cb2def2
call listener one last time within unsubscribe
splendido May 1, 2016
b4ddb6e
consider last call to listener within unsubscribe
splendido May 1, 2016
2ec2d44
Merge branch 'master' into avoid-useless-notifications
splendido May 1, 2016
4f08d20
consider last call to listener within unsubscribe
splendido May 1, 2016
fa52dc9
Merge branch 'master' into avoid-useless-notifications
splendido May 10, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/createStore.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export default function createStore(reducer, initialState, enhancer) {
var currentListeners = []
var nextListeners = currentListeners
var isDispatching = false
var needToNotify = true

function ensureCanMutateNextListeners() {
if (nextListeners === currentListeners) {
Expand Down Expand Up @@ -115,6 +116,8 @@ export default function createStore(reducer, initialState, enhancer) {

isSubscribed = false

listener()
Copy link
Member

Choose a reason for hiding this comment

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

Wait, why are you calling the listener on unsubscribe? That's unexpected behavior.

Copy link
Author

@splendido splendido Jul 24, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@timdorr what do you mean with unexpected behaviour?
This is not different from what is already happening: the listener is probably being called with a state which was already notified to him and, as you pointed out this is not a problem at all!
But with the other changes in place, this final call is needed to ensure that that listener is always notified with all state changes happened before its unsubscription.

It's not unexpexted behaviour at all ;-)

Edit: the only edge case could be when a listener is registered and soon after unregistered before any state change could happen. In this case I agree with you: the listener is being called even if the state did not change.

Copy link
Member

Choose a reason for hiding this comment

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

This listener call is entirely unrelated to any state change. It can only be called outside of any dispatch (this is now enforced via #1569). It is impossible to have a state change where the listener is not called.

This is basically creating exactly the thing this PR looks to avoid: an unnecessary listener call.


ensureCanMutateNextListeners()
var index = nextListeners.indexOf(listener)
nextListeners.splice(index, 1)
Expand Down Expand Up @@ -172,10 +175,12 @@ export default function createStore(reducer, initialState, enhancer) {
isDispatching = false
}

needToNotify = true
var listeners = currentListeners = nextListeners
for (var i = 0; i < listeners.length; i++) {
for (var i = 0; needToNotify && i < listeners.length; i++) {
listeners[i]()
}
needToNotify = false

return action
}
Expand Down
139 changes: 82 additions & 57 deletions test/createStore.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,28 +213,39 @@ describe('createStore', () => {
expect(listenerB.calls.length).toBe(1)

unsubscribeA()
expect(listenerA.calls.length).toBe(3)
expect(listenerA.calls.length).toBe(4)
expect(listenerB.calls.length).toBe(1)

store.dispatch(unknownAction())
expect(listenerA.calls.length).toBe(3)
expect(listenerA.calls.length).toBe(4)
expect(listenerB.calls.length).toBe(2)

unsubscribeB()
expect(listenerA.calls.length).toBe(3)
expect(listenerB.calls.length).toBe(2)
expect(listenerA.calls.length).toBe(4)
expect(listenerB.calls.length).toBe(3)

store.dispatch(unknownAction())
expect(listenerA.calls.length).toBe(3)
expect(listenerB.calls.length).toBe(2)
expect(listenerA.calls.length).toBe(4)
expect(listenerB.calls.length).toBe(3)

unsubscribeA = store.subscribe(listenerA)
expect(listenerA.calls.length).toBe(3)
expect(listenerB.calls.length).toBe(2)
expect(listenerA.calls.length).toBe(4)
expect(listenerB.calls.length).toBe(3)

store.dispatch(unknownAction())
expect(listenerA.calls.length).toBe(4)
expect(listenerB.calls.length).toBe(2)
expect(listenerA.calls.length).toBe(5)
expect(listenerB.calls.length).toBe(3)
})

it('calls listener when unsubscribe is called', () => {
const store = createStore(reducers.todos)
const listener = expect.createSpy(() => {})

const unsubscribe = store.subscribe(listener)
expect(listener.calls.length).toBe(0)

unsubscribe()
expect(listener.calls.length).toBe(1)
})

it('only removes listener once when unsubscribe is called', () => {
Expand All @@ -247,24 +258,30 @@ describe('createStore', () => {

unsubscribeA()
unsubscribeA()
expect(listenerA.calls.length).toBe(1)
expect(listenerB.calls.length).toBe(0)

store.dispatch(unknownAction())
expect(listenerA.calls.length).toBe(0)
expect(listenerA.calls.length).toBe(1)
expect(listenerB.calls.length).toBe(1)
})

it('only removes relevant listener when unsubscribe is called', () => {
const store = createStore(reducers.todos)
const listener = expect.createSpy(() => {})
const listenerA = expect.createSpy(() => {})
const listenerB = expect.createSpy(() => {})

store.subscribe(listener)
const unsubscribeSecond = store.subscribe(listener)
store.subscribe(listenerA)
const unsubscribeB = store.subscribe(listenerB)

unsubscribeSecond()
unsubscribeSecond()
unsubscribeB()
unsubscribeB()
expect(listenerA.calls.length).toBe(0)
expect(listenerB.calls.length).toBe(1)

store.dispatch(unknownAction())
expect(listener.calls.length).toBe(1)
expect(listenerA.calls.length).toBe(1)
expect(listenerB.calls.length).toBe(1)
})

it('supports removing a subscription within a subscription', () => {
Expand All @@ -281,10 +298,13 @@ describe('createStore', () => {
store.subscribe(listenerC)

store.dispatch(unknownAction())
store.dispatch(unknownAction())
expect(listenerA.calls.length).toBe(1)
expect(listenerB.calls.length).toBe(2)
expect(listenerC.calls.length).toBe(1)

store.dispatch(unknownAction())
expect(listenerA.calls.length).toBe(2)
expect(listenerB.calls.length).toBe(1)
expect(listenerB.calls.length).toBe(2)
expect(listenerC.calls.length).toBe(2)
})

Expand All @@ -297,25 +317,24 @@ describe('createStore', () => {
)

const listener1 = expect.createSpy(() => {})
const listener2 = expect.createSpy(() => {})
const listener2 = expect.createSpy(() => {}).andCall(() => {
doUnsubscribeAll()
})
const listener3 = expect.createSpy(() => {})

unsubscribeHandles.push(store.subscribe(() => listener1()))
unsubscribeHandles.push(store.subscribe(() => {
listener2()
doUnsubscribeAll()
}))
unsubscribeHandles.push(store.subscribe(() => listener3()))
unsubscribeHandles.push(store.subscribe(listener1))
unsubscribeHandles.push(store.subscribe(listener2))
unsubscribeHandles.push(store.subscribe(listener3))

store.dispatch(unknownAction())
expect(listener1.calls.length).toBe(1)
expect(listener2.calls.length).toBe(1)
expect(listener3.calls.length).toBe(1)
expect(listener1.calls.length).toBe(2)
expect(listener2.calls.length).toBe(2)
expect(listener3.calls.length).toBe(2)

store.dispatch(unknownAction())
expect(listener1.calls.length).toBe(1)
expect(listener2.calls.length).toBe(1)
expect(listener3.calls.length).toBe(1)
expect(listener1.calls.length).toBe(2)
expect(listener2.calls.length).toBe(2)
expect(listener3.calls.length).toBe(2)
})

it('delays subscribe until the end of current dispatch', () => {
Expand Down Expand Up @@ -353,43 +372,49 @@ describe('createStore', () => {
it('uses the last snapshot of subscribers during nested dispatch', () => {
const store = createStore(reducers.todos)

const listener1 = expect.createSpy(() => {})
const listener1 = expect.createSpy().andCall(() => {
if (!unsubscribe4) {
expect(listener1.calls.length).toBe(1)
expect(listener2.calls.length).toBe(0)
expect(listener3.calls.length).toBe(0)
expect(listener4.calls.length).toBe(0)

unsubscribe4 = store.subscribe(listener4)
unsubscribe1()
expect(listener1.calls.length).toBe(2)

store.dispatch(unknownAction())

expect(listener1.calls.length).toBe(2)
expect(listener2.calls.length).toBe(1)
expect(listener3.calls.length).toBe(1)
expect(listener4.calls.length).toBe(1)

}
})
const listener2 = expect.createSpy(() => {})
const listener3 = expect.createSpy(() => {})
const listener4 = expect.createSpy(() => {})

let unsubscribe4
const unsubscribe1 = store.subscribe(() => {
listener1()
expect(listener1.calls.length).toBe(1)
expect(listener2.calls.length).toBe(0)
expect(listener3.calls.length).toBe(0)
expect(listener4.calls.length).toBe(0)

unsubscribe1()
unsubscribe4 = store.subscribe(listener4)
store.dispatch(unknownAction())

expect(listener1.calls.length).toBe(1)
expect(listener2.calls.length).toBe(1)
expect(listener3.calls.length).toBe(1)
expect(listener4.calls.length).toBe(1)
})
const unsubscribe1 = store.subscribe(listener1)
store.subscribe(listener2)
store.subscribe(listener3)

store.dispatch(unknownAction())
expect(listener1.calls.length).toBe(1)
expect(listener2.calls.length).toBe(2)
expect(listener3.calls.length).toBe(2)
expect(listener1.calls.length).toBe(2)
expect(listener2.calls.length).toBe(1)
expect(listener3.calls.length).toBe(1)
expect(listener4.calls.length).toBe(1)

unsubscribe4()
expect(listener4.calls.length).toBe(2)

store.dispatch(unknownAction())
expect(listener1.calls.length).toBe(1)
expect(listener2.calls.length).toBe(3)
expect(listener3.calls.length).toBe(3)
expect(listener4.calls.length).toBe(1)
expect(listener1.calls.length).toBe(2)
expect(listener2.calls.length).toBe(2)
expect(listener3.calls.length).toBe(2)
expect(listener4.calls.length).toBe(2)
})

it('provides an up-to-date state when a subscriber is notified', done => {
Expand Down Expand Up @@ -699,7 +724,7 @@ describe('createStore', () => {
sub.unsubscribe()
store.dispatch({ type: 'bar' })

expect(results).toEqual([ { foo: 0, bar: 0 }, { foo: 1, bar: 0 } ])
expect(results).toEqual([ { foo: 0, bar: 0 }, { foo: 1, bar: 0 }, { foo: 1, bar: 0 } ])
})

it('should pass an integration test with a common library (RxJS)', () => {
Expand Down