-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
/** | ||
* 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; |
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' ); | ||
} ); | ||
} ); |
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; |
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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My own notes: to replace 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 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, it replaces There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, definitely something we should explore further. Actually, |
||
<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 ); | ||
} ); | ||
} ); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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