From 6b5b57bd6fbf066601a2e22384ffb981a83ae1a8 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Wed, 4 Jul 2018 10:10:01 -0400 Subject: [PATCH] Data: Optimize withSelect to avoid generating merge props on equal props (#7699) * Data: Optimize withSelect to avoid generating merge props on equal props * Data: Use shouldComponentUpdate and state forceUpdate for render updates --- packages/data/src/index.js | 85 ++++++++++++++++++++++----------- packages/data/src/test/index.js | 58 ++++++++++++++++++++++ 2 files changed, 116 insertions(+), 27 deletions(-) diff --git a/packages/data/src/index.js b/packages/data/src/index.js index 23cb9d6f81545..beb7bbe22ee76 100644 --- a/packages/data/src/index.js +++ b/packages/data/src/index.js @@ -266,27 +266,36 @@ export function dispatch( reducerKey ) { * @return {Component} Enhanced component with merged state data props. */ export const withSelect = ( mapStateToProps ) => createHigherOrderComponent( ( WrappedComponent ) => { + /** + * Default merge props. A constant value is used as the fallback since it + * can be more efficiently shallow compared in case component is repeatedly + * rendered without its own merge props. + * + * @type {Object} + */ const DEFAULT_MERGE_PROPS = {}; + /** + * Given a props object, returns the next merge props by mapStateToProps. + * + * @param {Object} props Props to pass as argument to mapStateToProps. + * + * @return {Object} Props to merge into rendered wrapped element. + */ + function getNextMergeProps( props ) { + return ( + mapStateToProps( select, props ) || + DEFAULT_MERGE_PROPS + ); + } + return class ComponentWithSelect extends Component { - constructor() { - super( ...arguments ); + constructor( props ) { + super( props ); this.subscribe(); - this.state = {}; - } - - static getDerivedStateFromProps( props ) { - // A constant value is used as the fallback since it can be more - // efficiently shallow compared in case component is repeatedly - // rendered without its own merge props. - const mergeProps = ( - mapStateToProps( select, props ) || - DEFAULT_MERGE_PROPS - ); - - return { mergeProps }; + this.mergeProps = getNextMergeProps( props ); } componentDidMount() { @@ -298,11 +307,28 @@ export const withSelect = ( mapStateToProps ) => createHigherOrderComponent( ( W this.unsubscribe(); } - shouldComponentUpdate( nextProps, nextState ) { - return ( - ! isShallowEqual( this.props, nextProps ) || - ! isShallowEqual( this.state.mergeProps, nextState.mergeProps ) - ); + shouldComponentUpdate( nextProps ) { + // This implementation of shouldComponentUpdate is only intended + // to handle incoming props changes. Changes effected via state + // changes are reflected by the subscribe handler's forceUpdate. + if ( isShallowEqual( this.props, nextProps ) ) { + return false; + } + + // Regardless of whether merge props are changing, the component + // will need to render if props are different. But if the merge + // props change as a result of the incoming props, they should be + // reflected as such in the next render. + const nextMergeProps = getNextMergeProps( nextProps ); + if ( ! isShallowEqual( this.mergeProps, nextMergeProps ) ) { + // Yes, this is a side effect. Yes, side effects don't belong + // in shouldComponentUpdate. This is not a typical component in + // that it is very much a hot code path, and will employ any + // and all gymnastics necessary for optimal performance. + this.mergeProps = nextMergeProps; + } + + return true; } subscribe() { @@ -311,17 +337,22 @@ export const withSelect = ( mapStateToProps ) => createHigherOrderComponent( ( W return; } - // Trigger an update. Behavior of `getDerivedStateFromProps` as - // of React 16.4.0 is such that it will be called by any update - // to the component, including state changes. - // - // See: https://reactjs.org/blog/2018/05/23/react-v-16-4.html#bugfix-for-getderivedstatefromprops - this.setState( () => ( {} ) ); + const nextMergeProps = getNextMergeProps( this.props ); + if ( isShallowEqual( this.mergeProps, nextMergeProps ) ) { + return; + } + + this.mergeProps = nextMergeProps; + + // forceUpdate is used to intentionally bypass the default + // behavior of shouldComponentUpdate, which in this component + // is specifically designed to handle only props changes. + this.forceUpdate(); } ); } render() { - return ; + return ; } }; }, 'withSelect' ); diff --git a/packages/data/src/test/index.js b/packages/data/src/test/index.js index 25dab32ba8fc3..53282b3aea1f5 100644 --- a/packages/data/src/test/index.js +++ b/packages/data/src/test/index.js @@ -529,6 +529,64 @@ describe( 'withSelect', () => { expect( OriginalComponent ).toHaveBeenCalledTimes( 2 ); } ); + it( 'should not run selection if props have not changed', () => { + store = registerReducer( 'unchanging', ( state = {} ) => state ); + + registerSelectors( 'unchanging', { + getState: ( state ) => state, + } ); + + const mapSelectToProps = jest.fn(); + + const OriginalComponent = jest.fn().mockImplementation( () =>
); + + const Component = compose( [ + withSelect( mapSelectToProps ), + ] )( OriginalComponent ); + + const Parent = ( props ) => ; + + wrapper = mount( ); + + expect( mapSelectToProps ).toHaveBeenCalledTimes( 1 ); + expect( OriginalComponent ).toHaveBeenCalledTimes( 1 ); + + wrapper.setProps( { propName: 'foo' } ); + + expect( mapSelectToProps ).toHaveBeenCalledTimes( 1 ); + expect( OriginalComponent ).toHaveBeenCalledTimes( 1 ); + } ); + + it( 'should not run selection if state has changed but merge props the same', () => { + store = registerReducer( 'demo', () => ( {} ) ); + + registerSelectors( 'demo', { + getUnchangingValue: () => 10, + } ); + + registerActions( 'demo', { + update: () => ( { type: 'update' } ), + } ); + + const mapSelectToProps = jest.fn().mockImplementation( ( _select ) => ( { + value: _select( 'demo' ).getUnchangingValue(), + } ) ); + + const OriginalComponent = jest.fn().mockImplementation( () =>
); + + const Component = withSelect( mapSelectToProps )( OriginalComponent ); + + wrapper = mount( ); + + expect( mapSelectToProps ).toHaveBeenCalledTimes( 1 ); + expect( OriginalComponent ).toHaveBeenCalledTimes( 1 ); + + dispatch( 'demo' ).update(); + + expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 ); + expect( OriginalComponent ).toHaveBeenCalledTimes( 1 ); + } ); + it( 'should render if props have changed but not state', () => { store = registerReducer( 'unchanging', ( state = {} ) => state );