Use change-emitter module to handle subscriptions

[acdlite]

createStore includes a really handy, bare-bones event subscription implementation that I found myself wanting in a few other projects. (E.g. here: acdlite/recompose#160). So I extracted it out into a separate module: https://github.com/acdlite/change-emitter

When I tweeted about it, @tappleby pointed me to this issue (which, by coincidence, was closed just three hours ago :D) tappleby/redux-batched-subscribe#8.

This PR rewrites createStore to use change-emitter. Not sure if it's something we want to do but I figured we should discuss it.

At the very least, I think it would be valuable to extract the subscription stuff into a separate file, even if it stays within the Redux project.

[acdlite]

The obvious disadvantage is that if we have subscription-related bugs, they'll have to be resolved in a separate project.

Potential upside is that if the package gets more libraries to use it (history, perhaps), we'll all benefit from its maintenance.

[splendido]

for an example of something that should be addressed within this package see #1626

[gaearon]

I think I’d be confident extracting it if we define a strict contract that it promises to satisfy. There is a thousand ways to implement a change emitter, and some constraints that Redux cares about might not be generally applicable so it’s easy to imagine them being changed as implementation details of a module like this, potentially leading to breakage in Redux. If, on the contrary, we keep the tests in Redux to prevent problems like this, I don’t see much of a point of keeping that code outside, as this increases rather than decreases the maintenance burden.

[acdlite]

@gaearon

I think I’d be confident extracting it if we define a strict contract that it promises to satisfy.

Agreed.

There is a thousand ways to implement a change emitter, and some constraints that Redux cares about might not be generally applicable

Could you elaborate? I agree with the statement but I'm having trouble putting the contract that Redux follows into words. As far as I can tell, the important bits are

1. subscribing or unsubscribing as the result of a change should not affect which listeners are called for that change.
2. a nested change (change emitted as the result of another change) should be sent to the most recent list of listeners, taking into account any listeners were added or removed during the current change (point 1).

Is there anything else I'm missing?

[gaearon]

I think so but I forget these rules all the time. The important part is to take a naïve connect implementation like

mount() {
  this.mostRecentState = store.getState()
  this.unsubscribe = store.subscribe(this.handleChange.bind(this))
}

unmount() {
  this.unsubscribe()
  this.unsubscribe = null
}

handleChange() {
  if (this.unsubscribe) {
    this.mostRecentState = store.getState()
  }
}

and make sure that this.mostRecentState always corresponds to the most recent state by the time the outermost dispatch exits.

[acdlite]

I think so but I forget these rules all the time

Haha me too. I think regardless of whether we extract it, we need to do a better job documenting this contract, either via better tests or an actual document. I'll look into this sometime soon.

[gaearon]

We have some clarifications in createStore doc (they were fixed once) but I’m not 100% sure they’re correct. I think #1729 (comment) is the best starting point; whatever contract I imagined was influenced only by getting this invariant to be true in all cases: all connected components receive the latest state by the time we exit dispatch. The components that just subscribed don't need to be notified because they read the state synchronously, but if the state changed because of nested dispatch again, we should call them as well. (In fact I'm not quite sure we're handling this situation correctly right now.)

[acdlite]

all connected components receive the latest state by the time we exit dispatch

Yeah this seems like the most important part, and yet we don't have a test for it :D The only thing we currently test is how often listeners are called, not what the state is at that time.

[gaearon]

We should rewrite the tests. 👍

[gaearon]

(With a fake naïve connect() rather than the real one)

[timdorr]

Hey @acdlite, any updates on this? This will also be helpful in React Redux, so this could kill a lot of birds with one stone 👍