Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data: Rerun selection if state changed during mount #11777

Merged
merged 1 commit into from
Nov 13, 2018
Merged

Data: Rerun selection if state changed during mount #11777

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/data/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Open in desktop
Original file line number Diff line number Diff line change
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 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subscribing in constructor can cause subscriptions to leak in two cases:

  • when server-side rendering, constructor is called, but componentWillUnmount never is.
  • in React Fiber concurrent mode, component instances can be constructed, but never actually committed to DOM. This can happen when Fiber is working through rendering a low-priority update (calling constructors and render methods) and suddenly a higher priority update is scheduled that invalidates the work that's been done. Then the unfinished low-priority work will be thrown away and restarted. In such a case, constructor is called, but the component(DidMount|WillUnmount) methods aren't.

The correct place to subscribe is therefore always componentDidMount.

Then, of course, we might miss a store update that happened between constructor and componentDidMount.

react-redux solves that by rerunning the selector (i.e., mapStateToProps) in componentDidMount and calling forceUpdate if anything changed. That's the stable v5.1.1.

In master, they have an unreleased beta version where the Redux provider and consumer are rewritten using the new React context API and where the connect-ed consumers don't subscribe to the store at all. Just the Provider does. That could be an inspiration for the @wordpress/data internals, too 🙂

Conclusion: this patch solves the bug that @Tug discovered, but other subtle issues are still present.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subscribing in the constructor is important for another use-case. It's important that the parent's component rerenders before its children do. For example when you remove a block, you don't want its inner components to rerender, you want the block list to rerender first to remove the block before the selectors of a removed block get called.

There's probably a better way to fix it but moving to componentDidMount causes the components to rerender in the wrong order because componentDidMount of children components runs before parents. I wonder how connect solves this.

Copy link
Contributor

@Tug Tug Nov 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a comment that I believe explains it: reduxjs/react-redux#1079 (comment)
Makes me wonder if we should use a per component listener?

Copy link
Member

@jsnajdr jsnajdr Nov 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In react-redux, nested connected components don't subscribe directly to the Redux store, but there is a nested hierarchy of Subscription objects and each child component subscribes to the parent's Subscription. Only components that don't have any connected parent subscribe directly to the store. Then the Subscription object makes sure that the listener of its owner is called first, and the listeners of the children are called after it. Something like this:

constructor() {
  this.subscription = new Subscription();
  // `this` is always the first subscriber
  this.subscription.subscribe( this.onStateChange ); 
}

componentDidMount() {
  this.subscription.subscribeToParent();
}

componentDidUnmount() {
  this.subscription.unsubscribeFromParent();
}

The parent subscription object is passed down to children in context. And then each child in turn passes a different parent subscription to its children.

It's very clever and satisfies all the subtle requirements: no leaks, parent listener called before children...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be fine with something like this if proposed, seems like a good enhancement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest React Redux 6 beta is even better. It uses React.createContext and only the Provider subscribes to anything. The connected components receive the latest Redux state through the context and rerender if anything relevant changed. Parent component receives the updated context before children do. Everything is much simpler and it just works.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm interested in exploring further enhancements, and have a few ideas floating in my head related to what's been discussed here. To clarify, do these concerns impact whether this should be merged as-is?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, do these concerns impact whether this should be merged as-is?

No. As far as I know, this PR makes things better and doesn't make anything worse 🚢

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, great. And thanks for the additional context. While I've done my fair share of scouring react-redux source, in how you've laid it out here, there seems to be much we could still do to improve. I'll plan to dig deeper very shortly 🙂


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
Open in desktop
Original file line number Diff line number Diff line change
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