Merge pull request #99 from epeli/stale-props

Fix issues with stale props

Author: gaearon

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 (
             <WrappedComponent ref='wrappedInstance'
-                              {...this.state.props} />
+                              {...this.nextState} />
           );
         }
       }

test/components/connect.spec.js

@@ -1147,5 +1147,103 @@ 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 (
+            <div>
+              <button ref="button" onClick={this.emitChange.bind(this)}>change</button>
+              <ChildContainer parentState={this.props.state} />
+            </div>
+          );
+        }
+      }
+
+      @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 <Passthrough {...this.props}/>;
+        }
+      }
+
+      const tree = TestUtils.renderIntoDocument(
+        <ProviderMock store={store}>
+          <Container />
+        </ProviderMock>
+      );
+
+      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);
+    });
+
+    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 <Passthrough {...this.props} />;
+        }
+      }
+
+      TestUtils.renderIntoDocument(
+        <ProviderMock store={store}>
+          <Container />
+        </ProviderMock>
+      );
+
+      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);
+    });
   });
 });