From fba803e46ba930b09e3763d224e5abfdf0275ed2 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 13 Nov 2018 11:02:39 -0500 Subject: [PATCH] Data: Rerun selection if state changed during mount (#11777) --- packages/data/CHANGELOG.md | 6 + .../data/src/components/with-select/index.js | 54 +++++---- .../src/components/with-select/test/index.js | 113 ++++++++++++++---- 3 files changed, 126 insertions(+), 47 deletions(-) diff --git a/packages/data/CHANGELOG.md b/packages/data/CHANGELOG.md index 6dad7cf833a79..259755bf03cd9 100644 --- a/packages/data/CHANGELOG.md +++ b/packages/data/CHANGELOG.md @@ -1,3 +1,9 @@ +## 3.1.3 (Unreleased) + +### Bug Fix + +- Resolve an issue where `withSelect`'s `mapSelectToProps` would not be rerun if the wrapped component had incurred a store change during its mount lifecycle. + ## 3.1.2 (2018-11-09) ## 3.1.1 (2018-11-09) diff --git a/packages/data/src/components/with-select/index.js b/packages/data/src/components/with-select/index.js index de681581904c5..c471bce36388c 100644 --- a/packages/data/src/components/with-select/index.js +++ b/packages/data/src/components/with-select/index.js @@ -48,6 +48,8 @@ const withSelect = ( mapSelectToProps ) => createHigherOrderComponent( ( Wrapped constructor( props ) { super( props ); + this.onStoreChange = this.onStoreChange.bind( this ); + this.subscribe( props.registry ); this.mergeProps = getNextMergeProps( props ); @@ -55,6 +57,14 @@ const withSelect = ( mapSelectToProps ) => createHigherOrderComponent( ( Wrapped componentDidMount() { this.canRunSelection = true; + + // A state change may have occurred between the constructor and + // mount of the component (e.g. during the wrapped component's own + // constructor), in which case selection should be rerun. + if ( this.hasQueuedSelection ) { + this.hasQueuedSelection = false; + this.onStoreChange(); + } } componentWillUnmount() { @@ -103,29 +113,31 @@ const withSelect = ( mapSelectToProps ) => createHigherOrderComponent( ( Wrapped return true; } - subscribe( registry ) { - this.unsubscribe = registry.subscribe( () => { - if ( ! this.canRunSelection ) { - return; - } + onStoreChange() { + if ( ! this.canRunSelection ) { + this.hasQueuedSelection = true; + return; + } - const nextMergeProps = getNextMergeProps( this.props ); - if ( isShallowEqual( this.mergeProps, nextMergeProps ) ) { - return; - } + const nextMergeProps = getNextMergeProps( this.props ); + if ( isShallowEqual( this.mergeProps, nextMergeProps ) ) { + return; + } + + this.mergeProps = nextMergeProps; - this.mergeProps = nextMergeProps; - - // Schedule an update. Merge props are not assigned to state - // because derivation of merge props from incoming props occurs - // within shouldComponentUpdate, where setState is not allowed. - // setState is used here instead of forceUpdate because forceUpdate - // bypasses shouldComponentUpdate altogether, which isn't desireable - // if both state and props change within the same render. - // Unfortunately this requires that next merge props are generated - // twice. - this.setState( {} ); - } ); + // Schedule an update. Merge props are not assigned to state since + // derivation of merge props from incoming props occurs within + // shouldComponentUpdate, where setState is not allowed. setState + // is used here instead of forceUpdate because forceUpdate bypasses + // shouldComponentUpdate altogether, which isn't desireable if both + // state and props change within the same render. Unfortunately, + // this requires that next merge props are generated twice. + this.setState( {} ); + } + + subscribe( registry ) { + this.unsubscribe = registry.subscribe( this.onStoreChange ); } render() { diff --git a/packages/data/src/components/with-select/test/index.js b/packages/data/src/components/with-select/test/index.js index 2af54df517020..4a874872df032 100644 --- a/packages/data/src/components/with-select/test/index.js +++ b/packages/data/src/components/with-select/test/index.js @@ -7,6 +7,7 @@ import TestRenderer from 'react-test-renderer'; * WordPress dependencies */ import { compose } from '@wordpress/compose'; +import { Component } from '@wordpress/element'; /** * Internal dependencies @@ -45,11 +46,11 @@ describe( 'withSelect', () => {
{ props.data }
) ); - const Component = withSelect( mapSelectToProps )( OriginalComponent ); + const DataBoundComponent = withSelect( mapSelectToProps )( OriginalComponent ); const testRenderer = TestRenderer.create( - + ); const testInstance = testRenderer.root; @@ -94,14 +95,14 @@ describe( 'withSelect', () => { ) ); - const Component = compose( [ + const DataBoundComponent = compose( [ withSelect( mapSelectToProps ), withDispatch( mapDispatchToProps ), ] )( OriginalComponent ); const testRenderer = TestRenderer.create( - + ); const testInstance = testRenderer.root; @@ -123,6 +124,66 @@ describe( 'withSelect', () => { expect( OriginalComponent ).toHaveBeenCalledTimes( 2 ); } ); + it( 'should rerun if had dispatched action during mount', () => { + registry.registerStore( 'counter', { + reducer: ( state = 0, action ) => { + if ( action.type === 'increment' ) { + return state + 1; + } + + return state; + }, + selectors: { + getCount: ( state ) => state, + }, + actions: { + increment: () => ( { type: 'increment' } ), + }, + } ); + + class OriginalComponent extends Component { + constructor( props ) { + super( ...arguments ); + + props.increment(); + } + + componentDidMount() { + this.props.increment(); + } + + render() { + return
{ this.props.count }
; + } + } + + jest.spyOn( OriginalComponent.prototype, 'render' ); + + const mapSelectToProps = jest.fn().mockImplementation( ( _select, ownProps ) => ( { + count: _select( 'counter' ).getCount( ownProps.offset ), + } ) ); + + const mapDispatchToProps = jest.fn().mockImplementation( ( _dispatch ) => ( { + increment: _dispatch( 'counter' ).increment, + } ) ); + + const DataBoundComponent = compose( [ + withSelect( mapSelectToProps ), + withDispatch( mapDispatchToProps ), + ] )( OriginalComponent ); + + const testRenderer = TestRenderer.create( + + + + ); + const testInstance = testRenderer.root; + + expect( testInstance.findByType( 'div' ).props.children ).toBe( 2 ); + expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 ); + expect( OriginalComponent.prototype.render ).toHaveBeenCalledTimes( 2 ); + } ); + it( 'should rerun selection on props changes', () => { registry.registerStore( 'counter', { reducer: ( state = 0, action ) => { @@ -145,11 +206,11 @@ describe( 'withSelect', () => {
{ props.count }
) ); - const Component = withSelect( mapSelectToProps )( OriginalComponent ); + const DataBoundComponent = withSelect( mapSelectToProps )( OriginalComponent ); const testRenderer = TestRenderer.create( - + ); const testInstance = testRenderer.root; @@ -159,7 +220,7 @@ describe( 'withSelect', () => { testRenderer.update( - + ); @@ -180,11 +241,11 @@ describe( 'withSelect', () => { const OriginalComponent = jest.fn().mockImplementation( () =>
); - const Component = compose( [ + const DataBoundComponent = compose( [ withSelect( mapSelectToProps ), ] )( OriginalComponent ); - const Parent = ( props ) => ; + const Parent = ( props ) => ; const testRenderer = TestRenderer.create( @@ -222,11 +283,11 @@ describe( 'withSelect', () => { const OriginalComponent = jest.fn().mockImplementation( () =>
); - const Component = withSelect( mapSelectToProps )( OriginalComponent ); + const DataBoundComponent = withSelect( mapSelectToProps )( OriginalComponent ); TestRenderer.create( - + ); @@ -251,13 +312,13 @@ describe( 'withSelect', () => { const OriginalComponent = jest.fn().mockImplementation( () =>
); - const Component = compose( [ + const DataBoundComponent = compose( [ withSelect( mapSelectToProps ), ] )( OriginalComponent ); const testRenderer = TestRenderer.create( - + ); @@ -266,7 +327,7 @@ describe( 'withSelect', () => { testRenderer.update( - + ); @@ -286,13 +347,13 @@ describe( 'withSelect', () => { const OriginalComponent = jest.fn().mockImplementation( () =>
); - const Component = compose( [ + const DataBoundComponent = compose( [ withSelect( mapSelectToProps ), ] )( OriginalComponent ); TestRenderer.create( - + ); @@ -322,11 +383,11 @@ describe( 'withSelect', () => { const OriginalComponent = jest.fn() .mockImplementation( ( props ) =>
{ JSON.stringify( props ) }
); - const Component = withSelect( mapSelectToProps )( OriginalComponent ); + const DataBoundComponent = withSelect( mapSelectToProps )( OriginalComponent ); const testRenderer = TestRenderer.create( - + ); const testInstance = testRenderer.root; @@ -339,7 +400,7 @@ describe( 'withSelect', () => { testRenderer.update( - + ); @@ -369,11 +430,11 @@ describe( 'withSelect', () => { ( props ) =>
{ props.count || 'Unknown' }
) ); - const Component = withSelect( mapSelectToProps )( OriginalComponent ); + const DataBoundComponent = withSelect( mapSelectToProps )( OriginalComponent ); const testRenderer = TestRenderer.create( - + ); const testInstance = testRenderer.root; @@ -384,7 +445,7 @@ describe( 'withSelect', () => { testRenderer.update( - + ); @@ -394,7 +455,7 @@ describe( 'withSelect', () => { testRenderer.update( - + ); @@ -465,11 +526,11 @@ describe( 'withSelect', () => {
{ props.value }
) ); - const Component = withSelect( mapSelectToProps )( OriginalComponent ); + const DataBoundComponent = withSelect( mapSelectToProps )( OriginalComponent ); const testRenderer = TestRenderer.create( - + ); const testInstance = testRenderer.root; @@ -491,7 +552,7 @@ describe( 'withSelect', () => { testRenderer.update( - + );