Merge pull request reduxjs#472 from jimbolla/fix-457

tests and fixes reduxjs#457

Author: timdorr

src/utils/Subscription.js

@@ -1,45 +1,61 @@
 // encapsulates the subscription logic for connecting a component to the redux store, as
 // well as nesting subscriptions of descendant components, so that we can ensure the
 // ancestor components re-render before descendants
+
+const CLEARED = null
+
+function createListenerCollection() {
+  // the current/next pattern is copied from redux's createStore code.
+  // TODO: refactor+expose that code to be reusable here?
+  let current = []
+  let next = []
+
+  return {
+    clear() {
+      next = CLEARED
+      current = CLEARED
+    },
+
+    notify() {
+      current = next
+      for (let i = 0; i < current.length; i++) {
+        current[i]()
+      }
+    },
+
+    subscribe(listener) {
+      let isSubscribed = true
+      if (next === current) next = current.slice()
+      next.push(listener)
+
+      return function unsubscribe() {
+        if (!isSubscribed || current === CLEARED) return
+        isSubscribed = false
+
+        if (next === current) next = current.slice()
+        next.splice(next.indexOf(listener), 1)
+      }
+    }
+  }
+}
+
 export default class Subscription {
   constructor(store, parentSub) {
     this.subscribe = parentSub
       ? parentSub.addNestedSub.bind(parentSub)
       : store.subscribe.bind(store)
 
     this.unsubscribe = null
-    this.nextListeners = this.currentListeners = []
-  }
-
-  ensureCanMutateNextListeners() {
-    if (this.nextListeners === this.currentListeners) {
-      this.nextListeners = this.currentListeners.slice()
-    }
+    this.listeners = createListenerCollection()
   }
 
   addNestedSub(listener) {
     this.trySubscribe()
-
-    let isSubscribed = true
-    this.ensureCanMutateNextListeners()
-    this.nextListeners.push(listener)
-
-    return function unsubscribe() {
-      if (!isSubscribed) return
-      isSubscribed = false
-
-      this.ensureCanMutateNextListeners()
-      const index = this.nextListeners.indexOf(listener)
-      this.nextListeners.splice(index, 1)
-    }
+    return this.listeners.subscribe(listener)
   }
 
   notifyNestedSubs() {
-    const listeners = this.currentListeners = this.nextListeners
-    const length = listeners.length
-    for (let i = 0; i < length; i++) {
-      listeners[i]()
-    }
+    this.listeners.notify()
   }
 
   isSubscribed() {
@@ -55,7 +71,10 @@ export default class Subscription {
   tryUnsubscribe() {
     if (this.unsubscribe) {
       this.unsubscribe()
+      this.listeners.clear()
     }
     this.unsubscribe = null
+    this.subscribe = null
+    this.listeners = { notify() {} }
   }
 }

test/components/connect.spec.js

@@ -910,6 +910,86 @@ describe('React', () => {
       expect(mapStateToPropsCalls).toBe(1)
     })
 
+    it('should not attempt to set state after unmounting nested components', () => {
+      const store = createStore(() => ({}))
+      let mapStateToPropsCalls = 0
+
+      let linkA, linkB
+
+      let App = ({ children, setLocation }) => {
+        const onClick = to => event => {
+          event.preventDefault()
+          setLocation(to)
+        }
+        /* eslint-disable react/jsx-no-bind */
+        return (
+          <div>
+            <a href="#" onClick={onClick('a')} ref={c => { linkA = c }}>A</a>
+            <a href="#" onClick={onClick('b')} ref={c => { linkB = c }}>B</a>
+            {children}
+          </div>
+        )
+        /* eslint-enable react/jsx-no-bind */
+      }
+      App = connect(() => ({}))(App)
+
+
+      let A = () => (<h1>A</h1>)
+      A = connect(() => ({ calls: ++mapStateToPropsCalls }))(A)
+
+
+      const B = () => (<h1>B</h1>)
+
+
+      class RouterMock extends React.Component {
+        constructor(...args) {
+          super(...args)
+          this.state = { location: { pathname: 'a' } }
+          this.setLocation = this.setLocation.bind(this)
+        }
+
+        setLocation(pathname) {
+          this.setState({ location: { pathname } })
+          store.dispatch({ type: 'TEST' })
+        }
+
+        getChildComponent(location) {
+          switch (location) {
+            case 'a': return <A />
+            case 'b': return <B />
+            default: throw new Error('Unknown location: ' + location)
+          }
+        }
+
+        render() {
+          return (<App setLocation={this.setLocation}>
+            {this.getChildComponent(this.state.location.pathname)}
+          </App>)
+        }
+      }
+
+
+      const div = document.createElement('div')
+      document.body.appendChild(div)
+      ReactDOM.render(
+        (<ProviderMock store={store}>
+          <RouterMock />
+        </ProviderMock>),
+        div
+      )
+
+      const spy = expect.spyOn(console, 'error')
+
+      linkA.click()
+      linkB.click()
+      linkB.click()
+
+      spy.destroy()
+      document.body.removeChild(div)
+      expect(mapStateToPropsCalls).toBe(3)
+      expect(spy.calls.length).toBe(0)
+    })
+
     it('should not attempt to set state when dispatching in componentWillUnmount', () => {
       const store = createStore(stringBuilder)
       let mapStateToPropsCalls = 0