diff --git a/src/utils/Subscription.js b/src/utils/Subscription.js index b3b63ec4d..98daa442a 100644 --- a/src/utils/Subscription.js +++ b/src/utils/Subscription.js @@ -1,6 +1,44 @@ // 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 @@ -8,38 +46,16 @@ export default class Subscription { : 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() {} } } } diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index b7f8e2a16..627f7ff75 100644 --- a/test/components/connect.spec.js +++ b/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 ( +
+ { linkA = c }}>A + { linkB = c }}>B + {children} +
+ ) + /* eslint-enable react/jsx-no-bind */ + } + App = connect(() => ({}))(App) + + + let A = () => (

A

) + A = connect(() => ({ calls: ++mapStateToPropsCalls }))(A) + + + const B = () => (

B

) + + + 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 + case 'b': return + default: throw new Error('Unknown location: ' + location) + } + } + + render() { + return ( + {this.getChildComponent(this.state.location.pathname)} + ) + } + } + + + const div = document.createElement('div') + document.body.appendChild(div) + ReactDOM.render( + ( + + ), + 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