Avoid unnecessary calls to listeners

[splendido]

This PR is a better version of #1626.
Please see the original comment for explanation of aims and proposed solution (mainly related to efficiency and effectiveness of code).

In my opinion the present proposal lowers the computational complexity of the recursive dispatch mechanism and prevents listeners to be notified more than once with the very same state value during the same call to dispatch.
The cost to be paid is the use of slice and shift operations on listener arrays which is anyway O(N) (with N the number of registered listeners) and usually in the order of microseconds.

There is another interesting issue which is solved by the present proposal. Consider the following example:

var unsubscribe2
const store = createStore(myReducer)

store.subscribe(() => {
  const state = store.getState()

  if (condition1) {
    unsubscribe2()
  }

  if (condition2) {
    store.dispatch(actionYYY())
  }
})

function listener2() {
  // ...
}
unsubscribe2 = store.subscribe(listener2)

store.dispatch(actionXXX())  // --> making true both condition1 and condition2 

With the current code, in case both condition1 and condition2 result true at the same time for a particular state, listener2 will be notified with the new state value as set because of actionYYY and not as it was set because of actionXXX (when the first listener decided it was the right time to unsubscribe listener2...).
With the new code, listener2 will be notified its last time right before its actual unsubscription, hence with the state as set by actionXXX.

But that's not all.
As a general note I think we should provide a more detailed contract for the subscribe and unsubscribe API calls and listeners notifications.
With the present proposal we could be able to state:

1. listeners will not be notified with state changes in response to calls to dispatch occurred before their subscription (trivial...).
2. listeners will not be notified with state changes in response to calls to dispatch occurred after unsubscription requests.
3. listeners will be notified at least once with the most recent state after a call to dispatch.
4. in case of nested calls to dispatch, listeners could be notified more than once with updating state values but this is not guaranteed and can easily vary depending on the order of subscriptions.
5. listeners will be notified at most once with the very same state in response to the very same call to dispatch.

By further modifying dispatch to look like this:

function dispatch(action) {

  let nextState
  try {
    isDispatching = true
    nextState = currentReducer(currentState, action)
  } finally {
    isDispatching = false
  }

  if (nextState !== currentState) {
    currentState = nextState
    listeners = currentListeners.slice()
    while (listeners.length) {
      listeners.shift()()
    }
  }

  return action
}

at no additional cost, we could change 5. to:
5. listeners will never be notified twice with the same state value

Obvioulsy, secific tests should be added to support these statements.
...and I'll be happy to add them in case you're interested to proceed further with this.

[timdorr]

We used to do this, but it was removed for performance and memory reasons in c031c0a. You're creating multiple duplicate arrays just to avoid race conditions when nesting your dispatches. The simple for loop from before is easier to read and faster.

[splendido]

Actually implications are much more than this!

Lets think about the nested calls to dispatch. When you do

listeners = currentListeners.slice()

you're saying: no matter who was already notified, we now have to notify everyone about the new change, and we do not have to notify everyone about every change...
This copy of the array also allows to check whether a listener being unsubscribed was already notified or not about the last state change.

At the same time, the time needed to shallow-copy an array is far less than the one needed to go through useless notifications, function calls, state checks within listeners, and so on.
It is a bit more pretending from a memory efficiency point of view, though.

To improve things we can avoid the use of shift() and revert back to listeners[i]() having the value of i controlled among the different nested dispatches.
I can make this change this evening...

[splendido]

I've tested the use of the for loop with this branch
See changes here

[timdorr]

1. listeners will never be notified twice with the same state value

I'm really not liking this part of the proposal. Many times subscribers are used to integrate external libraries with Redux. And these external libraries normally have some of their own state that is stored externally and make conditional decisions based on that external state, particular to the needs of that library. react-redux does this, using a subscriber and it's own local state.

By attempting to optimize on the user's behalf, we take away some control that they have to behave different depending on state external to the store. This would end up breaking certain types of integrations. It's simpler and more powerful to leave the referential equality check as an option to the user.

[timdorr]

Also, definitely see #1729, as it is directly related to this. I know @acdlite also wants to nail down the exact contract provided by subscribe.

For this effort, as @gaearon has stated, we should rewrite the tests (and allow them to break) to ensure that contract is codified. A test-first approach is best here because once that test suite passes, we can know we have the expected behavior. We just need a comprehensive set of tests.

[splendido]

I'm really not liking this part of the proposal. Many times subscribers are used to integrate external libraries with Redux.

Fair enough!
I was not aware of this, my fault

Edit: better looking at handleChange from react-redux, it doesn't seem a false positive could trigger any internal state change for the library. If pure is true nothing happens and the code returns (see here) while if pure is false the execution never enter the second condition (see here) and the store's state is wrongly marked as changed which is not true actually (see here)

At the same time, relying on (random) false positive triggers from redux to update react-redux internal state doesn't look like a robust strategy. I guess another independent mechanism to call handleChange (in response to other events or periodically?) is needed to ensure correct component updates.

[splendido]

Ok, this seems the golden proposal to me :-)
No use of slice nor shift, more or less is like before.
The crucial differences are:

• notify a listener in case it is being unsubscribed during dispatch notification and it was not yet notified about the last state change (see here); a specific test was added to ensure this (see here)
• use global listeners and listenerId to reset the notification cycle from zero at every new nested call to dispatch (see here)
• mark the end of notifications when the deepest nested call to dispatch completes the cycle so that shallower paused cycles will be abandoned straight away (see here)
• align currentListeners to nextListeners at the end of notifications to forget the old list of listeners and possibly free some resources; this is effective only in case during the notification cycle some subscriptions/unsubscriptions happened (see here)

Thanks @timdorr for your comments: without them I wouldn't have pushed too much for the optimization of array copies!

[timdorr]

Let's see what happens in #1729 for now. I would submit some PRs to /~https://github.com/acdlite/change-emitter to get the ball moving.