Skip to content

Commit

Permalink
Data: Rerun selection if state changed during mount (#11777)
Browse files Browse the repository at this point in the history
  • Loading branch information
@aduth
aduth committed Nov 13, 2018
1 parent d521f25 commit fba803e
Show file tree
Hide file tree
Showing 3 changed files with 126 additions and 47 deletions.
6 changes: 6 additions & 0 deletions packages/data/CHANGELOG.md
View file
@@ -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)
Expand Down
54 changes: 33 additions & 21 deletions packages/data/src/components/with-select/index.js
View file
Expand Up @@ -48,13 +48,23 @@ const withSelect = ( mapSelectToProps ) => createHigherOrderComponent( ( Wrapped
constructor( props ) {
super( props );

this.onStoreChange = this.onStoreChange.bind( this );

this.subscribe( props.registry );

this.mergeProps = getNextMergeProps( props );
}

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() {
Expand Down Expand Up @@ -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() {
Expand Down
113 changes: 87 additions & 26 deletions packages/data/src/components/with-select/test/index.js
View file
Expand Up @@ -7,6 +7,7 @@ import TestRenderer from 'react-test-renderer';
* WordPress dependencies
*/
import { compose } from '@wordpress/compose';
import { Component } from '@wordpress/element';

/**
* Internal dependencies
Expand Down Expand Up @@ -45,11 +46,11 @@ describe( 'withSelect', () => {
<div>{ props.data }</div>
) );

const Component = withSelect( mapSelectToProps )( OriginalComponent );
const DataBoundComponent = withSelect( mapSelectToProps )( OriginalComponent );

const testRenderer = TestRenderer.create(
<RegistryProvider value={ registry }>
<Component keyName="reactKey" />
<DataBoundComponent keyName="reactKey" />
</RegistryProvider>
);
const testInstance = testRenderer.root;
Expand Down Expand Up @@ -94,14 +95,14 @@ describe( 'withSelect', () => {
</button>
) );

const Component = compose( [
const DataBoundComponent = compose( [
withSelect( mapSelectToProps ),
withDispatch( mapDispatchToProps ),
] )( OriginalComponent );

const testRenderer = TestRenderer.create(
<RegistryProvider value={ registry }>
<Component />
<DataBoundComponent />
</RegistryProvider>
);
const testInstance = testRenderer.root;
Expand All @@ -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 <div>{ this.props.count }</div>;
}
}

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(
<RegistryProvider value={ registry }>
<DataBoundComponent />
</RegistryProvider>
);
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 ) => {
Expand All @@ -145,11 +206,11 @@ describe( 'withSelect', () => {
<div>{ props.count }</div>
) );

const Component = withSelect( mapSelectToProps )( OriginalComponent );
const DataBoundComponent = withSelect( mapSelectToProps )( OriginalComponent );

const testRenderer = TestRenderer.create(
<RegistryProvider value={ registry }>
<Component offset={ 0 } />
<DataBoundComponent offset={ 0 } />
</RegistryProvider>
);
const testInstance = testRenderer.root;
Expand All @@ -159,7 +220,7 @@ describe( 'withSelect', () => {

testRenderer.update(
<RegistryProvider value={ registry }>
<Component offset={ 10 } />
<DataBoundComponent offset={ 10 } />
</RegistryProvider>
);

Expand All @@ -180,11 +241,11 @@ describe( 'withSelect', () => {

const OriginalComponent = jest.fn().mockImplementation( () => <div /> );

const Component = compose( [
const DataBoundComponent = compose( [
withSelect( mapSelectToProps ),
] )( OriginalComponent );

const Parent = ( props ) => <Component propName={ props.propName } />;
const Parent = ( props ) => <DataBoundComponent propName={ props.propName } />;

const testRenderer = TestRenderer.create(
<RegistryProvider value={ registry }>
Expand Down Expand Up @@ -222,11 +283,11 @@ describe( 'withSelect', () => {

const OriginalComponent = jest.fn().mockImplementation( () => <div /> );

const Component = withSelect( mapSelectToProps )( OriginalComponent );
const DataBoundComponent = withSelect( mapSelectToProps )( OriginalComponent );

TestRenderer.create(
<RegistryProvider value={ registry }>
<Component />
<DataBoundComponent />
</RegistryProvider>
);

Expand All @@ -251,13 +312,13 @@ describe( 'withSelect', () => {

const OriginalComponent = jest.fn().mockImplementation( () => <div /> );

const Component = compose( [
const DataBoundComponent = compose( [
withSelect( mapSelectToProps ),
] )( OriginalComponent );

const testRenderer = TestRenderer.create(
<RegistryProvider value={ registry }>
<Component />
<DataBoundComponent />
</RegistryProvider>
);

Expand All @@ -266,7 +327,7 @@ describe( 'withSelect', () => {

testRenderer.update(
<RegistryProvider value={ registry }>
<Component propName="foo" />
<DataBoundComponent propName="foo" />
</RegistryProvider>
);

Expand All @@ -286,13 +347,13 @@ describe( 'withSelect', () => {

const OriginalComponent = jest.fn().mockImplementation( () => <div /> );

const Component = compose( [
const DataBoundComponent = compose( [
withSelect( mapSelectToProps ),
] )( OriginalComponent );

TestRenderer.create(
<RegistryProvider value={ registry }>
<Component />
<DataBoundComponent />
</RegistryProvider>
);

Expand Down Expand Up @@ -322,11 +383,11 @@ describe( 'withSelect', () => {
const OriginalComponent = jest.fn()
.mockImplementation( ( props ) => <div>{ JSON.stringify( props ) }</div> );

const Component = withSelect( mapSelectToProps )( OriginalComponent );
const DataBoundComponent = withSelect( mapSelectToProps )( OriginalComponent );

const testRenderer = TestRenderer.create(
<RegistryProvider value={ registry }>
<Component propName="foo" />
<DataBoundComponent propName="foo" />
</RegistryProvider>
);
const testInstance = testRenderer.root;
Expand All @@ -339,7 +400,7 @@ describe( 'withSelect', () => {

testRenderer.update(
<RegistryProvider value={ registry }>
<Component propName="bar" />
<DataBoundComponent propName="bar" />
</RegistryProvider>
);

Expand Down Expand Up @@ -369,11 +430,11 @@ describe( 'withSelect', () => {
( props ) => <div>{ props.count || 'Unknown' }</div>
) );

const Component = withSelect( mapSelectToProps )( OriginalComponent );
const DataBoundComponent = withSelect( mapSelectToProps )( OriginalComponent );

const testRenderer = TestRenderer.create(
<RegistryProvider value={ registry }>
<Component pass={ false } />
<DataBoundComponent pass={ false } />
</RegistryProvider>
);
const testInstance = testRenderer.root;
Expand All @@ -384,7 +445,7 @@ describe( 'withSelect', () => {

testRenderer.update(
<RegistryProvider value={ registry }>
<Component pass />
<DataBoundComponent pass />
</RegistryProvider>
);

Expand All @@ -394,7 +455,7 @@ describe( 'withSelect', () => {

testRenderer.update(
<RegistryProvider value={ registry }>
<Component pass={ false } />
<DataBoundComponent pass={ false } />
</RegistryProvider>
);

Expand Down Expand Up @@ -465,11 +526,11 @@ describe( 'withSelect', () => {
<div>{ props.value }</div>
) );

const Component = withSelect( mapSelectToProps )( OriginalComponent );
const DataBoundComponent = withSelect( mapSelectToProps )( OriginalComponent );

const testRenderer = TestRenderer.create(
<RegistryProvider value={ firstRegistry }>
<Component />
<DataBoundComponent />
</RegistryProvider>
);
const testInstance = testRenderer.root;
Expand All @@ -491,7 +552,7 @@ describe( 'withSelect', () => {

testRenderer.update(
<RegistryProvider value={ secondRegistry }>
<Component />
<DataBoundComponent />
</RegistryProvider>
);

Expand Down

0 comments on commit fba803e

Please sign in to comment.