Data: Refactor and optimize withSelect, withDispatch handling of regi…

…stry change (#11027)

* Data: Remove redundant remountOnPropChange from withDispatch

* Data: Use internal key generation utility for withSelect

* Compose: Deprecate remountOnPropChange

* Data: Handle registry change within withSelect

docs/reference/deprecated.md

@@ -1,9 +1,11 @@
 Gutenberg's deprecation policy is intended to support backwards-compatibility for releases, when possible. The current deprecations are listed below and are grouped by _the version at which they will be removed completely_. If your plugin depends on these behaviors, you must update to the recommended alternative before the noted version.
 
-# 4.4.0
- - `wp.date.getSettings` has been removed. Please use `wp.date.__experimentalGetSettings` instead.
+## 4.4.0
 
-# 4.3.0
+- `wp.date.getSettings` has been removed. Please use `wp.date.__experimentalGetSettings` instead.
+- `wp.compose.remountOnPropChange` has been removed.
+
+## 4.3.0
 
 - `isEditorSidebarPanelOpened` selector (`core/edit-post`) has been removed. Please use `isEditorPanelEnabled` instead.
 - `toggleGeneralSidebarEditorPanel` action (`core/edit-post`) has been removed. Please use `toggleEditorPanelOpened` instead.

lib/client-assets.php

@@ -269,7 +269,13 @@ function gutenberg_register_scripts_and_styles() {
 	gutenberg_override_script(
 		'wp-compose',
 		gutenberg_url( 'build/compose/index.js' ),
-		array( 'lodash', 'wp-element', 'wp-is-shallow-equal', 'wp-polyfill' ),
+		array(
+			'lodash',
+			'wp-deprecated',
+			'wp-element',
+			'wp-is-shallow-equal',
+			'wp-polyfill',
+		),
 		filemtime( gutenberg_dir_path() . 'build/compose/index.js' ),
 		true
 	);

packages/compose/CHANGELOG.md

@@ -1,3 +1,9 @@
+## 2.1.0 (Unreleased)
+
+### Deprecation
+
+- `remountOnPropChange` has been deprecated.
+
 ## 2.0.5 (2018-10-19)
 
 ## 2.0.4 (2018-10-18)

packages/compose/package.json

@@ -22,6 +22,7 @@
 	"react-native": "src/index",
 	"dependencies": {
 		"@babel/runtime": "^7.0.0",
+		"@wordpress/deprecated": "file:../deprecated",
 		"@wordpress/element": "file:../element",
 		"@wordpress/is-shallow-equal": "file:../is-shallow-equal",
 		"lodash": "^4.17.10"

packages/compose/src/remount-on-prop-change/index.js

@@ -2,6 +2,7 @@
  * WordPress dependencies
  */
 import { Component } from '@wordpress/element';
+import deprecated from '@wordpress/deprecated';
 
 /**
  * Internal dependencies
@@ -16,8 +17,13 @@ import createHigherOrderComponent from '../create-higher-order-component';
  *
  * @return {Function} Higher-order component.
  */
-const remountOnPropChange = ( propName ) => createHigherOrderComponent(
-	( WrappedComponent ) => class extends Component {
+const remountOnPropChange = ( propName ) => createHigherOrderComponent( ( WrappedComponent ) => {
+	deprecated( 'remountOnPropChange', {
+		plugin: 'Gutenberg',
+		version: '4.4.0',
+	} );
+
+	return class extends Component {
 		constructor( props ) {
 			super( ...arguments );
 			this.state = {
@@ -40,8 +46,7 @@ const remountOnPropChange = ( propName ) => createHigherOrderComponent(
 		render() {
 			return <WrappedComponent key={ this.state.propChangeId } { ...this.props } />;
 		}
-	},
-	'remountOnPropChange'
-);
+	};
+}, 'remountOnPropChange' );
 
 export default remountOnPropChange;

packages/compose/src/remount-on-prop-change/test/index.js

@@ -7,12 +7,15 @@ import TestRenderer from 'react-test-renderer';
  * WordPress dependencies
  */
 import { Component } from '@wordpress/element';
+import deprecated from '@wordpress/deprecated';
 
 /**
  * Internal dependencies
  */
 import remountOnPropChange from '../';
 
+jest.mock( '@wordpress/deprecated', () => jest.fn() );
+
 describe( 'remountOnPropChange', () => {
 	let count = 0;
 	class MountCounter extends Component {
@@ -52,6 +55,8 @@ describe( 'remountOnPropChange', () => {
 			<Wrapped monitor="unchanged" other="2" />
 		);
 		expect( testRenderer.toJSON() ).toBe( '1' );
+
+		expect( deprecated ).toHaveBeenCalled();
 	} );
 
 	it( 'Should remount the inner component if the prop value changes', () => {
@@ -67,5 +72,7 @@ describe( 'remountOnPropChange', () => {
 			<Wrapped monitor="updated" />
 		);
 		expect( testRenderer.toJSON() ).toBe( '2' );
+
+		expect( deprecated ).toHaveBeenCalled();
 	} );
 } );

packages/data/src/components/with-dispatch/index.js

@@ -7,7 +7,7 @@ import { mapValues } from 'lodash';
  * WordPress dependencies
  */
 import { Component } from '@wordpress/element';
-import { pure, compose, remountOnPropChange, createHigherOrderComponent } from '@wordpress/compose';
+import { pure, compose, createHigherOrderComponent } from '@wordpress/compose';
 
 /**
  * Internal dependencies
@@ -30,7 +30,7 @@ const withDispatch = ( mapDispatchToProps ) => createHigherOrderComponent(
 	compose( [
 		pure,
 		( WrappedComponent ) => {
-			const ComponentWithDispatch = remountOnPropChange( 'registry' )( class extends Component {
+			class ComponentWithDispatch extends Component {
 				constructor( props ) {
 					super( ...arguments );
 
@@ -48,8 +48,12 @@ const withDispatch = ( mapDispatchToProps ) => createHigherOrderComponent(
 				}
 
 				setProxyProps( props ) {
-					// Assign as instance property so that in reconciling subsequent
-					// renders, the assigned prop values are referentially equal.
+					// Assign as instance property so that in subsequent render
+					// reconciliation, the prop values are referentially equal.
+					// Importantly, note that while `mapDispatchToProps` is
+					// called, it is done only to determine the keys for which
+					// proxy functions should be created. The actual registry
+					// dispatch does not occur until the function is called.
 					const propsToDispatchers = mapDispatchToProps( this.props.registry.dispatch, props.ownProps );
 					this.proxyProps = mapValues( propsToDispatchers, ( dispatcher, propName ) => {
 						// Prebind with prop name so we have reference to the original
@@ -66,7 +70,7 @@ const withDispatch = ( mapDispatchToProps ) => createHigherOrderComponent(
 				render() {
 					return <WrappedComponent { ...this.props.ownProps } { ...this.proxyProps } />;
 				}
-			} );
+			}
 
 			return ( ownProps ) => (
 				<RegistryConsumer>

packages/data/src/components/with-dispatch/test/index.js

@@ -61,4 +61,59 @@ describe( 'withDispatch', () => {
 
 		expect( store.getState() ).toBe( 2 );
 	} );
+
+	it( 'calls dispatch on the correct registry if updated', () => {
+		const reducer = ( state = null ) => state;
+		const noop = () => ( { type: '__INERT__' } );
+		const firstRegistryAction = jest.fn().mockImplementation( noop );
+		const secondRegistryAction = jest.fn().mockImplementation( noop );
+
+		const firstRegistry = registry;
+		firstRegistry.registerStore( 'demo', {
+			reducer,
+			actions: {
+				noop: firstRegistryAction,
+			},
+		} );
+
+		const Component = withDispatch( ( _dispatch ) => {
+			const noopByReference = _dispatch( 'demo' ).noop;
+
+			return {
+				noop() {
+					_dispatch( 'demo' ).noop();
+					noopByReference();
+				},
+			};
+		} )( ( props ) => <button onClick={ props.noop } /> );
+
+		const testRenderer = TestRenderer.create(
+			<RegistryProvider value={ firstRegistry }>
+				<Component />
+			</RegistryProvider>
+		);
+		const testInstance = testRenderer.root;
+
+		testInstance.findByType( 'button' ).props.onClick();
+		expect( firstRegistryAction ).toHaveBeenCalledTimes( 2 );
+		expect( secondRegistryAction ).toHaveBeenCalledTimes( 0 );
+
+		const secondRegistry = createRegistry();
+		secondRegistry.registerStore( 'demo', {
+			reducer,
+			actions: {
+				noop: secondRegistryAction,
+			},
+		} );
+
+		testRenderer.update(
+			<RegistryProvider value={ secondRegistry }>
+				<Component />
+			</RegistryProvider>
+		);
+
+		testInstance.findByType( 'button' ).props.onClick();
+		expect( firstRegistryAction ).toHaveBeenCalledTimes( 2 );
+		expect( secondRegistryAction ).toHaveBeenCalledTimes( 2 );
+	} );
 } );

packages/data/src/components/with-select/index.js

@@ -3,7 +3,7 @@
  */
 import { Component } from '@wordpress/element';
 import isShallowEqual from '@wordpress/is-shallow-equal';
-import { remountOnPropChange, createHigherOrderComponent } from '@wordpress/compose';
+import { createHigherOrderComponent } from '@wordpress/compose';
 
 /**
  * Internal dependencies
@@ -44,11 +44,11 @@ const withSelect = ( mapSelectToProps ) => createHigherOrderComponent( ( Wrapped
 		);
 	}
 
-	const ComponentWithSelect = remountOnPropChange( 'registry' )( class extends Component {
+	class ComponentWithSelect extends Component {
 		constructor( props ) {
 			super( props );
 
-			this.subscribe();
+			this.subscribe( props.registry );
 
 			this.mergeProps = getNextMergeProps( props );
 		}
@@ -63,35 +63,48 @@ const withSelect = ( mapSelectToProps ) => createHigherOrderComponent( ( Wrapped
 		}
 
 		shouldComponentUpdate( nextProps, nextState ) {
-			const hasPropsChanged = ! isShallowEqual( this.props.ownProps, nextProps.ownProps );
+			// Cycle subscription if registry changes.
+			const hasRegistryChanged = nextProps.registry !== this.props.registry;
+			if ( hasRegistryChanged ) {
+				this.unsubscribe();
+				this.subscribe( nextProps.registry );
+			}
+
+			// Treat a registry change as equivalent to `ownProps`, to reflect
+			// `mergeProps` to rendered component if and only if updated.
+			const hasPropsChanged = (
+				hasRegistryChanged ||
+				! isShallowEqual( this.props.ownProps, nextProps.ownProps )
+			);
 
 			// Only render if props have changed or merge props have been updated
 			// from the store subscriber.
 			if ( this.state === nextState && ! hasPropsChanged ) {
 				return false;
 			}
 
-			// If merge props change as a result of the incoming props, they
-			// should be reflected as such in the upcoming render.
 			if ( hasPropsChanged ) {
 				const nextMergeProps = getNextMergeProps( nextProps );
 				if ( ! isShallowEqual( this.mergeProps, nextMergeProps ) ) {
-					// Side effects are typically discouraged in lifecycle methods, but
-					// this component is heavily used and this is the most performant
-					// code we've found thus far.
-					// Prior efforts to use `getDerivedStateFromProps` have demonstrated
-					// miserable performance.
+					// If merge props change as a result of the incoming props,
+					// they should be reflected as such in the upcoming render.
+					// While side effects are discouraged in lifecycle methods,
+					// this component is used heavily, and prior efforts to use
+					// `getDerivedStateFromProps` had demonstrated miserable
+					// performance.
 					this.mergeProps = nextMergeProps;
 				}
+
+				// Regardless whether merge props are changing, fall through to
+				// incur the render since the component will need to receive
+				// the changed `ownProps`.
 			}
 
 			return true;
 		}
 
-		subscribe() {
-			const { subscribe } = this.props.registry;
-
-			this.unsubscribe = subscribe( () => {
+		subscribe( registry ) {
+			this.unsubscribe = registry.subscribe( () => {
 				if ( ! this.canRunSelection ) {
 					return;
 				}
@@ -118,7 +131,7 @@ const withSelect = ( mapSelectToProps ) => createHigherOrderComponent( ( Wrapped
 		render() {
 			return <WrappedComponent { ...this.props.ownProps } { ...this.mergeProps } />;
 		}
-	} );
+	}
 
 	return ( ownProps ) => (
 		<RegistryConsumer>

packages/data/src/components/with-select/test/index.js

@@ -447,4 +447,59 @@ describe( 'withSelect', () => {
 		expect( ChildOriginalComponent ).toHaveBeenCalledTimes( 1 );
 		expect( ParentOriginalComponent ).toHaveBeenCalledTimes( 2 );
 	} );
+
+	it( 'should rerun selection on registry change', () => {
+		const firstRegistry = registry;
+		firstRegistry.registerStore( 'demo', {
+			reducer: ( state = 'first' ) => state,
+			selectors: {
+				getValue: ( state ) => state,
+			},
+		} );
+
+		const mapSelectToProps = jest.fn().mockImplementation( ( _select ) => ( {
+			value: _select( 'demo' ).getValue(),
+		} ) );
+
+		const OriginalComponent = jest.fn().mockImplementation( ( props ) => (
+			<div>{ props.value }</div>
+		) );
+
+		const Component = withSelect( mapSelectToProps )( OriginalComponent );
+
+		const testRenderer = TestRenderer.create(
+			<RegistryProvider value={ firstRegistry }>
+				<Component />
+			</RegistryProvider>
+		);
+		const testInstance = testRenderer.root;
+
+		expect( mapSelectToProps ).toHaveBeenCalledTimes( 1 );
+		expect( OriginalComponent ).toHaveBeenCalledTimes( 1 );
+
+		expect( testInstance.findByType( 'div' ).props ).toEqual( {
+			children: 'first',
+		} );
+
+		const secondRegistry = createRegistry();
+		secondRegistry.registerStore( 'demo', {
+			reducer: ( state = 'second' ) => state,
+			selectors: {
+				getValue: ( state ) => state,
+			},
+		} );
+
+		testRenderer.update(
+			<RegistryProvider value={ secondRegistry }>
+				<Component />
+			</RegistryProvider>
+		);
+
+		expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 );
+		expect( OriginalComponent ).toHaveBeenCalledTimes( 2 );
+
+		expect( testInstance.findByType( 'div' ).props ).toEqual( {
+			children: 'second',
+		} );
+	} );
 } );