diff --git a/packages/data/CHANGELOG.md b/packages/data/CHANGELOG.md index 533101d02e8a8..391d68abe4c46 100644 --- a/packages/data/CHANGELOG.md +++ b/packages/data/CHANGELOG.md @@ -1,3 +1,13 @@ +## Master + +### New Feature + +- Expose `useSelect` hook for usage in functional components. ([#15737](https://github.com/WordPress/gutenberg/pull/15737)) + +### Enhancements + +- `withSelect` internally uses the new `useSelect` hook. ([#15737](https://github.com/WordPress/gutenberg/pull/15737). **Note:** This _could_ impact performance of code using `withSelect` in edge-cases. To avoid impact, memoize passed in `mapSelectToProps` callbacks or implement `useSelect` directly with dependencies. + ## 4.5.0 (2019-05-21) ### Bug Fix diff --git a/packages/data/README.md b/packages/data/README.md index 65b525d404099..8984e86b1c577 100644 --- a/packages/data/README.md +++ b/packages/data/README.md @@ -379,11 +379,43 @@ _Returns_ # **RegistryConsumer** -Undocumented declaration. +A custom react Context consumer exposing the provided `registry` to +children components. Used along with the RegistryProvider. + +You can read more about the react context api here: + + +_Usage_ + +````js +const { + RegistryProvider, + RegistryConsumer, + createRegistry +} = wp.data; + +const registry = createRegistry( {} ); + +const App = ( { props } ) => { + return +
Hello There
+ + { ( registry ) => ( + + +} # **RegistryProvider** -Undocumented declaration. +A custom Context provider for exposing the provided `registry` to children +components via a consumer. + +See RegistryConsumer documentation for +example. # **select** @@ -391,13 +423,13 @@ Given the name of a registered store, returns an object of the store's selectors The selector functions are been pre-bound to pass the current state automatically. As a consumer, you need only pass arguments of the selector, if applicable. -_Usage_ +*Usage* ```js const { select } = wp.data; select( 'my-shop' ).getPrice( 'hammer' ); -``` +```` _Parameters_ @@ -435,6 +467,91 @@ _Parameters_ Undocumented declaration. +# **useRegistry** + +A custom react hook exposing the registry context for use. + +This exposes the `registry` value provided via the +Registry Provider to a component implementing +this hook. + +It acts similarly to the `useContext` react hook. + +Note: Generally speaking, `useRegistry` is a low level hook that in most cases +won't be needed for implementation. Most interactions with the wp.data api +can be performed via the `useSelect` hook, or the `withSelect` and +`withDispatch` higher order components. + +_Usage_ + +```js +const { + RegistryProvider, + createRegistry, + useRegistry, +} = wp.data + +const registry = createRegistry( {} ); + +const SomeChildUsingRegistry = ( props ) => { + const registry = useRegistry( registry ); + // ...logic implementing the registry in other react hooks. +}; + + +const ParentProvidingRegistry = ( props ) => { + return + + +}; +``` + +_Returns_ + +- `Function`: A custom react hook exposing the registry context value. + +# **useSelect** + +Custom react hook for retrieving props from registered selectors. + +In general, this custom React hook follows the +[rules of hooks](https://reactjs.org/docs/hooks-rules.html). + +_Usage_ + +```js +const { useSelect } = wp.data; + +function HammerPriceDisplay( { currency } ) { + const price = useSelect( ( select ) => { + return select( 'my-shop' ).getPrice( 'hammer', currency ) + }, [ currency ] ); + return new Intl.NumberFormat( 'en-US', { + style: 'currency', + currency, + } ).format( price ); +} + +// Rendered in the application: +// +``` + +In the above example, when `HammerPriceDisplay` is rendered into an +application, the price will be retrieved from the store state using the +`mapSelect` callback on `useSelect`. If the currency prop changes then +any price in the state for that currency is retrieved. If the currency prop +doesn't change and other props are passed in that do change, the price will +not change because the dependency is just the currency. + +_Parameters_ + +- _\_mapSelect_ `Function`: Function called on every state change. The returned value is exposed to the component implementing this hook. The function receives the `registry.select` method on the first argument and the `registry` on the second argument. +- _deps_ `Array`: If provided, this memoizes the mapSelect so the same `mapSelect` is invoked on every state change unless the dependencies change. + +_Returns_ + +- `Function`: A custom react hook. + # **withDispatch** Higher-order component used to add dispatch props using registered action creators. @@ -545,7 +662,10 @@ const HammerPriceDisplay = withSelect( ( select, ownProps ) => { // ``` -In the above example, when `HammerPriceDisplay` is rendered into an application, it will pass the price into the underlying `PriceDisplay` component and update automatically if the price of a hammer ever changes in the store. +In the above example, when `HammerPriceDisplay` is rendered into an +application, it will pass the price into the underlying `PriceDisplay` +component and update automatically if the price of a hammer ever changes in +the store. _Parameters_ diff --git a/packages/data/src/components/async-mode-provider/context.js b/packages/data/src/components/async-mode-provider/context.js new file mode 100644 index 0000000000000..c0e59ef713818 --- /dev/null +++ b/packages/data/src/components/async-mode-provider/context.js @@ -0,0 +1,12 @@ +/** + * WordPress dependencies + */ +import { createContext } from '@wordpress/element'; + +export const Context = createContext( false ); + +const { Consumer, Provider } = Context; + +export const AsyncModeConsumer = Consumer; + +export default Provider; diff --git a/packages/data/src/components/async-mode-provider/index.js b/packages/data/src/components/async-mode-provider/index.js index 30e877d0f1ed3..778dce8fc6eb2 100644 --- a/packages/data/src/components/async-mode-provider/index.js +++ b/packages/data/src/components/async-mode-provider/index.js @@ -1,10 +1,2 @@ -/** - * WordPress dependencies - */ -import { createContext } from '@wordpress/element'; - -const { Consumer, Provider } = createContext( false ); - -export const AsyncModeConsumer = Consumer; - -export default Provider; +export { default as useAsyncMode } from './use-async-mode'; +export { default as AsyncModeProvider, AsyncModeConsumer } from './context'; diff --git a/packages/data/src/components/async-mode-provider/use-async-mode.js b/packages/data/src/components/async-mode-provider/use-async-mode.js new file mode 100644 index 0000000000000..2f53c34ae2e07 --- /dev/null +++ b/packages/data/src/components/async-mode-provider/use-async-mode.js @@ -0,0 +1,13 @@ +/** + * WordPress dependencies + */ +import { useContext } from '@wordpress/element'; + +/** + * Internal dependencies + */ +import { Context } from './context'; + +export default function useAsyncMode() { + return useContext( Context ); +} diff --git a/packages/data/src/components/registry-provider/context.js b/packages/data/src/components/registry-provider/context.js new file mode 100644 index 0000000000000..a1fe1a9e56183 --- /dev/null +++ b/packages/data/src/components/registry-provider/context.js @@ -0,0 +1,54 @@ +/** + * WordPress dependencies + */ +import { createContext } from '@wordpress/element'; + +/** + * Internal dependencies + */ +import defaultRegistry from '../../default-registry'; + +export const Context = createContext( defaultRegistry ); + +const { Consumer, Provider } = Context; + +/** + * A custom react Context consumer exposing the provided `registry` to + * children components. Used along with the RegistryProvider. + * + * You can read more about the react context api here: + * https://reactjs.org/docs/context.html#contextprovider + * + * @example + * ```js + * const { + * RegistryProvider, + * RegistryConsumer, + * createRegistry + * } = wp.data; + * + * const registry = createRegistry( {} ); + * + * const App = ( { props } ) => { + * return + *
Hello There
+ * + * { ( registry ) => ( + * + * + * } + */ +export const RegistryConsumer = Consumer; + +/** + * A custom Context provider for exposing the provided `registry` to children + * components via a consumer. + * + * See RegistryConsumer documentation for + * example. + */ +export default Provider; diff --git a/packages/data/src/components/registry-provider/index.js b/packages/data/src/components/registry-provider/index.js index c296f46a1ab1a..e4f8d99bec597 100644 --- a/packages/data/src/components/registry-provider/index.js +++ b/packages/data/src/components/registry-provider/index.js @@ -1,15 +1,2 @@ -/** - * 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; +export { default as RegistryProvider, RegistryConsumer } from './context'; +export { default as useRegistry } from './use-registry'; diff --git a/packages/data/src/components/registry-provider/use-registry.js b/packages/data/src/components/registry-provider/use-registry.js new file mode 100644 index 0000000000000..846cb5627fd2f --- /dev/null +++ b/packages/data/src/components/registry-provider/use-registry.js @@ -0,0 +1,52 @@ +/** + * WordPress dependencies + */ +import { useContext } from '@wordpress/element'; + +/** + * Internal dependencies + */ +import { Context } from './context'; + +/** + * A custom react hook exposing the registry context for use. + * + * This exposes the `registry` value provided via the + * Registry Provider to a component implementing + * this hook. + * + * It acts similarly to the `useContext` react hook. + * + * Note: Generally speaking, `useRegistry` is a low level hook that in most cases + * won't be needed for implementation. Most interactions with the wp.data api + * can be performed via the `useSelect` hook, or the `withSelect` and + * `withDispatch` higher order components. + * + * @example + * ```js + * const { + * RegistryProvider, + * createRegistry, + * useRegistry, + * } = wp.data + * + * const registry = createRegistry( {} ); + * + * const SomeChildUsingRegistry = ( props ) => { + * const registry = useRegistry( registry ); + * // ...logic implementing the registry in other react hooks. + * }; + * + * + * const ParentProvidingRegistry = ( props ) => { + * return + * + * + * }; + * ``` + * + * @return {Function} A custom react hook exposing the registry context value. + */ +export default function useRegistry() { + return useContext( Context ); +} diff --git a/packages/data/src/components/use-select/index.js b/packages/data/src/components/use-select/index.js new file mode 100644 index 0000000000000..d71f72b21c917 --- /dev/null +++ b/packages/data/src/components/use-select/index.js @@ -0,0 +1,167 @@ +/** + * WordPress dependencies + */ +import { createQueue } from '@wordpress/priority-queue'; +import { + useLayoutEffect, + useRef, + useMemo, + useCallback, + useEffect, + useReducer, +} from '@wordpress/element'; +import { isShallowEqualObjects } from '@wordpress/is-shallow-equal'; + +/** + * Internal dependencies + */ +import useRegistry from '../registry-provider/use-registry'; +import useAsyncMode from '../async-mode-provider/use-async-mode'; + +/** + * Favor useLayoutEffect to ensure the store subscription callback always has + * the selector from the latest render. If a store update happens between render + * and the effect, this could cause missed/stale updates or inconsistent state. + * + * Fallback to useEffect for server rendered components because currently React + * throws a warning when using useLayoutEffect in that environment. + */ +const useIsomorphicLayoutEffect = + typeof window !== 'undefined' ? useLayoutEffect : useEffect; + +const renderQueue = createQueue(); + +/** + * Custom react hook for retrieving props from registered selectors. + * + * In general, this custom React hook follows the + * [rules of hooks](https://reactjs.org/docs/hooks-rules.html). + * + * @param {Function} _mapSelect Function called on every state change. The + * returned value is exposed to the component + * implementing this hook. The function receives + * the `registry.select` method on the first + * argument and the `registry` on the second + * argument. + * @param {Array} deps If provided, this memoizes the mapSelect so the + * same `mapSelect` is invoked on every state + * change unless the dependencies change. + * + * @example + * ```js + * const { useSelect } = wp.data; + * + * function HammerPriceDisplay( { currency } ) { + * const price = useSelect( ( select ) => { + * return select( 'my-shop' ).getPrice( 'hammer', currency ) + * }, [ currency ] ); + * return new Intl.NumberFormat( 'en-US', { + * style: 'currency', + * currency, + * } ).format( price ); + * } + * + * // Rendered in the application: + * // + * ``` + * + * In the above example, when `HammerPriceDisplay` is rendered into an + * application, the price will be retrieved from the store state using the + * `mapSelect` callback on `useSelect`. If the currency prop changes then + * any price in the state for that currency is retrieved. If the currency prop + * doesn't change and other props are passed in that do change, the price will + * not change because the dependency is just the currency. + * + * @return {Function} A custom react hook. + */ +export default function useSelect( _mapSelect, deps ) { + const mapSelect = useCallback( _mapSelect, deps ); + const registry = useRegistry(); + const isAsync = useAsyncMode(); + const queueContext = useMemo( () => ( { queue: true } ), [ registry ] ); + const [ , forceRender ] = useReducer( ( s ) => s + 1, 0 ); + + const latestMapSelect = useRef(); + const latestIsAsync = useRef( isAsync ); + const latestMapOutput = useRef(); + const latestMapOutputError = useRef(); + const isMounted = useRef(); + + let mapOutput; + + try { + if ( + latestMapSelect.current !== mapSelect || + latestMapOutputError.current + ) { + mapOutput = mapSelect( registry.select, registry ); + } else { + mapOutput = latestMapOutput.current; + } + } catch ( error ) { + let errorMessage = `An error occurred while running 'mapSelect': ${ error.message }`; + + if ( latestMapOutputError.current ) { + errorMessage += `\nThe error may be correlated with this previous error:\n`; + errorMessage += `${ latestMapOutputError.current.stack }\n\n`; + errorMessage += 'Original stack trace:'; + + throw new Error( errorMessage ); + } + } + + useIsomorphicLayoutEffect( () => { + latestMapSelect.current = mapSelect; + if ( latestIsAsync.current !== isAsync ) { + latestIsAsync.current = isAsync; + renderQueue.flush( queueContext ); + } + latestMapOutput.current = mapOutput; + latestMapOutputError.current = undefined; + isMounted.current = true; + } ); + + useIsomorphicLayoutEffect( () => { + const onStoreChange = () => { + if ( isMounted.current ) { + try { + const newMapOutput = latestMapSelect.current( + registry.select, + registry + ); + if ( isShallowEqualObjects( latestMapOutput.current, newMapOutput ) ) { + return; + } + latestMapOutput.current = newMapOutput; + } catch ( error ) { + latestMapOutputError.current = error; + } + forceRender( {} ); + } + }; + + // catch any possible state changes during mount before the subscription + // could be set. + if ( latestIsAsync.current ) { + renderQueue.add( queueContext, onStoreChange ); + } else { + onStoreChange(); + } + + const unsubscribe = registry.subscribe( () => { + if ( latestIsAsync.current ) { + renderQueue.add( queueContext, onStoreChange ); + } else { + onStoreChange(); + } + } ); + + return () => { + isMounted.current = false; + unsubscribe(); + renderQueue.flush( queueContext ); + }; + }, [ registry ] ); + + return mapOutput; +} diff --git a/packages/data/src/components/use-select/test/index.js b/packages/data/src/components/use-select/test/index.js new file mode 100644 index 0000000000000..ce8d15f3aa070 --- /dev/null +++ b/packages/data/src/components/use-select/test/index.js @@ -0,0 +1,136 @@ +/** + * External dependencies + */ +import TestRenderer, { act } from 'react-test-renderer'; + +/** + * Internal dependencies + */ +import { createRegistry } from '../../../registry'; +import { RegistryProvider } from '../../registry-provider'; +import useSelect from '../index'; + +describe( 'useSelect', () => { + let registry; + beforeEach( () => { + registry = createRegistry(); + } ); + + const getTestComponent = ( mapSelectSpy, dependencyKey ) => ( props ) => { + const dependencies = props[ dependencyKey ]; + mapSelectSpy.mockImplementation( + ( select ) => ( { + results: select( 'testStore' ).testSelector( props.keyName ), + } ) + ); + const data = useSelect( mapSelectSpy, [ dependencies ] ); + return
{ data.results }
; + }; + + it( 'passes the relevant data to the component', () => { + registry.registerStore( 'testStore', { + reducer: () => ( { foo: 'bar' } ), + selectors: { + testSelector: ( state, key ) => state[ key ], + }, + } ); + const selectSpy = jest.fn(); + const TestComponent = jest.fn().mockImplementation( + getTestComponent( selectSpy, 'keyName' ) + ); + let renderer; + act( () => { + renderer = TestRenderer.create( + + + + ); + } ); + const testInstance = renderer.root; + // 2 times expected + // - 1 for initial mount + // - 1 for after mount before subscription set. + expect( selectSpy ).toHaveBeenCalledTimes( 2 ); + expect( TestComponent ).toHaveBeenCalledTimes( 1 ); + + // ensure expected state was rendered + expect( testInstance.findByType( 'div' ).props ).toEqual( { + children: 'bar', + } ); + } ); + + it( 'uses memoized selector if dependencies do not change', () => { + registry.registerStore( 'testStore', { + reducer: () => ( { foo: 'bar' } ), + selectors: { + testSelector: ( state, key ) => state[ key ], + }, + } ); + + const selectSpyFoo = jest.fn().mockImplementation( () => 'foo' ); + const selectSpyBar = jest.fn().mockImplementation( () => 'bar' ); + const TestComponent = jest.fn().mockImplementation( + ( props ) => { + const mapSelect = props.change ? selectSpyFoo : selectSpyBar; + const data = useSelect( mapSelect, [ props.keyName ] ); + return
{ data }
; + } + ); + let renderer; + act( () => { + renderer = TestRenderer.create( + + + + ); + } ); + const testInstance = renderer.root; + + expect( selectSpyFoo ).toHaveBeenCalledTimes( 2 ); + expect( selectSpyBar ).toHaveBeenCalledTimes( 0 ); + expect( TestComponent ).toHaveBeenCalledTimes( 1 ); + + // ensure expected state was rendered + expect( testInstance.findByType( 'div' ).props ).toEqual( { + children: 'foo', + } ); + + //rerender with non dependency changed + act( () => { + renderer.update( + + + + ); + } ); + + expect( selectSpyFoo ).toHaveBeenCalledTimes( 2 ); + expect( selectSpyBar ).toHaveBeenCalledTimes( 0 ); + expect( TestComponent ).toHaveBeenCalledTimes( 2 ); + + // ensure expected state was rendered + expect( testInstance.findByType( 'div' ).props ).toEqual( { + children: 'foo', + } ); + + // rerender with dependency changed + // rerender with non dependency changed + act( () => { + renderer.update( + + + + ); + } ); + + expect( selectSpyFoo ).toHaveBeenCalledTimes( 2 ); + expect( selectSpyBar ).toHaveBeenCalledTimes( 1 ); + expect( TestComponent ).toHaveBeenCalledTimes( 3 ); + + // ensure expected state was rendered + expect( testInstance.findByType( 'div' ).props ).toEqual( { + children: 'bar', + } ); + } ); +} ); + diff --git a/packages/data/src/components/with-dispatch/test/index.js b/packages/data/src/components/with-dispatch/test/index.js index 98f225fad2c40..4bde9b30810ac 100644 --- a/packages/data/src/components/with-dispatch/test/index.js +++ b/packages/data/src/components/with-dispatch/test/index.js @@ -8,7 +8,7 @@ import TestRenderer from 'react-test-renderer'; */ import withDispatch from '../'; import { createRegistry } from '../../../registry'; -import RegistryProvider from '../../registry-provider'; +import { RegistryProvider } from '../../registry-provider'; describe( 'withDispatch', () => { let registry; diff --git a/packages/data/src/components/with-select/index.js b/packages/data/src/components/with-select/index.js index d2fd4ae462b3b..f9659e7e865a8 100644 --- a/packages/data/src/components/with-select/index.js +++ b/packages/data/src/components/with-select/index.js @@ -1,18 +1,12 @@ /** * WordPress dependencies */ -import { Component } from '@wordpress/element'; -import { isShallowEqualObjects } from '@wordpress/is-shallow-equal'; -import { createHigherOrderComponent } from '@wordpress/compose'; -import { createQueue } from '@wordpress/priority-queue'; +import { createHigherOrderComponent, pure } from '@wordpress/compose'; /** * Internal dependencies */ -import { RegistryConsumer } from '../registry-provider'; -import { AsyncModeConsumer } from '../async-mode-provider'; - -const renderQueue = createQueue(); +import useSelect from '../use-select'; /** * Higher-order component used to inject state-derived props using registered @@ -46,163 +40,27 @@ const renderQueue = createQueue(); * // * // * ``` - * In the above example, when `HammerPriceDisplay` is rendered into an application, it will pass the price into the underlying `PriceDisplay` component and update automatically if the price of a hammer ever changes in the store. + * In the above example, when `HammerPriceDisplay` is rendered into an + * application, it will pass the price into the underlying `PriceDisplay` + * component and update automatically if the price of a hammer ever changes in + * the store. * * @return {Component} Enhanced component with merged state data props. */ -const withSelect = ( mapSelectToProps ) => createHigherOrderComponent( ( WrappedComponent ) => { - /** - * Default merge props. A constant value is used as the fallback since it - * can be more efficiently shallow compared in case component is repeatedly - * rendered without its own merge props. - * - * @type {Object} - */ - const DEFAULT_MERGE_PROPS = {}; - - /** - * Given a props object, returns the next merge props by mapSelectToProps. - * - * @param {Object} props Props to pass as argument to mapSelectToProps. - * - * @return {Object} Props to merge into rendered wrapped element. - */ - function getNextMergeProps( props ) { - return ( - mapSelectToProps( props.registry.select, props.ownProps, props.registry ) || - DEFAULT_MERGE_PROPS - ); - } - - class ComponentWithSelect extends Component { - constructor( props ) { - super( props ); - - this.onStoreChange = this.onStoreChange.bind( this ); - - this.subscribe( props.registry ); - - this.mergeProps = getNextMergeProps( props ); - } - - componentDidMount() { - this.canRunSelection = true; - - // A state change may have occurred between the constructor and - // mount of the component (e.g. during the wrapped component's own - // constructor), in which case selection should be rerun. - if ( this.hasQueuedSelection ) { - this.hasQueuedSelection = false; - this.onStoreChange(); - } - } - - componentWillUnmount() { - this.canRunSelection = false; - this.unsubscribe(); - renderQueue.flush( this ); - } - - shouldComponentUpdate( nextProps, nextState ) { - // Cycle subscription if registry changes. - const hasRegistryChanged = nextProps.registry !== this.props.registry; - const hasSyncRenderingChanged = nextProps.isAsync !== this.props.isAsync; - - if ( hasRegistryChanged ) { - this.unsubscribe(); - this.subscribe( nextProps.registry ); - } - - if ( hasSyncRenderingChanged ) { - renderQueue.flush( this ); - } - - // Treat a registry change as equivalent to `ownProps`, to reflect - // `mergeProps` to rendered component if and only if updated. - const hasPropsChanged = ( - hasRegistryChanged || - ! isShallowEqualObjects( 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 && ! hasSyncRenderingChanged ) { - return false; - } - - if ( hasPropsChanged || hasSyncRenderingChanged ) { - const nextMergeProps = getNextMergeProps( nextProps ); - if ( ! isShallowEqualObjects( this.mergeProps, nextMergeProps ) ) { - // 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; +const withSelect = ( mapSelectToProps ) => createHigherOrderComponent( + ( WrappedComponent ) => pure( + ( ownProps ) => { + const mapSelect = + ( select, registry ) => mapSelectToProps( + select, + ownProps, + registry + ); + const mergeProps = useSelect( mapSelect ); + return ; } - - onStoreChange() { - if ( ! this.canRunSelection ) { - this.hasQueuedSelection = true; - return; - } - - const nextMergeProps = getNextMergeProps( this.props ); - if ( isShallowEqualObjects( this.mergeProps, nextMergeProps ) ) { - return; - } - - this.mergeProps = nextMergeProps; - - // Schedule an update. Merge props are not assigned to state since - // derivation of merge props from incoming props occurs within - // shouldComponentUpdate, where setState is not allowed. setState - // is used here instead of forceUpdate because forceUpdate bypasses - // shouldComponentUpdate altogether, which isn't desireable if both - // state and props change within the same render. Unfortunately, - // this requires that next merge props are generated twice. - this.setState( {} ); - } - - subscribe( registry ) { - this.unsubscribe = registry.subscribe( () => { - if ( this.props.isAsync ) { - renderQueue.add( this, this.onStoreChange ); - } else { - this.onStoreChange(); - } - } ); - } - - render() { - return ; - } - } - - return ( ownProps ) => ( - - { ( isAsync ) => ( - - { ( registry ) => ( - - ) } - - ) } - - ); -}, 'withSelect' ); + ), + 'withSelect' +); export default withSelect; diff --git a/packages/data/src/components/with-select/test/index.js b/packages/data/src/components/with-select/test/index.js index f42926e02266f..3b2049826c63b 100644 --- a/packages/data/src/components/with-select/test/index.js +++ b/packages/data/src/components/with-select/test/index.js @@ -1,7 +1,7 @@ /** * External dependencies */ -import TestRenderer from 'react-test-renderer'; +import TestRenderer, { act } from 'react-test-renderer'; /** * WordPress dependencies @@ -15,7 +15,7 @@ import { Component } from '@wordpress/element'; import withSelect from '../'; import withDispatch from '../../with-dispatch'; import { createRegistry } from '../../../registry'; -import RegistryProvider from '../../registry-provider'; +import { RegistryProvider } from '../../registry-provider'; describe( 'withSelect', () => { let registry; @@ -47,15 +47,19 @@ describe( 'withSelect', () => { ) ); const DataBoundComponent = withSelect( mapSelectToProps )( OriginalComponent ); - - const testRenderer = TestRenderer.create( - - - - ); + let testRenderer; + act( () => { + testRenderer = TestRenderer.create( + + + + ); + } ); const testInstance = testRenderer.root; - - expect( mapSelectToProps ).toHaveBeenCalledTimes( 1 ); + // Expected two times: + // - Once on initial render. + // - Once on effect before subscription set. + expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 ); expect( OriginalComponent ).toHaveBeenCalledTimes( 1 ); // Wrapper is the enhanced component. Find props on the rendered child. @@ -100,31 +104,46 @@ describe( 'withSelect', () => { withDispatch( mapDispatchToProps ), ] )( OriginalComponent ); - const testRenderer = TestRenderer.create( - - - - ); + let testRenderer; + act( () => { + testRenderer = TestRenderer.create( + + + + ); + } ); const testInstance = testRenderer.root; expect( OriginalComponent ).toHaveBeenCalledTimes( 1 ); - expect( mapSelectToProps ).toHaveBeenCalledTimes( 1 ); + // 2 times: + // - 1 on initial render + // - 1 on effect before subscription set. + expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 ); expect( mapDispatchToProps ).toHaveBeenCalledTimes( 1 ); // Simulate a click on the button - testInstance.findByType( 'button' ).props.onClick(); + act( () => { + testInstance.findByType( 'button' ).props.onClick(); + } ); expect( testInstance.findByType( 'button' ).props.children ).toBe( 1 ); // 2 times = // 1. Initial mount // 2. When click handler is called expect( mapDispatchToProps ).toHaveBeenCalledTimes( 2 ); - expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 ); + // 4 times + // - 1 on initial render + // - 1 on effect before subscription set. + // - 1 on click triggering subscription firing. + // - 1 on rerender. + expect( mapSelectToProps ).toHaveBeenCalledTimes( 4 ); + // verifies component only renders twice. expect( OriginalComponent ).toHaveBeenCalledTimes( 2 ); } ); - it( 'should rerun if had dispatched action during mount', () => { - registry.registerStore( 'counter', { + describe( 'expected behaviour when dispatching actions during mount', () => { + const testRegistry = createRegistry(); + testRegistry.registerStore( 'counter', { reducer: ( state = 0, action ) => { if ( action.type === 'increment' ) { return state + 1; @@ -140,6 +159,10 @@ describe( 'withSelect', () => { }, } ); + // @todo, Should we allow this behaviour? Side-effects + // on mount are discouraged in React (breaks Suspense and React Async Mode) + // leaving in place for now under the assumption there's current usage + // of withSelect in GB that expects support. class OriginalComponent extends Component { constructor( props ) { super( ...arguments ); @@ -156,10 +179,10 @@ describe( 'withSelect', () => { } } - jest.spyOn( OriginalComponent.prototype, 'render' ); + const renderSpy = jest.spyOn( OriginalComponent.prototype, 'render' ); - const mapSelectToProps = jest.fn().mockImplementation( ( _select, ownProps ) => ( { - count: _select( 'counter' ).getCount( ownProps.offset ), + const mapSelectToProps = jest.fn().mockImplementation( ( _select ) => ( { + count: _select( 'counter' ).getCount(), } ) ); const mapDispatchToProps = jest.fn().mockImplementation( ( _dispatch ) => ( { @@ -171,16 +194,39 @@ describe( 'withSelect', () => { withDispatch( mapDispatchToProps ), ] )( OriginalComponent ); - const testRenderer = TestRenderer.create( - + let testRenderer, testInstance; + const createTestRenderer = () => TestRenderer.create( + ); - const testInstance = testRenderer.root; - - expect( testInstance.findByType( 'div' ).props.children ).toBe( 2 ); - expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 ); - expect( OriginalComponent.prototype.render ).toHaveBeenCalledTimes( 2 ); + act( () => { + testRenderer = createTestRenderer(); + } ); + testInstance = testRenderer.root; + it( 'should rerun if had dispatched action during mount', () => { + expect( testInstance.findByType( 'div' ).props.children ).toBe( 2 ); + // Expected 3 times because: + // - 1 on initial render + // - 1 on effect before subscription set. + // - 1 for the rerender because of the mapOutput change detected. + expect( mapSelectToProps ).toHaveBeenCalledTimes( 3 ); + expect( renderSpy ).toHaveBeenCalledTimes( 2 ); + } ); + it( 'should rerun on unmount and mount', () => { + act( () => { + testRenderer.unmount(); + testRenderer = createTestRenderer(); + } ); + testInstance = testRenderer.root; + expect( testInstance.findByType( 'div' ).props.children ).toBe( 4 ); + // Expected an additional 3 times because of the unmount and remount: + // - 1 on initial render + // - 1 on effect before subscription set. + // - once for the rerender because of the mapOutput change detected. + expect( mapSelectToProps ).toHaveBeenCalledTimes( 6 ); + expect( renderSpy ).toHaveBeenCalledTimes( 4 ); + } ); } ); it( 'should rerun selection on props changes', () => { @@ -207,24 +253,32 @@ describe( 'withSelect', () => { const DataBoundComponent = withSelect( mapSelectToProps )( OriginalComponent ); - const testRenderer = TestRenderer.create( - - - - ); + let testRenderer; + act( () => { + testRenderer = TestRenderer.create( + + + + ); + } ); const testInstance = testRenderer.root; - expect( mapSelectToProps ).toHaveBeenCalledTimes( 1 ); + // 2 times: + // - 1 on initial render + // - 1 on effect before subscription set. + expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 ); expect( OriginalComponent ).toHaveBeenCalledTimes( 1 ); - testRenderer.update( - - - - ); + act( () => { + testRenderer.update( + + + + ); + } ); expect( testInstance.findByType( 'div' ).props.children ).toBe( 10 ); - expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 ); + expect( mapSelectToProps ).toHaveBeenCalledTimes( 3 ); expect( OriginalComponent ).toHaveBeenCalledTimes( 2 ); } ); @@ -246,26 +300,34 @@ describe( 'withSelect', () => { const Parent = ( props ) => ; - const testRenderer = TestRenderer.create( - - - - ); + let testRenderer; + act( () => { + testRenderer = TestRenderer.create( + + + + ); + } ); - expect( mapSelectToProps ).toHaveBeenCalledTimes( 1 ); + // 2 times: + // - 1 on initial render + // - 1 on effect before subscription set. + expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 ); expect( OriginalComponent ).toHaveBeenCalledTimes( 1 ); - testRenderer.update( - - - - ); + act( () => { + testRenderer.update( + + + + ); + } ); - expect( mapSelectToProps ).toHaveBeenCalledTimes( 1 ); + expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 ); expect( OriginalComponent ).toHaveBeenCalledTimes( 1 ); } ); - it( 'should not run selection if state has changed but merge props the same', () => { + it( 'should not rerender if state has changed but merge props the same', () => { registry.registerStore( 'demo', { reducer: () => ( {} ), selectors: { @@ -284,18 +346,23 @@ describe( 'withSelect', () => { const DataBoundComponent = withSelect( mapSelectToProps )( OriginalComponent ); - TestRenderer.create( - - - - ); + act( () => { + TestRenderer.create( + + + + ); + } ); - expect( mapSelectToProps ).toHaveBeenCalledTimes( 1 ); + // 2 times: + // - 1 on initial render + // - 1 on effect before subscription set. + expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 ); expect( OriginalComponent ).toHaveBeenCalledTimes( 1 ); registry.dispatch( 'demo' ).update(); - expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 ); + expect( mapSelectToProps ).toHaveBeenCalledTimes( 3 ); expect( OriginalComponent ).toHaveBeenCalledTimes( 1 ); } ); @@ -315,22 +382,30 @@ describe( 'withSelect', () => { withSelect( mapSelectToProps ), ] )( OriginalComponent ); - const testRenderer = TestRenderer.create( - - - - ); + let testRenderer; + act( () => { + testRenderer = TestRenderer.create( + + + + ); + } ); - expect( mapSelectToProps ).toHaveBeenCalledTimes( 1 ); + // 2 times: + // - 1 on initial render + // - 1 on effect before subscription set. + expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 ); expect( OriginalComponent ).toHaveBeenCalledTimes( 1 ); - testRenderer.update( - - - - ); + act( () => { + testRenderer.update( + + + + ); + } ); - expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 ); + expect( mapSelectToProps ).toHaveBeenCalledTimes( 3 ); expect( OriginalComponent ).toHaveBeenCalledTimes( 2 ); } ); @@ -350,18 +425,23 @@ describe( 'withSelect', () => { withSelect( mapSelectToProps ), ] )( OriginalComponent ); - TestRenderer.create( - - - - ); + act( () => { + TestRenderer.create( + + + + ); + } ); - expect( mapSelectToProps ).toHaveBeenCalledTimes( 1 ); + // 2 times: + // - 1 on initial render + // - 1 on effect before subscription set. + expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 ); expect( OriginalComponent ).toHaveBeenCalledTimes( 1 ); store.dispatch( { type: 'dummy' } ); - expect( mapSelectToProps ).toHaveBeenCalledTimes( 1 ); + expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 ); expect( OriginalComponent ).toHaveBeenCalledTimes( 1 ); } ); @@ -384,26 +464,34 @@ describe( 'withSelect', () => { const DataBoundComponent = withSelect( mapSelectToProps )( OriginalComponent ); - const testRenderer = TestRenderer.create( - - - - ); + let testRenderer; + act( () => { + testRenderer = TestRenderer.create( + + + + ); + } ); const testInstance = testRenderer.root; - expect( mapSelectToProps ).toHaveBeenCalledTimes( 1 ); + // 2 times: + // - 1 on initial render + // - 1 on effect before subscription set. + expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 ); expect( OriginalComponent ).toHaveBeenCalledTimes( 1 ); expect( JSON.parse( testInstance.findByType( 'div' ).props.children ) ) .toEqual( { foo: 'OK', propName: 'foo' } ); - testRenderer.update( - - - - ); + act( () => { + testRenderer.update( + + + + ); + } ); - expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 ); + expect( mapSelectToProps ).toHaveBeenCalledTimes( 3 ); expect( OriginalComponent ).toHaveBeenCalledTimes( 2 ); expect( JSON.parse( testInstance.findByType( 'div' ).props.children ) ) .toEqual( { bar: 'OK', propName: 'bar' } ); @@ -431,39 +519,49 @@ describe( 'withSelect', () => { const DataBoundComponent = withSelect( mapSelectToProps )( OriginalComponent ); - const testRenderer = TestRenderer.create( - - - - ); + let testRenderer; + act( () => { + testRenderer = TestRenderer.create( + + + + ); + } ); const testInstance = testRenderer.root; - expect( mapSelectToProps ).toHaveBeenCalledTimes( 1 ); + // 2 times: + // - 1 on initial render + // - 1 on effect before subscription set. + expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 ); expect( OriginalComponent ).toHaveBeenCalledTimes( 1 ); expect( testInstance.findByType( 'div' ).props.children ).toBe( 'Unknown' ); - testRenderer.update( - - - - ); + act( () => { + testRenderer.update( + + + + ); + } ); - expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 ); + expect( mapSelectToProps ).toHaveBeenCalledTimes( 3 ); expect( OriginalComponent ).toHaveBeenCalledTimes( 2 ); expect( testInstance.findByType( 'div' ).props.children ).toBe( 'OK' ); - testRenderer.update( - - - - ); + act( () => { + testRenderer.update( + + + + ); + } ); - expect( mapSelectToProps ).toHaveBeenCalledTimes( 3 ); + expect( mapSelectToProps ).toHaveBeenCalledTimes( 4 ); expect( OriginalComponent ).toHaveBeenCalledTimes( 3 ); expect( testInstance.findByType( 'div' ).props.children ).toBe( 'Unknown' ); } ); - it( 'should run selections on parents before its children', () => { + it( 'should limit unnecessary selections run on children', () => { registry.registerStore( 'childRender', { reducer: ( state = true, action ) => ( action.type === 'TOGGLE_RENDER' ? ! state : state @@ -477,9 +575,9 @@ describe( 'withSelect', () => { } ); const childMapSelectToProps = jest.fn(); - const parentMapSelectToProps = jest.fn().mockImplementation( ( _select ) => ( { - isRenderingChild: _select( 'childRender' ).getValue(), - } ) ); + const parentMapSelectToProps = jest.fn().mockImplementation( ( _select ) => ( + { isRenderingChild: _select( 'childRender' ).getValue() } + ) ); const ChildOriginalComponent = jest.fn().mockImplementation( () =>
); const ParentOriginalComponent = jest.fn().mockImplementation( ( props ) => ( @@ -489,21 +587,32 @@ describe( 'withSelect', () => { const Child = withSelect( childMapSelectToProps )( ChildOriginalComponent ); const Parent = withSelect( parentMapSelectToProps )( ParentOriginalComponent ); - TestRenderer.create( - - - - ); + act( () => { + TestRenderer.create( + + + + ); + } ); - expect( childMapSelectToProps ).toHaveBeenCalledTimes( 1 ); - expect( parentMapSelectToProps ).toHaveBeenCalledTimes( 1 ); + // 2 times: + // - 1 on initial render + // - 1 on effect before subscription set. + expect( childMapSelectToProps ).toHaveBeenCalledTimes( 2 ); + expect( parentMapSelectToProps ).toHaveBeenCalledTimes( 2 ); expect( ChildOriginalComponent ).toHaveBeenCalledTimes( 1 ); expect( ParentOriginalComponent ).toHaveBeenCalledTimes( 1 ); - registry.dispatch( 'childRender' ).toggleRender(); + act( () => { + registry.dispatch( 'childRender' ).toggleRender(); + } ); - expect( childMapSelectToProps ).toHaveBeenCalledTimes( 1 ); - expect( parentMapSelectToProps ).toHaveBeenCalledTimes( 2 ); + // 3 times because + // - 1 on initial render + // - 1 on effect before subscription set. + // - 1 child subscription fires. + expect( childMapSelectToProps ).toHaveBeenCalledTimes( 3 ); + expect( parentMapSelectToProps ).toHaveBeenCalledTimes( 4 ); expect( ChildOriginalComponent ).toHaveBeenCalledTimes( 1 ); expect( ParentOriginalComponent ).toHaveBeenCalledTimes( 2 ); } ); @@ -527,14 +636,20 @@ describe( 'withSelect', () => { const DataBoundComponent = withSelect( mapSelectToProps )( OriginalComponent ); - const testRenderer = TestRenderer.create( - - - - ); + let testRenderer; + act( () => { + testRenderer = TestRenderer.create( + + + + ); + } ); const testInstance = testRenderer.root; - expect( mapSelectToProps ).toHaveBeenCalledTimes( 1 ); + // 2 times: + // - 1 on initial render + // - 1 on effect before subscription set. + expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 ); expect( OriginalComponent ).toHaveBeenCalledTimes( 1 ); expect( testInstance.findByType( 'div' ).props ).toEqual( { @@ -549,13 +664,19 @@ describe( 'withSelect', () => { }, } ); - testRenderer.update( - - - - ); - - expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 ); + act( () => { + testRenderer.update( + + + + ); + } ); + // 4 times: + // - 1 on initial render + // - 1 on effect before subscription set. + // - 1 on re-render + // - 1 on effect before new subscription set (because registry has changed) + expect( mapSelectToProps ).toHaveBeenCalledTimes( 4 ); expect( OriginalComponent ).toHaveBeenCalledTimes( 2 ); expect( testInstance.findByType( 'div' ).props ).toEqual( { diff --git a/packages/data/src/index.js b/packages/data/src/index.js index c6b941d2cec95..46c830289b91d 100644 --- a/packages/data/src/index.js +++ b/packages/data/src/index.js @@ -12,8 +12,15 @@ import * as plugins from './plugins'; export { default as withSelect } from './components/with-select'; export { default as withDispatch } from './components/with-dispatch'; export { default as withRegistry } from './components/with-registry'; -export { default as RegistryProvider, RegistryConsumer } from './components/registry-provider'; -export { default as __experimentalAsyncModeProvider } from './components/async-mode-provider'; +export { + RegistryProvider, + RegistryConsumer, + useRegistry, +} from './components/registry-provider'; +export { default as useSelect } from './components/use-select'; +export { + AsyncModeProvider as __experimentalAsyncModeProvider, +} from './components/async-mode-provider'; export { createRegistry } from './registry'; export { plugins }; export { createRegistrySelector, createRegistryControl } from './factory'; diff --git a/packages/viewport/src/test/if-viewport-matches.js b/packages/viewport/src/test/if-viewport-matches.js index c5eca13c0ea14..4a57039e9c7f1 100644 --- a/packages/viewport/src/test/if-viewport-matches.js +++ b/packages/viewport/src/test/if-viewport-matches.js @@ -1,7 +1,7 @@ /** * External dependencies */ -import TestRenderer from 'react-test-renderer'; +import TestRenderer, { act } from 'react-test-renderer'; /** * WordPress dependencies @@ -20,16 +20,24 @@ describe( 'ifViewportMatches()', () => { it( 'should not render if query does not match', () => { dispatch( 'core/viewport' ).setIsMatching( { '> wide': false } ); const EnhancedComponent = ifViewportMatches( '> wide' )( Component ); - const testRenderer = TestRenderer.create( ); + + let testRenderer; + act( () => { + testRenderer = TestRenderer.create( ); + } ); expect( testRenderer.root.findAllByType( Component ) ).toHaveLength( 0 ); } ); it( 'should render if query does match', () => { - dispatch( 'core/viewport' ).setIsMatching( { '> wide': true } ); + act( () => { + dispatch( 'core/viewport' ).setIsMatching( { '> wide': true } ); + } ); const EnhancedComponent = ifViewportMatches( '> wide' )( Component ); - const testRenderer = TestRenderer.create( ); - + let testRenderer; + act( () => { + testRenderer = TestRenderer.create( ); + } ); expect( testRenderer.root.findAllByType( Component ) ).toHaveLength( 1 ); } ); } );