From 661e643b60b4eab4ec41296fe41d50c389ded3c1 Mon Sep 17 00:00:00 2001 From: Esa-Matti Suuronen Date: Wed, 9 Sep 2015 10:14:16 +0000 Subject: [PATCH 1/3] Add failing test for #86 --- test/components/connect.spec.js | 65 +++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index 4f757a399..1145ce13f 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -1147,5 +1147,70 @@ describe('React', () => { wrapper.setState({ value: 1 }); expect(target.props.statefulValue).toEqual(1); }); + + it('should pass state consistently to mapState', () => { + const store = createStore(stringBuilder); + + store.dispatch({ type: 'APPEND', body: 'a'}); + let childMapStateInvokes = 0; + + @connect(state => ({ state })) + class Container extends Component { + + emitChange() { + store.dispatch({ type: 'APPEND', body: 'b'}); + } + + render() { + return ( +
+ + +
+ ); + } + } + + @connect((state, parentProps) => { + childMapStateInvokes++; + // The state from parent props should always be consistent with the current state + expect(state).toEqual(parentProps.parentState); + return {}; + }) + class ChildContainer extends Component { + render() { + return ; + } + } + + const tree = TestUtils.renderIntoDocument( + + + + ); + + expect(childMapStateInvokes).toBe(2); + + // The store state stays consistent when setState calls are batched + ReactDOM.unstable_batchedUpdates(() => { + store.dispatch({ type: 'APPEND', body: 'c'}); + }); + expect(childMapStateInvokes).toBe(3); + + // setState calls DOM handlers are batched + const container = TestUtils.findRenderedComponentWithType(tree, Container); + const node = React.findDOMNode(container.getWrappedInstance().refs.button); + TestUtils.Simulate.click(node); + expect(childMapStateInvokes).toBe(4); + + // In future all setState calls will be batched[1]. Uncomment when it + // happens. For now redux-batched-updates middleware can be used as + // workaround this. + // + // [1]: https://twitter.com/sebmarkbage/status/642366976824864768 + // + // store.dispatch({ type: 'APPEND', body: 'd'}); + // expect(childMapStateInvokes).toBe(5); + }); }); }); From b6272698d2fc9ff7f67014b3e527579f738cd81d Mon Sep 17 00:00:00 2001 From: Esa-Matti Suuronen Date: Mon, 14 Sep 2015 09:52:24 +0000 Subject: [PATCH 2/3] Ensure consistent state in mapState Closes #86 --- src/components/createConnect.js | 57 +++++++++++++++++---------------- 1 file changed, 29 insertions(+), 28 deletions(-) diff --git a/src/components/createConnect.js b/src/components/createConnect.js index a983fe069..9afbde391 100644 --- a/src/components/createConnect.js +++ b/src/components/createConnect.js @@ -89,7 +89,30 @@ export default function createConnect(React) { }; shouldComponentUpdate(nextProps, nextState) { - return !pure || !shallowEqual(this.state.props, nextState.props); + if (!pure) { + this.updateState(nextProps); + return true; + } + + const storeChanged = nextState.storeState !== this.state.storeState; + const propsChanged = !shallowEqual(nextProps, this.props); + let mapStateProducedChange = false; + let dispatchPropsChanged = false; + + if (storeChanged || (propsChanged && shouldUpdateStateProps)) { + mapStateProducedChange = this.updateStateProps(nextProps); + } + + if (propsChanged && shouldUpdateDispatchProps) { + dispatchPropsChanged = this.updateDispatchProps(nextProps); + } + + if (propsChanged || mapStateProducedChange || dispatchPropsChanged) { + this.updateState(nextProps); + return true; + } + + return false; } constructor(props, context) { @@ -106,9 +129,8 @@ export default function createConnect(React) { this.stateProps = computeStateProps(this.store, props); this.dispatchProps = computeDispatchProps(this.store, props); - this.state = { - props: this.computeNextState() - }; + this.state = { storeState: null }; + this.updateState(); } computeNextState(props = this.props) { @@ -140,12 +162,7 @@ export default function createConnect(React) { } updateState(props = this.props) { - const nextState = this.computeNextState(props); - if (!shallowEqual(nextState, this.state.props)) { - this.setState({ - props: nextState - }); - } + this.nextState = this.computeNextState(props); } isSubscribed() { @@ -170,20 +187,6 @@ export default function createConnect(React) { this.trySubscribe(); } - componentWillReceiveProps(nextProps) { - if (!shallowEqual(nextProps, this.props)) { - if (shouldUpdateStateProps) { - this.updateStateProps(nextProps); - } - - if (shouldUpdateDispatchProps) { - this.updateDispatchProps(nextProps); - } - - this.updateState(nextProps); - } - } - componentWillUnmount() { this.tryUnsubscribe(); } @@ -193,9 +196,7 @@ export default function createConnect(React) { return; } - if (this.updateStateProps()) { - this.updateState(); - } + this.setState({storeState: this.store.getState()}); } getWrappedInstance() { @@ -205,7 +206,7 @@ export default function createConnect(React) { render() { return ( + {...this.nextState} /> ); } } From 5ed9f40be79d70e5ee623f77fd186138d56784e4 Mon Sep 17 00:00:00 2001 From: Esa-Matti Suuronen Date: Wed, 9 Sep 2015 11:36:01 +0000 Subject: [PATCH 3/3] New render optimization test --- test/components/connect.spec.js | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index 1145ce13f..30741853f 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -1212,5 +1212,38 @@ describe('React', () => { // store.dispatch({ type: 'APPEND', body: 'd'}); // expect(childMapStateInvokes).toBe(5); }); + + it('should not render the wrapped component when mapState does not produce change', () => { + const store = createStore(stringBuilder); + let renderCalls = 0; + let mapStateCalls = 0; + + @connect(() => { + mapStateCalls++; + return {}; // no change! + }) + class Container extends Component { + render() { + renderCalls++; + return ; + } + } + + TestUtils.renderIntoDocument( + + + + ); + + expect(renderCalls).toBe(1); + expect(mapStateCalls).toBe(2); + + store.dispatch({ type: 'APPEND', body: 'a'}); + + // After store a change mapState has been called + expect(mapStateCalls).toBe(3); + // But render is not because it did not make any actual changes + expect(renderCalls).toBe(1); + }); }); });