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

Data Module: Introduce the "registry" concept #7527

Merged
merged 3 commits into from
Jul 12, 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
15 changes: 15 additions & 0 deletions packages/data/src/components/registry-provider/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/**
* WordPress dependencies
*/
import { createContext } from '@wordpress/element';

/**
* Internal dependencies
*/
import defaultRegistry from '../../default-registry';

const { Consumer, Provider } = createContext( defaultRegistry );

export const RegistryConsumer = Consumer;

export default Provider;
42 changes: 42 additions & 0 deletions packages/data/src/components/remountOnPropChange/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

This folder name should be kebab-case, remount-on-prop-change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I just noticed it too. This will be fixed in the @wordpress/compose PR I'm working on

* WordPress dependencies
*/
import { createHigherOrderComponent, Component } from '@wordpress/element';

/**
* Higher-order component creator, creating a new component that remounts
* the wrapped component each time a given prop value changes.
*
* @param {string} propName Prop name to monitor.
*
* @return {Function} Higher-order component.
*/
const remountOnPropChange = ( propName ) => createHigherOrderComponent(
( WrappedComponent ) => class extends Component {
constructor( props ) {
super( ...arguments );
this.state = {
propChangeId: 0,
propValue: props[ propName ],
};
}

static getDerivedStateFromProps( props, state ) {
if ( props[ propName ] === state.propValue ) {
return null;
}

return {
propChangeId: state.propChangeId + 1,
propValue: props[ propName ],
};
}

render() {
return <WrappedComponent key={ this.state.propChangeId } { ...this.props } />;
}
},
'remountOnPropChange'
);

export default remountOnPropChange;
71 changes: 71 additions & 0 deletions packages/data/src/components/remountOnPropChange/test/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/**
* External dependencies
*/
import TestRenderer from 'react-test-renderer';

/**
* WordPress dependencies
*/
import { Component } from '@wordpress/element';

/**
* Internal dependencies
*/
import remountOnPropChange from '../';

describe( 'remountOnPropChange', () => {
let count = 0;
class MountCounter extends Component {
constructor() {
super( ...arguments );
this.state = {
count: 0,
};
}

componentDidMount() {
count++;
this.setState( {
count: count,
} );
}

render() {
return this.state.count;
}
}

beforeEach( () => {
count = 0;
} );

it( 'Should not remount the inner component if the prop value doesn\'t change', () => {
const Wrapped = remountOnPropChange( 'monitor' )( MountCounter );
const testRenderer = TestRenderer.create(
<Wrapped monitor="unchanged" other="1" />
);

expect( testRenderer.toJSON() ).toBe( '1' );

// Changing an unmonitored prop
testRenderer.update(
<Wrapped monitor="unchanged" other="2" />
);
expect( testRenderer.toJSON() ).toBe( '1' );
} );

it( 'Should remount the inner component if the prop value changes', () => {
const Wrapped = remountOnPropChange( 'monitor' )( MountCounter );
const testRenderer = TestRenderer.create(
<Wrapped monitor="initial" />
);

expect( testRenderer.toJSON() ).toBe( '1' );

// Changing an the monitored prop remounts the component
testRenderer.update(
<Wrapped monitor="updated" />
);
expect( testRenderer.toJSON() ).toBe( '2' );
} );
} );
91 changes: 91 additions & 0 deletions packages/data/src/components/with-dispatch/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
/**
* External dependencies
*/
import { mapValues } from 'lodash';

/**
* WordPress dependencies
*/
import {
Component,
compose,
createHigherOrderComponent,
pure,
} from '@wordpress/element';

/**
* Internal dependencies
*/
import remountOnPropChange from '../remountOnPropChange';
import { RegistryConsumer } from '../registry-provider';

/**
* Higher-order component used to add dispatch props using registered action
* creators.
*
* @param {Object} mapDispatchToProps Object of prop names where value is a
* dispatch-bound action creator, or a
* function to be called with with the
* component's props and returning an
* action creator.
*
* @return {Component} Enhanced component with merged dispatcher props.
*/
const withDispatch = ( mapDispatchToProps ) => createHigherOrderComponent(
compose( [
pure,
( WrappedComponent ) => {
const ComponentWithDispatch = remountOnPropChange( 'registry' )( class extends Component {
constructor( props ) {
super( ...arguments );

this.proxyProps = {};
this.setProxyProps( props );
}

componentDidUpdate() {
this.setProxyProps( this.props );
}

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 reconciling subsequent
// renders, the assigned prop values are referentially equal.
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 );
} );
}

render() {
return <WrappedComponent { ...this.props.ownProps } { ...this.proxyProps } />;
}
} );

return ( ownProps ) => (
<RegistryConsumer>
{ ( registry ) => (
<ComponentWithDispatch
ownProps={ ownProps }
registry={ registry }
/>
) }
</RegistryConsumer>
);
},
] ),
'withDispatch'
);

export default withDispatch;
64 changes: 64 additions & 0 deletions packages/data/src/components/with-dispatch/test/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/**
* External dependencies
*/
import TestRenderer from 'react-test-renderer';

/**
* Internal dependencies
*/
import withDispatch from '../';
import { createRegistry } from '../../../registry';
import RegistryProvider from '../../registry-provider';

describe( 'withDispatch', () => {
let registry;
beforeEach( () => {
registry = createRegistry();
Copy link
Member

Choose a reason for hiding this comment

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

I love how this approach makes these unit tests not rely on any global state 😍

} );

it( 'passes the relevant data to the component', () => {
const store = registry.registerStore( 'counter', {
reducer: ( state = 0, action ) => {
if ( action.type === 'increment' ) {
return state + action.count;
}
return state;
},
actions: {
increment: ( count = 1 ) => ( { type: 'increment', count } ),
},
} );

const Component = withDispatch( ( _dispatch, ownProps ) => {
const { count } = ownProps;

return {
increment: () => _dispatch( 'counter' ).increment( count ),
};
} )( ( props ) => <button onClick={ props.increment } /> );

const testRenderer = TestRenderer.create(
Copy link
Member

@gziolo gziolo Jun 27, 2018

Choose a reason for hiding this comment

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

My own notes: to replace shallow from enzyme we would need only:

jest.shallow = ( element ) => {
    const TestRenderer  = require( 'react-test-renderer' );
    const testRenderer = TestRenderer.create( element );
    const testInstance = testRenderer.root;
    if ( testInstance !== null) { // not sure if it can be null?
        testInstance.update = testRenderer.update;
    }
    return testInstance;
}; 

Not that much of the work needed :)

Copy link
Member

Choose a reason for hiding this comment

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

Actually, it replaces mount as far as I can see. Interesting 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Would be good to explore removing enzyme in lieu of this approach in a separate PR. The lack of React 16 support is frustrating. My only concern is whether or not it makes our tests run significantly slower.

Copy link
Member

@gziolo gziolo Jun 27, 2018

Choose a reason for hiding this comment

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

Yes, definitely something we should explore further. Actually, enzyme brings lots of performance penalty on its own. See:
https://github.com/WordPress/packages/blob/master/packages/jest-preset-default/scripts/setup-test-framework.js#L3-L6
We lazy load it only when it is necessary to mitigate that :)
It isn't as much of an issue at the moment, but I could observe a huge difference when dealing with 1 000+ test suites in Calypso.

<RegistryProvider value={ registry }>
<Component count={ 0 } />
</RegistryProvider>
);
const testInstance = testRenderer.root;

const incrementBeforeSetProps = testInstance.findByType( 'button' ).props.onClick;

// Verify that dispatch respects props at the time of being invoked by
// changing props after the initial mount.
testRenderer.update(
<RegistryProvider value={ registry }>
<Component count={ 2 } />
</RegistryProvider>
);

// Function value reference should not have changed in props update.
expect( testInstance.findByType( 'button' ).props.onClick ).toBe( incrementBeforeSetProps );

incrementBeforeSetProps();

expect( store.getState() ).toBe( 2 );
} );
} );
Loading