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.

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

Try adding a pure Higher Order Component #6313

Merged
merged 3 commits into from
Apr 30, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
180 changes: 94 additions & 86 deletions data/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import memoize from 'memize';
/**
* WordPress dependencies
*/
import { Component, createHigherOrderComponent } from '@wordpress/element';
import { Component, createHigherOrderComponent, pure, compose } from '@wordpress/element';

/**
* Internal dependencies
Expand Down Expand Up @@ -241,67 +241,69 @@ export function dispatch( reducerKey ) {
*
* @return {Component} Enhanced component with merged state data props.
*/
export const withSelect = ( mapStateToProps ) => createHigherOrderComponent( ( WrappedComponent ) => {
return class ComponentWithSelect extends Component {
constructor() {
super( ...arguments );
export const withSelect = ( mapStateToProps ) => createHigherOrderComponent(
compose( [
pure,
( WrappedComponent ) => {
return class ComponentWithSelect extends Component {
constructor() {
super( ...arguments );

this.runSelection = this.runSelection.bind( this );
this.runSelection = this.runSelection.bind( this );

this.state = {};
}

shouldComponentUpdate( nextProps, nextState ) {
return ! isShallowEqual( nextProps, this.props ) || ! isShallowEqual( nextState, this.state );
}
this.state = {};
}

componentWillMount() {
this.subscribe();
componentWillMount() {
this.subscribe();

// Populate initial state.
this.runSelection();
}
// Populate initial state.
this.runSelection();
}

componentWillReceiveProps( nextProps ) {
if ( ! isShallowEqual( nextProps, this.props ) ) {
this.runSelection( nextProps );
}
}
componentWillReceiveProps( nextProps ) {
if ( ! isShallowEqual( nextProps, this.props ) ) {
this.runSelection( nextProps );
}
}

componentWillUnmount() {
this.unsubscribe();
componentWillUnmount() {
this.unsubscribe();

// While above unsubscribe avoids future listener calls, callbacks
// are snapshotted before being invoked, so if unmounting occurs
// during a previous callback, we need to explicitly track and
// avoid the `runSelection` that is scheduled to occur.
this.isUnmounting = true;
}
// While above unsubscribe avoids future listener calls, callbacks
// are snapshotted before being invoked, so if unmounting occurs
// during a previous callback, we need to explicitly track and
// avoid the `runSelection` that is scheduled to occur.
this.isUnmounting = true;
}

subscribe() {
this.unsubscribe = subscribe( this.runSelection );
}
subscribe() {
this.unsubscribe = subscribe( this.runSelection );
}

runSelection( props = this.props ) {
if ( this.isUnmounting ) {
return;
}
runSelection( props = this.props ) {
if ( this.isUnmounting ) {
return;
}

const { mergeProps } = this.state;
const nextMergeProps = mapStateToProps( select, props ) || {};
const { mergeProps } = this.state;
const nextMergeProps = mapStateToProps( select, props ) || {};

if ( ! isShallowEqual( nextMergeProps, mergeProps ) ) {
this.setState( {
mergeProps: nextMergeProps,
} );
}
}
if ( ! isShallowEqual( nextMergeProps, mergeProps ) ) {
this.setState( {
mergeProps: nextMergeProps,
} );
}
}

render() {
return <WrappedComponent { ...this.props } { ...this.state.mergeProps } />;
}
};
}, 'withSelect' );
render() {
return <WrappedComponent { ...this.props } { ...this.state.mergeProps } />;
}
};
},
] ),
'withSelect'
);

/**
* Higher-order component used to add dispatch props using registered action
Expand All @@ -315,48 +317,54 @@ export const withSelect = ( mapStateToProps ) => createHigherOrderComponent( ( W
*
* @return {Component} Enhanced component with merged dispatcher props.
*/
export const withDispatch = ( mapDispatchToProps ) => createHigherOrderComponent( ( WrappedComponent ) => {
return class ComponentWithDispatch extends Component {
constructor() {
super( ...arguments );

this.proxyProps = {};
}

componentWillMount() {
this.setProxyProps( this.props );
}
export const withDispatch = ( mapDispatchToProps ) => createHigherOrderComponent(
compose( [
pure,
( WrappedComponent ) => {
return class ComponentWithDispatch extends Component {
constructor() {
super( ...arguments );

this.proxyProps = {};
}

componentWillUpdate( nextProps ) {
this.setProxyProps( nextProps );
}
componentWillMount() {
this.setProxyProps( this.props );
}

proxyDispatch( propName, ...args ) {
// Original dispatcher is a pre-bound (dispatching) action creator.
mapDispatchToProps( dispatch, this.props )[ propName ]( ...args );
}
componentWillUpdate( nextProps ) {
this.setProxyProps( nextProps );
}

setProxyProps( props ) {
// Assign as instance property so that in reconciling subsequent
// renders, the assigned prop values are referentially equal.
const propsToDispatchers = mapDispatchToProps( dispatch, props );
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 ];
proxyDispatch( propName, ...args ) {
// Original dispatcher is a pre-bound (dispatching) action creator.
mapDispatchToProps( dispatch, this.props )[ propName ]( ...args );
}

return this.proxyDispatch.bind( this, propName );
} );
}
setProxyProps( props ) {
// Assign as instance property so that in reconciling subsequent
// renders, the assigned prop values are referentially equal.
const propsToDispatchers = mapDispatchToProps( dispatch, props );
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 );
} );
}

render() {
return <WrappedComponent { ...this.props } { ...this.proxyProps } />;
}
};
}, 'withDispatch' );
render() {
return <WrappedComponent { ...this.props } { ...this.proxyProps } />;
}
};
},
] ),
'withDispatch'
);

/**
* Returns true if the given argument appears to be a dispatchable action.
Expand Down
2 changes: 1 addition & 1 deletion editor/components/block-list/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -705,5 +705,5 @@ export default compose(
};
} ),
withFilters( 'editor.BlockListBlock' ),
withHoverAreas
withHoverAreas,
)( BlockListBlock );
32 changes: 32 additions & 0 deletions element/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
isString,
upperFirst,
} from 'lodash';
import isShallowEqual from 'shallowequal';

/**
* Internal dependencies
Expand Down Expand Up @@ -209,3 +210,34 @@ export function RawHTML( { children, ...props } ) {
...props,
} );
}

/**
* Given a component returns the enhanced component augmented with a component
* only rerendering when its props/state change
*
* @param {Function} mapComponentToEnhancedComponent Function mapping component
* to enhanced component.
* @param {string} modifierName Seed name from which to
* generated display name.
*
* @return {WPComponent} Component class with generated display name assigned.
*/
export const pure = createHigherOrderComponent( ( Wrapped ) => {
if ( Wrapped.prototype instanceof Component ) {
return class extends Wrapped {
shouldComponentUpdate( nextProps, nextState ) {
return ! isShallowEqual( nextProps, this.props ) || ! isShallowEqual( nextState, this.state );
}
};
}

return class extends Component {
shouldComponentUpdate( nextProps ) {
return ! isShallowEqual( nextProps, this.props );
Copy link
Member

Choose a reason for hiding this comment

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

Interesting conclusion that we don't need to compare state here, and I suppose it makes sense since it's wrapping the original (non-class) component.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed on this one, there is no state in functional components 👍

}

render() {
return <Wrapped { ...this.props } />;
}
};
}, 'pure' );
47 changes: 46 additions & 1 deletion element/test/index.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { shallow } from 'enzyme';
import { shallow, mount } from 'enzyme';

/**
* Internal dependencies
Expand All @@ -14,6 +14,7 @@ import {
renderToString,
switchChildrenNodeName,
RawHTML,
pure,
} from '../';

describe( 'element', () => {
Expand Down Expand Up @@ -212,4 +213,48 @@ describe( 'element', () => {
expect( element.prop( 'children' ) ).toBe( undefined );
} );
} );

describe( 'pure', () => {
it( 'functional component should rerender only when props change', () => {
let i = 0;
const MyComp = pure( () => {
return <p>{ ++i }</p>;
} );
const wrapper = mount( <MyComp /> );
wrapper.update(); // Updating with same props doesn't rerender
expect( wrapper.html() ).toBe( '<p>1</p>' );
wrapper.setProps( { prop: 'a' } ); // New prop should trigger a rerender
expect( wrapper.html() ).toBe( '<p>2</p>' );
wrapper.setProps( { prop: 'a' } ); // Keeping the same prop value should not rerender
expect( wrapper.html() ).toBe( '<p>2</p>' );
wrapper.setProps( { prop: 'b' } ); // Changing the prop value should rerender
expect( wrapper.html() ).toBe( '<p>3</p>' );
} );

it( 'class component should rerender if the props or state change', () => {
let i = 0;
const MyComp = pure( class extends Component {
constructor() {
super( ...arguments );
this.state = {};
}
render() {
return <p>{ ++i }</p>;
}
} );
const wrapper = mount( <MyComp /> );
wrapper.update(); // Updating with same props doesn't rerender
expect( wrapper.html() ).toBe( '<p>1</p>' );
wrapper.setProps( { prop: 'a' } ); // New prop should trigger a rerender
expect( wrapper.html() ).toBe( '<p>2</p>' );
wrapper.setProps( { prop: 'a' } ); // Keeping the same prop value should not rerender
expect( wrapper.html() ).toBe( '<p>2</p>' );
wrapper.setProps( { prop: 'b' } ); // Changing the prop value should rerender
expect( wrapper.html() ).toBe( '<p>3</p>' );
wrapper.setState( { state: 'a' } ); // New state value should trigger a rerender
expect( wrapper.html() ).toBe( '<p>4</p>' );
wrapper.setState( { state: 'a' } ); // Keeping the same state value should not trigger a rerender
expect( wrapper.html() ).toBe( '<p>4</p>' );
} );
} );
} );