Data: Avoid responding to componentDidUpdate in withDispatch

.eslintrc.js

@@ -165,6 +165,10 @@ module.exports = {
 				selector: 'CallExpression[callee.object.name="Math"][callee.property.name="random"]',
 				message: 'Do not use Math.random() to generate unique IDs; use withInstanceId instead. (If you’re not generating unique IDs: ignore this message.)',
 			},
+			{
+				selector: 'CallExpression[callee.name="withDispatch"] > :function > BlockStatement > :not(VariableDeclaration,ReturnStatement)',
+				message: 'withDispatch must return an object with consistent keys. Avoid performing logic in `mapDispatchToProps`.',
+			},
 		],
 		'react/forbid-elements': [ 'error', {
 			forbid: [

packages/data/README.md

@@ -286,6 +286,8 @@ const SaleButton = withDispatch( ( dispatch, ownProps ) => {
 //  <SaleButton>Start Sale!</SaleButton>
 ```
 
+*Note:* It is important that the `mapDispatchToProps` function always returns an object with the same keys. For example, it should not contain conditions under which a different value would be returned.
+
 ## Generic Stores
 
 The `@wordpress/data` module offers a more advanced and generic interface for the purposes of integrating other data systems and situations where more direct control over a data system is needed. In this case, a data store will need to be implemented outside of `@wordpress/data` and then plugged in via three functions:

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, createHigherOrderComponent } from '@wordpress/compose';
+import { createHigherOrderComponent } from '@wordpress/compose';
 
 /**
  * Internal dependencies
@@ -27,63 +27,57 @@ import { RegistryConsumer } from '../registry-provider';
  * @return {Component} Enhanced component with merged dispatcher props.
  */
 const withDispatch = ( mapDispatchToProps ) => createHigherOrderComponent(
-	compose( [
-		pure,
-		( WrappedComponent ) => {
-			class ComponentWithDispatch extends Component {
-				constructor( props ) {
-					super( ...arguments );
+	( WrappedComponent ) => {
+		class ComponentWithDispatch extends Component {
+			constructor( props ) {
+				super( ...arguments );
 
-					this.proxyProps = {};
-					this.setProxyProps( props );
-				}
+				this.proxyProps = {};
 
-				componentDidUpdate() {
-					this.setProxyProps( this.props );
-				}
+				this.setProxyProps( props );
+			}
 
-				proxyDispatch( propName, ...args ) {
-					// Original dispatcher is a pre-bound (dispatching) action creator.
-					mapDispatchToProps( this.props.registry.dispatch, this.props.ownProps )[ propName ]( ...args );
-				}
+			proxyDispatch( propName, ...args ) {
+				// Original dispatcher is a pre-bound (dispatching) action creator.
+				mapDispatchToProps( this.props.registry.dispatch, this.props.ownProps )[ propName ]( ...args );
+			}
 
-				setProxyProps( props ) {
-					// 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
-						// dispatcher to invoke. Track between re-renders to avoid
-						// creating new function references every render.
-						if ( this.proxyProps.hasOwnProperty( propName ) ) {
-							return this.proxyProps[ propName ];
-						}
+			setProxyProps( props ) {
+				// 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
+					// dispatcher to invoke. Track between re-renders to avoid
+					// creating new function references every render.
+					if ( this.proxyProps.hasOwnProperty( propName ) ) {
+						return this.proxyProps[ propName ];
+					}
 
-						return this.proxyDispatch.bind( this, propName );
-					} );
-				}
+					return this.proxyDispatch.bind( this, propName );
+				} );
+			}
 
-				render() {
-					return <WrappedComponent { ...this.props.ownProps } { ...this.proxyProps } />;
-				}
+			render() {
+				return <WrappedComponent { ...this.props.ownProps } { ...this.proxyProps } />;
 			}
+		}
 
-			return ( ownProps ) => (
-				<RegistryConsumer>
-					{ ( registry ) => (
-						<ComponentWithDispatch
-							ownProps={ ownProps }
-							registry={ registry }
-						/>
-					) }
-				</RegistryConsumer>
-			);
-		},
-	] ),
+		return ( ownProps ) => (
+			<RegistryConsumer>
+				{ ( registry ) => (
+					<ComponentWithDispatch
+						ownProps={ ownProps }
+						registry={ registry }
+					/>
+				) }
+			</RegistryConsumer>
+		);
+	},
 	'withDispatch'
 );
 

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

@@ -115,11 +115,10 @@ describe( 'withSelect', () => {
 		testInstance.findByType( 'button' ).props.onClick();
 
 		expect( testInstance.findByType( 'button' ).props.children ).toBe( 1 );
-		// 3 times =
+		// 2 times =
 		//  1. Initial mount
 		//  2. When click handler is called
-		//  3. After select updates its merge props
-		expect( mapDispatchToProps ).toHaveBeenCalledTimes( 3 );
+		expect( mapDispatchToProps ).toHaveBeenCalledTimes( 2 );
 		expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 );
 		expect( OriginalComponent ).toHaveBeenCalledTimes( 2 );
 	} );