From c7f0efd097642b913bf7ca6486ff0028156bcbf3 Mon Sep 17 00:00:00 2001 From: Jim Bolla Date: Tue, 23 Aug 2016 20:49:02 -0400 Subject: [PATCH 1/3] tests and fixes #457 --- src/utils/Subscription.js | 69 +++++++++++++++++----------- test/components/connect.spec.js | 80 +++++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 25 deletions(-) diff --git a/src/utils/Subscription.js b/src/utils/Subscription.js index b3b63ec4d..ff4a063c1 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 + +function initListeners() { + let count = 0 + let current = [] + let next = [] + + return { + clear() { + count = 0 + next = null + current = null + }, + + notify() { + current = next + for (let i = 0; i < count; i++) { + current[i]() + } + }, + + subscribe(listener) { + let isSubscribed = true + if (next === current) next = current.slice() + next.push(listener) + count++ + + return function unsubscribe() { + if (!isSubscribed || count === 0) return + isSubscribed = false + + if (next === current) next = current.slice() + next.splice(next.indexOf(listener), 1) + count-- + } + } + } +} + 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 = initListeners() } 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 From ec641a78986886e8d2f99e3f640017db1dd43797 Mon Sep 17 00:00:00 2001 From: Jim Bolla Date: Tue, 23 Aug 2016 21:07:53 -0400 Subject: [PATCH 2/3] refactors out count variable in Subscription.js --- src/utils/Subscription.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/utils/Subscription.js b/src/utils/Subscription.js index ff4a063c1..d7854b9a1 100644 --- a/src/utils/Subscription.js +++ b/src/utils/Subscription.js @@ -3,20 +3,18 @@ // ancestor components re-render before descendants function initListeners() { - let count = 0 let current = [] let next = [] return { clear() { - count = 0 next = null current = null }, notify() { current = next - for (let i = 0; i < count; i++) { + for (let i = 0; i < current.length; i++) { current[i]() } }, @@ -25,15 +23,13 @@ function initListeners() { let isSubscribed = true if (next === current) next = current.slice() next.push(listener) - count++ return function unsubscribe() { - if (!isSubscribed || count === 0) return + if (!isSubscribed || !current) return isSubscribed = false if (next === current) next = current.slice() next.splice(next.indexOf(listener), 1) - count-- } } } From 78b4b781bc6410fb90b93abda8cd4c1bf1f6c5c8 Mon Sep 17 00:00:00 2001 From: Jim Bolla Date: Thu, 25 Aug 2016 08:07:27 -0400 Subject: [PATCH 3/3] refactors subscription to be slightly clearer --- src/utils/Subscription.js | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/utils/Subscription.js b/src/utils/Subscription.js index d7854b9a1..98daa442a 100644 --- a/src/utils/Subscription.js +++ b/src/utils/Subscription.js @@ -2,14 +2,18 @@ // well as nesting subscriptions of descendant components, so that we can ensure the // ancestor components re-render before descendants -function initListeners() { +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 = null - current = null + next = CLEARED + current = CLEARED }, notify() { @@ -25,7 +29,7 @@ function initListeners() { next.push(listener) return function unsubscribe() { - if (!isSubscribed || !current) return + if (!isSubscribed || current === CLEARED) return isSubscribed = false if (next === current) next = current.slice() @@ -42,7 +46,7 @@ export default class Subscription { : store.subscribe.bind(store) this.unsubscribe = null - this.listeners = initListeners() + this.listeners = createListenerCollection() } addNestedSub(listener) {