From 20d3d044d95402fbd668445252680d56f6acf5dd Mon Sep 17 00:00:00 2001 From: Tim Dorr Date: Fri, 17 Feb 2017 09:54:07 -0500 Subject: [PATCH 1/2] Get rid of sCU Let's see what happens... --- src/components/connectAdvanced.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index 7f403f47e..bc67ef105 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -161,10 +161,6 @@ export default function connectAdvanced( this.selector.run(nextProps) } - shouldComponentUpdate() { - return this.selector.shouldComponentUpdate - } - componentWillUnmount() { if (this.subscription) this.subscription.tryUnsubscribe() this.subscription = null From 03ecbb8339e5b89caec4d7a3bcd34999efe1da6c Mon Sep 17 00:00:00 2001 From: Tim Dorr Date: Mon, 8 May 2017 16:12:45 -0400 Subject: [PATCH 2/2] Remove some more sCU mentions. Fix/remove old tests --- docs/troubleshooting.md | 22 ------- src/components/connectAdvanced.js | 8 +-- src/connect/selectorFactory.js | 9 +-- test/components/connect.spec.js | 96 +++---------------------------- 4 files changed, 14 insertions(+), 121 deletions(-) diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 9ec68653e..d3311fa74 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -40,28 +40,6 @@ render() { Conveniently, this gives your components access to the router state! You can also upgrade to React Router 1.0 which shouldn’t have this problem. (Let us know if it does!) -### My views aren’t updating when something changes outside of Redux - -If your views depend on global state or [React “context”](http://facebook.github.io/react/docs/context.html), you might find that views decorated with `connect()` will fail to update. - ->This is because `connect()` implements [shouldComponentUpdate](https://facebook.github.io/react/docs/component-specs.html#updating-shouldcomponentupdate) by default, assuming that your component will produce the same results given the same props and state. This is a similar concept to React’s [PureRenderMixin](https://facebook.github.io/react/docs/pure-render-mixin.html). - -The _best_ solution to this is to make sure that your components are pure and pass any external state to them via props. This will ensure that your views do not re-render unless they actually need to re-render and will greatly speed up your application. - -If that’s not practical for whatever reason (for example, if you’re using a library that depends heavily on React context), you may pass the `pure: false` option to `connect()`: - -``` -function mapStateToProps(state) { - return { todos: state.todos } -} - -export default connect(mapStateToProps, null, null, { - pure: false -})(TodoApp) -``` - -This will remove the assumption that `TodoApp` is pure and cause it to update whenever its parent component renders. Note that this will make your application less performant, so only do this if you have no other option. - ### Could not find "store" in either the context or props If you have context issues, diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index bc67ef105..bfdd1ab8d 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -190,16 +190,16 @@ export default function connectAdvanced( initSubscription() { if (!shouldHandleStateChanges) return - + // parentSub's source should match where store came from: props vs. context. A component // connected to the store via props shouldn't use subscription from context, or vice versa. const parentSub = (this.propsMode ? this.props : this.context)[subscriptionKey] this.subscription = new Subscription(this.store, parentSub, this.onStateChange.bind(this)) - + // `notifyNestedSubs` is duplicated to handle the case where the component is unmounted in // the middle of the notification loop, where `this.subscription` will then be null. An // extra null check every change can be avoided by copying the method onto `this` and then - // replacing it with a no-op on unmount. This can probably be avoided if Subscription's + // replacing it with a no-op on unmount. This can probably be avoided if Subscription's // listeners logic is changed to not call listeners that have been unsubscribed in the // middle of the notification loop. this.notifyNestedSubs = this.subscription.notifyNestedSubs.bind(this.subscription) @@ -214,7 +214,7 @@ export default function connectAdvanced( this.componentDidUpdate = this.notifyNestedSubsOnComponentDidUpdate this.setState(dummyState) } - } + } notifyNestedSubsOnComponentDidUpdate() { // `componentDidUpdate` is conditionally implemented when `onStateChange` determines it diff --git a/src/connect/selectorFactory.js b/src/connect/selectorFactory.js index 4479431e0..d81e59148 100644 --- a/src/connect/selectorFactory.js +++ b/src/connect/selectorFactory.js @@ -1,5 +1,5 @@ import verifySubselectors from './verifySubselectors' - + export function impureFinalPropsSelectorFactory( mapStateToProps, mapDispatchToProps, @@ -64,7 +64,7 @@ export function pureFinalPropsSelectorFactory( const nextStateProps = mapStateToProps(state, ownProps) const statePropsChanged = !areStatePropsEqual(nextStateProps, stateProps) stateProps = nextStateProps - + if (statePropsChanged) mergedProps = mergeProps(stateProps, dispatchProps, ownProps) @@ -92,11 +92,6 @@ export function pureFinalPropsSelectorFactory( // TODO: Add more comments -// If pure is true, the selector returned by selectorFactory will memoize its results, -// allowing connectAdvanced's shouldComponentUpdate to return false if final -// props have not changed. If false, the selector will always return a new -// object and shouldComponentUpdate will always return true. - export default function finalPropsSelectorFactory(dispatch, { initMapStateToProps, initMapDispatchToProps, diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index 094f4d697..999e6722f 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -6,6 +6,7 @@ import ReactDOM from 'react-dom' import TestUtils from 'react-addons-test-utils' import { createStore } from 'redux' import { connect } from '../../src/index' +import isEqual from 'lodash/isEqual' describe('React', () => { describe('connect', () => { @@ -398,6 +399,10 @@ describe('React', () => { } } + shouldComponentUpdate(nextProps) { + return !isEqual(this.props, nextProps) + } + componentDidMount() { // Simulate deep object mutation const bar = this.state.bar @@ -1105,94 +1110,6 @@ describe('React', () => { expect(spy.calls.length).toBe(3) }) - it('should shallowly compare the merged state to prevent unnecessary updates', () => { - const store = createStore(stringBuilder) - const spy = expect.createSpy(() => ({})) - function render({ string, pass }) { - spy() - return - } - - @connect( - state => ({ string: state }), - dispatch => ({ dispatch }), - (stateProps, dispatchProps, parentProps) => ({ - ...dispatchProps, - ...stateProps, - ...parentProps - }) - ) - class Container extends Component { - render() { - return render(this.props) - } - } - - class Root extends Component { - constructor(props) { - super(props) - this.state = { pass: '' } - } - - render() { - return ( - - - - ) - } - } - - const tree = TestUtils.renderIntoDocument() - const stub = TestUtils.findRenderedComponentWithType(tree, Passthrough) - expect(spy.calls.length).toBe(1) - expect(stub.props.string).toBe('') - expect(stub.props.pass).toBe('') - - store.dispatch({ type: 'APPEND', body: 'a' }) - expect(spy.calls.length).toBe(2) - expect(stub.props.string).toBe('a') - expect(stub.props.pass).toBe('') - - tree.setState({ pass: '' }) - expect(spy.calls.length).toBe(2) - expect(stub.props.string).toBe('a') - expect(stub.props.pass).toBe('') - - tree.setState({ pass: 'through' }) - expect(spy.calls.length).toBe(3) - expect(stub.props.string).toBe('a') - expect(stub.props.pass).toBe('through') - - tree.setState({ pass: 'through' }) - expect(spy.calls.length).toBe(3) - expect(stub.props.string).toBe('a') - expect(stub.props.pass).toBe('through') - - const obj = { prop: 'val' } - tree.setState({ pass: obj }) - expect(spy.calls.length).toBe(4) - expect(stub.props.string).toBe('a') - expect(stub.props.pass).toBe(obj) - - tree.setState({ pass: obj }) - expect(spy.calls.length).toBe(4) - expect(stub.props.string).toBe('a') - expect(stub.props.pass).toBe(obj) - - const obj2 = Object.assign({}, obj, { val: 'otherval' }) - tree.setState({ pass: obj2 }) - expect(spy.calls.length).toBe(5) - expect(stub.props.string).toBe('a') - expect(stub.props.pass).toBe(obj2) - - obj2.val = 'mutation' - tree.setState({ pass: obj2 }) - expect(spy.calls.length).toBe(5) - expect(stub.props.string).toBe('a') - expect(stub.props.passVal).toBe('otherval') - }) - it('should throw an error if a component is not passed to the function returned by connect', () => { expect(connect()).toThrow( /You must pass a component to the function/ @@ -1940,6 +1857,9 @@ describe('React', () => { @connect(null, mapDispatchFactory, mergeParentDispatch) class Passthrough extends Component { + shouldComponentUpdate(nextProps) { + return !isEqual(this.props, nextProps) + } componentWillUpdate() { updatedCount++ }