From edeec702ff4125e034080a24afbffdd1b19c9ff5 Mon Sep 17 00:00:00 2001 From: Darren Ethier Date: Mon, 20 May 2019 08:15:51 -0400 Subject: [PATCH 01/22] Add useAsyncMode hook - exports entire context for internal use - exports the new hook --- .../src/components/async-mode-provider/context.js | 12 ++++++++++++ .../src/components/async-mode-provider/index.js | 12 ++---------- .../async-mode-provider/use-async-mode.js | 13 +++++++++++++ 3 files changed, 27 insertions(+), 10 deletions(-) create mode 100644 packages/data/src/components/async-mode-provider/context.js create mode 100644 packages/data/src/components/async-mode-provider/use-async-mode.js 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 00000000000000..c0e59ef7138183 --- /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 30e877d0f1ed3c..778dce8fc6eb23 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 00000000000000..2f53c34ae2e07d --- /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 ); +} From fbc9f0a0d53ddb017d5b1f39c08e9b0d43948d86 Mon Sep 17 00:00:00 2001 From: Darren Ethier Date: Mon, 20 May 2019 08:17:06 -0400 Subject: [PATCH 02/22] add useRegistry hook - export entire context for internal use - export hook --- .../src/components/registry-provider/context.js | 17 +++++++++++++++++ .../src/components/registry-provider/index.js | 17 ++--------------- .../registry-provider/use-registry.js | 13 +++++++++++++ 3 files changed, 32 insertions(+), 15 deletions(-) create mode 100644 packages/data/src/components/registry-provider/context.js create mode 100644 packages/data/src/components/registry-provider/use-registry.js 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 00000000000000..825b89ab9fcc5e --- /dev/null +++ b/packages/data/src/components/registry-provider/context.js @@ -0,0 +1,17 @@ +/** + * WordPress dependencies + */ +import { createContext } from '@wordpress/element'; + +/** + * Internal dependencies + */ +import defaultRegistry from '../../default-registry'; + +export const Context = createContext( defaultRegistry ); + +const { Consumer, Provider } = Context; + +export const RegistryConsumer = Consumer; + +export default Provider; diff --git a/packages/data/src/components/registry-provider/index.js b/packages/data/src/components/registry-provider/index.js index c296f46a1ab1a8..e4f8d99bec597f 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 00000000000000..253737a14782eb --- /dev/null +++ b/packages/data/src/components/registry-provider/use-registry.js @@ -0,0 +1,13 @@ +/** + * WordPress dependencies + */ +import { useContext } from '@wordpress/element'; + +/** + * Internal dependencies + */ +import { Context } from './context'; + +export default function useRegistry() { + return useContext( Context ); +} From b698a7e4bf8585e6198960c0b2e2615cc9a21148 Mon Sep 17 00:00:00 2001 From: Darren Ethier Date: Mon, 20 May 2019 08:18:38 -0400 Subject: [PATCH 03/22] add new useSelect hook --- .../data/src/components/use-select/index.js | 101 ++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100644 packages/data/src/components/use-select/index.js 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 00000000000000..38da695456643a --- /dev/null +++ b/packages/data/src/components/use-select/index.js @@ -0,0 +1,101 @@ +/** + * WordPress dependencies + */ +import { createQueue } from '@wordpress/priority-queue'; +import { + useLayoutEffect, + useEffect, + useState, + useRef, + useMemo, + useCallback, +} 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 useIsomorphicLayoutEffect = typeof window !== 'undefined' ? + useLayoutEffect : useEffect; + +const renderQueue = createQueue(); + +export default function useSelect( mapSelect, nextProps ) { + const registry = useRegistry(); + const isAsync = useAsyncMode(); + const [ mapOutput, setMapOutput ] = useState( mapSelect( registry.select ) ); + const queueContext = useMemo( () => ( { queue: true } ), [ registry ] ); + + const latestMapSelect = useRef( mapSelect ); + const latestIsAsync = useRef( isAsync ); + const latestMapOutput = useRef( mapOutput ); + const isMounted = useRef(); + + /** + * Set current values. + */ + useIsomorphicLayoutEffect( () => { + latestMapSelect.current = mapSelect; + isMounted.current = true; + } ); + + /** + * Flush queue if isAsync changes + */ + useIsomorphicLayoutEffect( () => { + latestIsAsync.current = isAsync; + renderQueue.flush( queueContext ); + }, [ isAsync ] ); + + /** + * Callback for doing changes to store. + */ + const onStoreChange = useCallback( () => { + if ( isMounted.current ) { + const newMapOutput = latestMapSelect.current( registry.select, registry ); + if ( ! isShallowEqualObjects( latestMapOutput.current, newMapOutput ) ) { + latestMapOutput.current = newMapOutput; + setMapOutput( newMapOutput ); + } + } + }, [ registry, nextProps ] ); + + /** + * Map output should always be called if incoming prop dependencies + * has changed. + */ + useIsomorphicLayoutEffect( () => { + onStoreChange(); + }, [ registry, nextProps ] ); + + useIsomorphicLayoutEffect( () => { + const unsubscribe = registry.subscribe( () => { + if ( latestIsAsync.current ) { + renderQueue.add( queueContext, onStoreChange ); + } else { + onStoreChange(); + } + } ); + + return () => { + isMounted.current = false; + unsubscribe(); + renderQueue.flush( queueContext ); + }; + }, [ registry ] ); + + return mapOutput; +} From a331997cc8960a837eb6b4181873c393d343e293 Mon Sep 17 00:00:00 2001 From: Darren Ethier Date: Mon, 20 May 2019 08:20:20 -0400 Subject: [PATCH 04/22] try new `withSelect` implementing useSelect internally. - renamed old withSelect and kept file temprorarily for reference. On release would get removed. --- .../data/src/components/with-select/index.js | 208 +----------------- .../components/with-select/with-select-new.js | 25 +++ .../components/with-select/with-select-old.js | 208 ++++++++++++++++++ 3 files changed, 234 insertions(+), 207 deletions(-) create mode 100644 packages/data/src/components/with-select/with-select-new.js create mode 100644 packages/data/src/components/with-select/with-select-old.js diff --git a/packages/data/src/components/with-select/index.js b/packages/data/src/components/with-select/index.js index d2fd4ae462b3bb..e1d04c6b48dc2d 100644 --- a/packages/data/src/components/with-select/index.js +++ b/packages/data/src/components/with-select/index.js @@ -1,208 +1,2 @@ -/** - * 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'; +export { default as withSelect } from './with-select-new'; -/** - * Internal dependencies - */ -import { RegistryConsumer } from '../registry-provider'; -import { AsyncModeConsumer } from '../async-mode-provider'; - -const renderQueue = createQueue(); - -/** - * Higher-order component used to inject state-derived props using registered - * selectors. - * - * @param {Function} mapSelectToProps Function called on every state change, - * expected to return object of props to - * merge with the component's own props. - * - * @example - * ```js - * function PriceDisplay( { price, currency } ) { - * return new Intl.NumberFormat( 'en-US', { - * style: 'currency', - * currency, - * } ).format( price ); - * } - * - * const { withSelect } = wp.data; - * - * const HammerPriceDisplay = withSelect( ( select, ownProps ) => { - * const { getPrice } = select( 'my-shop' ); - * const { currency } = ownProps; - * - * return { - * price: getPrice( 'hammer', currency ), - * }; - * } )( PriceDisplay ); - * - * // Rendered in the application: - * // - * // - * ``` - * 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; - } - - 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' ); - -export default withSelect; diff --git a/packages/data/src/components/with-select/with-select-new.js b/packages/data/src/components/with-select/with-select-new.js new file mode 100644 index 00000000000000..190f96bf5ea40e --- /dev/null +++ b/packages/data/src/components/with-select/with-select-new.js @@ -0,0 +1,25 @@ +/** + * WordPress dependencies + */ +import { createHigherOrderComponent } from '@wordpress/compose'; + +/** + * Internal dependencies + */ +import useSelect from '../use-select'; + +const withSelect = ( mapSelectToProps ) => createHigherOrderComponent( + ( WrappedComponent ) => ( ownProps ) => { + const mapSelect = + ( select, registry ) => mapSelectToProps( + select, + ownProps, + registry + ); + const mergeProps = useSelect( mapSelect, ownProps ); + return ; + }, + 'withSelect' +); + +export default withSelect; diff --git a/packages/data/src/components/with-select/with-select-old.js b/packages/data/src/components/with-select/with-select-old.js new file mode 100644 index 00000000000000..d2fd4ae462b3bb --- /dev/null +++ b/packages/data/src/components/with-select/with-select-old.js @@ -0,0 +1,208 @@ +/** + * 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'; + +/** + * Internal dependencies + */ +import { RegistryConsumer } from '../registry-provider'; +import { AsyncModeConsumer } from '../async-mode-provider'; + +const renderQueue = createQueue(); + +/** + * Higher-order component used to inject state-derived props using registered + * selectors. + * + * @param {Function} mapSelectToProps Function called on every state change, + * expected to return object of props to + * merge with the component's own props. + * + * @example + * ```js + * function PriceDisplay( { price, currency } ) { + * return new Intl.NumberFormat( 'en-US', { + * style: 'currency', + * currency, + * } ).format( price ); + * } + * + * const { withSelect } = wp.data; + * + * const HammerPriceDisplay = withSelect( ( select, ownProps ) => { + * const { getPrice } = select( 'my-shop' ); + * const { currency } = ownProps; + * + * return { + * price: getPrice( 'hammer', currency ), + * }; + * } )( PriceDisplay ); + * + * // Rendered in the application: + * // + * // + * ``` + * 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; + } + + 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' ); + +export default withSelect; From dd58983e5307c5dee62a4322df9f38fc1b16232b Mon Sep 17 00:00:00 2001 From: Darren Ethier Date: Mon, 20 May 2019 08:21:09 -0400 Subject: [PATCH 05/22] updates to exports --- packages/data/src/index.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/data/src/index.js b/packages/data/src/index.js index c6b941d2cec954..d9c7c32ea75fc1 100644 --- a/packages/data/src/index.js +++ b/packages/data/src/index.js @@ -9,11 +9,14 @@ import combineReducers from 'turbo-combine-reducers'; import defaultRegistry from './default-registry'; import * as plugins from './plugins'; -export { default as withSelect } from './components/with-select'; +export { 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 } from './components/registry-provider'; +export { + AsyncModeProvider as __experimentalAsyncModeProvider, + useAsyncMode as __experimentalUseAsyncMode, +} from './components/async-mode-provider'; export { createRegistry } from './registry'; export { plugins }; export { createRegistrySelector, createRegistryControl } from './factory'; From d781cfa3e37c1df8436b5d936bc0fc0db11c0fa7 Mon Sep 17 00:00:00 2001 From: Darren Ethier Date: Mon, 20 May 2019 08:22:07 -0400 Subject: [PATCH 06/22] updates to README.md file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - note new withSelect isn’t documented yet, it likely will keep the same documentation on release. --- packages/data/README.md | 39 +-------------------------------------- 1 file changed, 1 insertion(+), 38 deletions(-) diff --git a/packages/data/README.md b/packages/data/README.md index 65b525d404099d..83aeb72e4513ee 100644 --- a/packages/data/README.md +++ b/packages/data/README.md @@ -516,44 +516,7 @@ _Returns_ # **withSelect** -Higher-order component used to inject state-derived props using registered -selectors. - -_Usage_ - -```js -function PriceDisplay( { price, currency } ) { - return new Intl.NumberFormat( 'en-US', { - style: 'currency', - currency, - } ).format( price ); -} - -const { withSelect } = wp.data; - -const HammerPriceDisplay = withSelect( ( select, ownProps ) => { - const { getPrice } = select( 'my-shop' ); - const { currency } = ownProps; - - return { - price: getPrice( 'hammer', currency ), - }; -} )( PriceDisplay ); - -// Rendered in the application: -// -// -``` - -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_ - -- _mapSelectToProps_ `Function`: Function called on every state change, expected to return object of props to merge with the component's own props. - -_Returns_ - -- `Component`: Enhanced component with merged state data props. +Undocumented declaration. From 98bf61e9e21cc3bda5a0bd3b6eae1764b64d68df Mon Sep 17 00:00:00 2001 From: Darren Ethier Date: Mon, 20 May 2019 11:10:03 -0400 Subject: [PATCH 07/22] =?UTF-8?q?just=20use=20useLayoutEffect=20since=20GB?= =?UTF-8?q?=20as=20a=20whole=20doesn=E2=80=99t=20suppor=20server=20side=20?= =?UTF-8?q?rendering=20yet?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../data/src/components/use-select/index.js | 22 ++++--------------- 1 file changed, 4 insertions(+), 18 deletions(-) diff --git a/packages/data/src/components/use-select/index.js b/packages/data/src/components/use-select/index.js index 38da695456643a..18c6890dcc7093 100644 --- a/packages/data/src/components/use-select/index.js +++ b/packages/data/src/components/use-select/index.js @@ -4,7 +4,6 @@ import { createQueue } from '@wordpress/priority-queue'; import { useLayoutEffect, - useEffect, useState, useRef, useMemo, @@ -18,19 +17,6 @@ import { isShallowEqualObjects } from '@wordpress/is-shallow-equal'; 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 useIsomorphicLayoutEffect = typeof window !== 'undefined' ? - useLayoutEffect : useEffect; - const renderQueue = createQueue(); export default function useSelect( mapSelect, nextProps ) { @@ -47,7 +33,7 @@ export default function useSelect( mapSelect, nextProps ) { /** * Set current values. */ - useIsomorphicLayoutEffect( () => { + useLayoutEffect( () => { latestMapSelect.current = mapSelect; isMounted.current = true; } ); @@ -55,7 +41,7 @@ export default function useSelect( mapSelect, nextProps ) { /** * Flush queue if isAsync changes */ - useIsomorphicLayoutEffect( () => { + useLayoutEffect( () => { latestIsAsync.current = isAsync; renderQueue.flush( queueContext ); }, [ isAsync ] ); @@ -77,11 +63,11 @@ export default function useSelect( mapSelect, nextProps ) { * Map output should always be called if incoming prop dependencies * has changed. */ - useIsomorphicLayoutEffect( () => { + useLayoutEffect( () => { onStoreChange(); }, [ registry, nextProps ] ); - useIsomorphicLayoutEffect( () => { + useLayoutEffect( () => { const unsubscribe = registry.subscribe( () => { if ( latestIsAsync.current ) { renderQueue.add( queueContext, onStoreChange ); From 4ae346124a27a3af2ea406065a104aedfd74f516 Mon Sep 17 00:00:00 2001 From: Darren Ethier Date: Mon, 20 May 2019 11:11:28 -0400 Subject: [PATCH 08/22] only make `onStoreChange` dependent on registry and add dependency for subscription --- .../data/src/components/use-select/index.js | 25 ++++++++----------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/packages/data/src/components/use-select/index.js b/packages/data/src/components/use-select/index.js index 18c6890dcc7093..382ac799a0f598 100644 --- a/packages/data/src/components/use-select/index.js +++ b/packages/data/src/components/use-select/index.js @@ -30,6 +30,16 @@ export default function useSelect( mapSelect, nextProps ) { const latestMapOutput = useRef( mapOutput ); const isMounted = useRef(); + const onStoreChange = useCallback( () => { + if ( isMounted.current ) { + const newMapOutput = latestMapSelect.current( registry.select, registry ); + if ( ! isShallowEqualObjects( latestMapOutput.current, newMapOutput ) ) { + latestMapOutput.current = newMapOutput; + setMapOutput( newMapOutput ); + } + } + }, [ registry ] ); + /** * Set current values. */ @@ -46,19 +56,6 @@ export default function useSelect( mapSelect, nextProps ) { renderQueue.flush( queueContext ); }, [ isAsync ] ); - /** - * Callback for doing changes to store. - */ - const onStoreChange = useCallback( () => { - if ( isMounted.current ) { - const newMapOutput = latestMapSelect.current( registry.select, registry ); - if ( ! isShallowEqualObjects( latestMapOutput.current, newMapOutput ) ) { - latestMapOutput.current = newMapOutput; - setMapOutput( newMapOutput ); - } - } - }, [ registry, nextProps ] ); - /** * Map output should always be called if incoming prop dependencies * has changed. @@ -81,7 +78,7 @@ export default function useSelect( mapSelect, nextProps ) { unsubscribe(); renderQueue.flush( queueContext ); }; - }, [ registry ] ); + }, [ onStoreChange ] ); return mapOutput; } From 6aaeb56e0dc95907b83f677207c10241c9c23f8b Mon Sep 17 00:00:00 2001 From: Darren Ethier Date: Mon, 20 May 2019 18:40:59 -0400 Subject: [PATCH 09/22] latest changes - reimplement useIsomorphicLayoutEffect - set mapOutput on initial render - implement forRender call instead of mapOutPut being stateful. --- .../data/src/components/use-select/index.js | 77 ++++++++++--------- 1 file changed, 41 insertions(+), 36 deletions(-) diff --git a/packages/data/src/components/use-select/index.js b/packages/data/src/components/use-select/index.js index 382ac799a0f598..0e131bad1deb94 100644 --- a/packages/data/src/components/use-select/index.js +++ b/packages/data/src/components/use-select/index.js @@ -4,10 +4,10 @@ import { createQueue } from '@wordpress/priority-queue'; import { useLayoutEffect, - useState, useRef, useMemo, - useCallback, + useEffect, + useReducer, } from '@wordpress/element'; import { isShallowEqualObjects } from '@wordpress/is-shallow-equal'; @@ -17,54 +17,59 @@ import { isShallowEqualObjects } from '@wordpress/is-shallow-equal'; 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(); -export default function useSelect( mapSelect, nextProps ) { +export default function useSelect( mapSelect ) { const registry = useRegistry(); const isAsync = useAsyncMode(); - const [ mapOutput, setMapOutput ] = useState( mapSelect( registry.select ) ); const queueContext = useMemo( () => ( { queue: true } ), [ registry ] ); + const [ , forceRender ] = useReducer( ( s ) => s + 1, 0 ); - const latestMapSelect = useRef( mapSelect ); + const latestMapSelect = useRef(); const latestIsAsync = useRef( isAsync ); - const latestMapOutput = useRef( mapOutput ); + const latestMapOutput = useRef(); const isMounted = useRef(); - const onStoreChange = useCallback( () => { - if ( isMounted.current ) { - const newMapOutput = latestMapSelect.current( registry.select, registry ); - if ( ! isShallowEqualObjects( latestMapOutput.current, newMapOutput ) ) { - latestMapOutput.current = newMapOutput; - setMapOutput( newMapOutput ); - } - } - }, [ registry ] ); + let mapOutput; + + if ( latestMapSelect.current !== mapSelect ) { + mapOutput = mapSelect( registry.select, registry ); + } else { + mapOutput = latestMapOutput.current; + } - /** - * Set current values. - */ - useLayoutEffect( () => { + useIsomorphicLayoutEffect( () => { latestMapSelect.current = mapSelect; + if ( latestIsAsync.current !== isAsync ) { + latestIsAsync.current = isAsync; + renderQueue.flush( queueContext ); + } + latestMapOutput.current = mapOutput; isMounted.current = true; } ); - /** - * Flush queue if isAsync changes - */ - useLayoutEffect( () => { - latestIsAsync.current = isAsync; - renderQueue.flush( queueContext ); - }, [ isAsync ] ); - - /** - * Map output should always be called if incoming prop dependencies - * has changed. - */ - useLayoutEffect( () => { - onStoreChange(); - }, [ registry, nextProps ] ); + useIsomorphicLayoutEffect( () => { + const onStoreChange = () => { + if ( isMounted.current ) { + const newMapOutput = latestMapSelect.current( registry.select, registry ); + if ( ! isShallowEqualObjects( latestMapOutput.current, newMapOutput ) ) { + latestMapOutput.current = newMapOutput; + forceRender(); + } + } + }; - useLayoutEffect( () => { const unsubscribe = registry.subscribe( () => { if ( latestIsAsync.current ) { renderQueue.add( queueContext, onStoreChange ); @@ -78,7 +83,7 @@ export default function useSelect( mapSelect, nextProps ) { unsubscribe(); renderQueue.flush( queueContext ); }; - }, [ onStoreChange ] ); + }, [ registry ] ); return mapOutput; } From a06fa10a3f75d66d490c726c0100e857aebb2e7a Mon Sep 17 00:00:00 2001 From: Darren Ethier Date: Mon, 20 May 2019 18:41:42 -0400 Subject: [PATCH 10/22] implement memoized mapSelect callback. --- .../data/src/components/with-select/with-select-new.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/data/src/components/with-select/with-select-new.js b/packages/data/src/components/with-select/with-select-new.js index 190f96bf5ea40e..7d78ed182bb984 100644 --- a/packages/data/src/components/with-select/with-select-new.js +++ b/packages/data/src/components/with-select/with-select-new.js @@ -2,6 +2,7 @@ * WordPress dependencies */ import { createHigherOrderComponent } from '@wordpress/compose'; +import { useCallback } from '@wordpress/element'; /** * Internal dependencies @@ -10,13 +11,15 @@ import useSelect from '../use-select'; const withSelect = ( mapSelectToProps ) => createHigherOrderComponent( ( WrappedComponent ) => ( ownProps ) => { - const mapSelect = + const mapSelect = useCallback( ( select, registry ) => mapSelectToProps( select, ownProps, registry - ); - const mergeProps = useSelect( mapSelect, ownProps ); + ), + [ ownProps ] + ); + const mergeProps = useSelect( mapSelect ); return ; }, 'withSelect' From 59bb4c3df2ac6bb16171c8d575a5ae176cef479e Mon Sep 17 00:00:00 2001 From: Darren Ethier Date: Tue, 21 May 2019 07:37:09 -0400 Subject: [PATCH 11/22] implement pure on inner component --- .../components/with-select/with-select-new.js | 27 +++++++++---------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/packages/data/src/components/with-select/with-select-new.js b/packages/data/src/components/with-select/with-select-new.js index 7d78ed182bb984..2476d7a784d887 100644 --- a/packages/data/src/components/with-select/with-select-new.js +++ b/packages/data/src/components/with-select/with-select-new.js @@ -1,8 +1,7 @@ /** * WordPress dependencies */ -import { createHigherOrderComponent } from '@wordpress/compose'; -import { useCallback } from '@wordpress/element'; +import { createHigherOrderComponent, pure } from '@wordpress/compose'; /** * Internal dependencies @@ -10,18 +9,18 @@ import { useCallback } from '@wordpress/element'; import useSelect from '../use-select'; const withSelect = ( mapSelectToProps ) => createHigherOrderComponent( - ( WrappedComponent ) => ( ownProps ) => { - const mapSelect = useCallback( - ( select, registry ) => mapSelectToProps( - select, - ownProps, - registry - ), - [ ownProps ] - ); - const mergeProps = useSelect( mapSelect ); - return ; - }, + ( WrappedComponent ) => pure( + ( ownProps ) => { + const mapSelect = + ( select, registry ) => mapSelectToProps( + select, + ownProps, + registry + ); + const mergeProps = useSelect( mapSelect ); + return ; + } + ), 'withSelect' ); From c761c546598a617ace7db236d1d8a69430b8c375 Mon Sep 17 00:00:00 2001 From: Darren Ethier Date: Tue, 21 May 2019 19:56:06 -0400 Subject: [PATCH 12/22] implement try/catch approach for dealing with zombie props --- .../data/src/components/use-select/index.js | 37 +++++++++++++++---- 1 file changed, 30 insertions(+), 7 deletions(-) diff --git a/packages/data/src/components/use-select/index.js b/packages/data/src/components/use-select/index.js index 0e131bad1deb94..962a603ce0d947 100644 --- a/packages/data/src/components/use-select/index.js +++ b/packages/data/src/components/use-select/index.js @@ -39,14 +39,27 @@ export default function useSelect( mapSelect ) { const latestMapSelect = useRef(); const latestIsAsync = useRef( isAsync ); const latestMapOutput = useRef(); + const latestMapOutputError = useRef(); const isMounted = useRef(); let mapOutput; - if ( latestMapSelect.current !== mapSelect ) { - mapOutput = mapSelect( registry.select, registry ); - } else { - mapOutput = latestMapOutput.current; + 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( () => { @@ -56,17 +69,27 @@ export default function useSelect( mapSelect ) { renderQueue.flush( queueContext ); } latestMapOutput.current = mapOutput; + latestMapOutputError.current = undefined; isMounted.current = true; } ); useIsomorphicLayoutEffect( () => { const onStoreChange = () => { if ( isMounted.current ) { - const newMapOutput = latestMapSelect.current( registry.select, registry ); - if ( ! isShallowEqualObjects( latestMapOutput.current, newMapOutput ) ) { + try { + const newMapOutput = latestMapSelect.current( + registry.select, + registry + ); + if ( isShallowEqualObjects( latestMapOutput.current, newMapOutput ) ) { + return; + } latestMapOutput.current = newMapOutput; - forceRender(); + } catch ( error ) { + latestMapOutputError.current = error; } + + forceRender( {} ); } }; From 86f864dab1392883eacdba2560f5ab46742e2cd3 Mon Sep 17 00:00:00 2001 From: Darren Ethier Date: Tue, 21 May 2019 21:18:36 -0400 Subject: [PATCH 13/22] implement depdendencies argument --- packages/data/src/components/use-select/index.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/data/src/components/use-select/index.js b/packages/data/src/components/use-select/index.js index 962a603ce0d947..a2c8f92e696a66 100644 --- a/packages/data/src/components/use-select/index.js +++ b/packages/data/src/components/use-select/index.js @@ -30,7 +30,8 @@ const useIsomorphicLayoutEffect = const renderQueue = createQueue(); -export default function useSelect( mapSelect ) { +export default function useSelect( _mapSelect, deps ) { + const mapSelect = useMemo( () => _mapSelect, deps ); const registry = useRegistry(); const isAsync = useAsyncMode(); const queueContext = useMemo( () => ( { queue: true } ), [ registry ] ); @@ -63,7 +64,7 @@ export default function useSelect( mapSelect ) { } useIsomorphicLayoutEffect( () => { - latestMapSelect.current = mapSelect; + latestMapSelect.current = _mapSelect; if ( latestIsAsync.current !== isAsync ) { latestIsAsync.current = isAsync; renderQueue.flush( queueContext ); From e891f53782eca83f76f676476f2c393f7542bbe7 Mon Sep 17 00:00:00 2001 From: Darren Ethier Date: Tue, 21 May 2019 21:28:30 -0400 Subject: [PATCH 14/22] =?UTF-8?q?hmm=20nope,=20let=E2=80=99s=20add=20=5Fma?= =?UTF-8?q?pSelect=20to=20useMemo=20dependencies?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/data/src/components/use-select/index.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/data/src/components/use-select/index.js b/packages/data/src/components/use-select/index.js index a2c8f92e696a66..e39f85da7ff060 100644 --- a/packages/data/src/components/use-select/index.js +++ b/packages/data/src/components/use-select/index.js @@ -30,8 +30,8 @@ const useIsomorphicLayoutEffect = const renderQueue = createQueue(); -export default function useSelect( _mapSelect, deps ) { - const mapSelect = useMemo( () => _mapSelect, deps ); +export default function useSelect( _mapSelect, deps = [] ) { + const mapSelect = useMemo( () => _mapSelect, [ _mapSelect, ...deps ] ); const registry = useRegistry(); const isAsync = useAsyncMode(); const queueContext = useMemo( () => ( { queue: true } ), [ registry ] ); @@ -64,7 +64,7 @@ export default function useSelect( _mapSelect, deps ) { } useIsomorphicLayoutEffect( () => { - latestMapSelect.current = _mapSelect; + latestMapSelect.current = mapSelect; if ( latestIsAsync.current !== isAsync ) { latestIsAsync.current = isAsync; renderQueue.flush( queueContext ); From e0aeac23804975340d878bdb93f27adcefb707e4 Mon Sep 17 00:00:00 2001 From: Darren Ethier Date: Tue, 21 May 2019 21:54:39 -0400 Subject: [PATCH 15/22] consider dependencies on whether to regenerate mapOutput --- .../data/src/components/use-select/index.js | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/packages/data/src/components/use-select/index.js b/packages/data/src/components/use-select/index.js index e39f85da7ff060..96cc1190cb8b1d 100644 --- a/packages/data/src/components/use-select/index.js +++ b/packages/data/src/components/use-select/index.js @@ -9,7 +9,7 @@ import { useEffect, useReducer, } from '@wordpress/element'; -import { isShallowEqualObjects } from '@wordpress/is-shallow-equal'; +import { isShallowEqualObjects, isShallowEqualArrays } from '@wordpress/is-shallow-equal'; /** * Internal dependencies @@ -30,8 +30,7 @@ const useIsomorphicLayoutEffect = const renderQueue = createQueue(); -export default function useSelect( _mapSelect, deps = [] ) { - const mapSelect = useMemo( () => _mapSelect, [ _mapSelect, ...deps ] ); +export default function useSelect( mapSelect, deps ) { const registry = useRegistry(); const isAsync = useAsyncMode(); const queueContext = useMemo( () => ( { queue: true } ), [ registry ] ); @@ -41,12 +40,23 @@ export default function useSelect( _mapSelect, deps = [] ) { const latestIsAsync = useRef( isAsync ); const latestMapOutput = useRef(); const latestMapOutputError = useRef(); + const latestDependencies = useRef(); const isMounted = useRef(); let mapOutput; + const hasSameMapSelect = latestMapSelect.current === mapSelect; + const hasSameDependencies = isShallowEqualArrays( + latestDependencies.current, + deps + ); + try { - if ( latestMapSelect.current !== mapSelect || latestMapOutputError.current ) { + if ( + ! hasSameMapSelect || + ! hasSameDependencies || + latestMapOutputError.current + ) { mapOutput = mapSelect( registry.select, registry ); } else { mapOutput = latestMapOutput.current; @@ -65,6 +75,7 @@ export default function useSelect( _mapSelect, deps = [] ) { useIsomorphicLayoutEffect( () => { latestMapSelect.current = mapSelect; + latestDependencies.current = deps; if ( latestIsAsync.current !== isAsync ) { latestIsAsync.current = isAsync; renderQueue.flush( queueContext ); From 78d182b941aab85bb42ac8174a9976f242319ca4 Mon Sep 17 00:00:00 2001 From: Darren Ethier Date: Tue, 21 May 2019 22:14:24 -0400 Subject: [PATCH 16/22] =?UTF-8?q?and=E2=80=A6=20useCallback=20to=20the=20r?= =?UTF-8?q?escue.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../data/src/components/use-select/index.js | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/packages/data/src/components/use-select/index.js b/packages/data/src/components/use-select/index.js index 96cc1190cb8b1d..ae6fa51c4561e1 100644 --- a/packages/data/src/components/use-select/index.js +++ b/packages/data/src/components/use-select/index.js @@ -6,10 +6,11 @@ import { useLayoutEffect, useRef, useMemo, + useCallback, useEffect, useReducer, } from '@wordpress/element'; -import { isShallowEqualObjects, isShallowEqualArrays } from '@wordpress/is-shallow-equal'; +import { isShallowEqualObjects } from '@wordpress/is-shallow-equal'; /** * Internal dependencies @@ -30,7 +31,8 @@ const useIsomorphicLayoutEffect = const renderQueue = createQueue(); -export default function useSelect( mapSelect, deps ) { +export default function useSelect( _mapSelect, deps ) { + const mapSelect = useCallback( _mapSelect, deps ); const registry = useRegistry(); const isAsync = useAsyncMode(); const queueContext = useMemo( () => ( { queue: true } ), [ registry ] ); @@ -40,21 +42,13 @@ export default function useSelect( mapSelect, deps ) { const latestIsAsync = useRef( isAsync ); const latestMapOutput = useRef(); const latestMapOutputError = useRef(); - const latestDependencies = useRef(); const isMounted = useRef(); let mapOutput; - const hasSameMapSelect = latestMapSelect.current === mapSelect; - const hasSameDependencies = isShallowEqualArrays( - latestDependencies.current, - deps - ); - try { if ( - ! hasSameMapSelect || - ! hasSameDependencies || + latestMapSelect.current !== mapSelect || latestMapOutputError.current ) { mapOutput = mapSelect( registry.select, registry ); @@ -75,7 +69,6 @@ export default function useSelect( mapSelect, deps ) { useIsomorphicLayoutEffect( () => { latestMapSelect.current = mapSelect; - latestDependencies.current = deps; if ( latestIsAsync.current !== isAsync ) { latestIsAsync.current = isAsync; renderQueue.flush( queueContext ); From a757297e3bca23a296b042f65ae14178d585d928 Mon Sep 17 00:00:00 2001 From: Darren Ethier Date: Sat, 25 May 2019 13:19:39 -0400 Subject: [PATCH 17/22] cover scenarios where state updated _during_ mount before subscriptions are set. --- packages/data/src/components/use-select/index.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/data/src/components/use-select/index.js b/packages/data/src/components/use-select/index.js index ae6fa51c4561e1..64b975f7baeaad 100644 --- a/packages/data/src/components/use-select/index.js +++ b/packages/data/src/components/use-select/index.js @@ -93,11 +93,18 @@ export default function useSelect( _mapSelect, deps ) { } 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 ); From 1de3ee6ee4e19ae39a63c0cb40b677ee3f3df362 Mon Sep 17 00:00:00 2001 From: Darren Ethier Date: Sat, 25 May 2019 13:20:10 -0400 Subject: [PATCH 18/22] update with-select tests --- .../src/components/with-select/test/index.js | 399 ++++++++++++------ 1 file changed, 260 insertions(+), 139 deletions(-) diff --git a/packages/data/src/components/with-select/test/index.js b/packages/data/src/components/with-select/test/index.js index f42926e02266f7..8149e661e6a484 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 @@ -12,10 +12,10 @@ import { Component } from '@wordpress/element'; /** * Internal dependencies */ -import withSelect from '../'; +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( { From 5565d5cc2534d8a0a573d9073f616decefe54f0b Mon Sep 17 00:00:00 2001 From: Darren Ethier Date: Sat, 25 May 2019 13:20:39 -0400 Subject: [PATCH 19/22] add new hook tests --- .../src/components/use-select/test/index.js | 136 ++++++++++++++++++ 1 file changed, 136 insertions(+) create mode 100644 packages/data/src/components/use-select/test/index.js 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 00000000000000..ce8d15f3aa0707 --- /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', + } ); + } ); +} ); + From 71984eefe3abafd378aa9cb0a0ed27c9b2894d50 Mon Sep 17 00:00:00 2001 From: Darren Ethier Date: Sat, 25 May 2019 14:09:58 -0400 Subject: [PATCH 20/22] update tests --- .../src/components/with-dispatch/test/index.js | 2 +- .../viewport/src/test/if-viewport-matches.js | 18 +++++++++++++----- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/packages/data/src/components/with-dispatch/test/index.js b/packages/data/src/components/with-dispatch/test/index.js index 98f225fad2c406..4bde9b30810ac1 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/viewport/src/test/if-viewport-matches.js b/packages/viewport/src/test/if-viewport-matches.js index c5eca13c0ea14d..4a57039e9c7f1b 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 ); } ); } ); From c65f45fb7c3a8e43c4e634cf607771c30dc2cb01 Mon Sep 17 00:00:00 2001 From: Darren Ethier Date: Sat, 25 May 2019 15:10:32 -0400 Subject: [PATCH 21/22] code cleanup and add documentation --- packages/data/README.md | 167 +++++++++++++- .../components/registry-provider/context.js | 37 ++++ .../registry-provider/use-registry.js | 39 ++++ .../data/src/components/use-select/index.js | 43 ++++ .../data/src/components/with-select/index.js | 66 +++++- .../src/components/with-select/test/index.js | 2 +- .../components/with-select/with-select-new.js | 27 --- .../components/with-select/with-select-old.js | 208 ------------------ packages/data/src/index.js | 10 +- 9 files changed, 354 insertions(+), 245 deletions(-) delete mode 100644 packages/data/src/components/with-select/with-select-new.js delete mode 100644 packages/data/src/components/with-select/with-select-old.js diff --git a/packages/data/README.md b/packages/data/README.md index 83aeb72e4513ee..8984e86b1c5770 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. @@ -516,7 +633,47 @@ _Returns_ # **withSelect** -Undocumented declaration. +Higher-order component used to inject state-derived props using registered +selectors. + +_Usage_ + +```js +function PriceDisplay( { price, currency } ) { + return new Intl.NumberFormat( 'en-US', { + style: 'currency', + currency, + } ).format( price ); +} + +const { withSelect } = wp.data; + +const HammerPriceDisplay = withSelect( ( select, ownProps ) => { + const { getPrice } = select( 'my-shop' ); + const { currency } = ownProps; + + return { + price: getPrice( 'hammer', currency ), + }; +} )( PriceDisplay ); + +// Rendered in the application: +// +// +``` + +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_ + +- _mapSelectToProps_ `Function`: Function called on every state change, expected to return object of props to merge with the component's own props. + +_Returns_ + +- `Component`: Enhanced component with merged state data props. diff --git a/packages/data/src/components/registry-provider/context.js b/packages/data/src/components/registry-provider/context.js index 825b89ab9fcc5e..a1fe1a9e56183c 100644 --- a/packages/data/src/components/registry-provider/context.js +++ b/packages/data/src/components/registry-provider/context.js @@ -12,6 +12,43 @@ 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/use-registry.js b/packages/data/src/components/registry-provider/use-registry.js index 253737a14782eb..846cb5627fd2f3 100644 --- a/packages/data/src/components/registry-provider/use-registry.js +++ b/packages/data/src/components/registry-provider/use-registry.js @@ -8,6 +8,45 @@ import { useContext } from '@wordpress/element'; */ 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 index 64b975f7baeaad..d71f72b21c917f 100644 --- a/packages/data/src/components/use-select/index.js +++ b/packages/data/src/components/use-select/index.js @@ -31,6 +31,49 @@ const useIsomorphicLayoutEffect = 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(); diff --git a/packages/data/src/components/with-select/index.js b/packages/data/src/components/with-select/index.js index e1d04c6b48dc2d..f9659e7e865a8f 100644 --- a/packages/data/src/components/with-select/index.js +++ b/packages/data/src/components/with-select/index.js @@ -1,2 +1,66 @@ -export { default as withSelect } from './with-select-new'; +/** + * WordPress dependencies + */ +import { createHigherOrderComponent, pure } from '@wordpress/compose'; +/** + * Internal dependencies + */ +import useSelect from '../use-select'; + +/** + * Higher-order component used to inject state-derived props using registered + * selectors. + * + * @param {Function} mapSelectToProps Function called on every state change, + * expected to return object of props to + * merge with the component's own props. + * + * @example + * ```js + * function PriceDisplay( { price, currency } ) { + * return new Intl.NumberFormat( 'en-US', { + * style: 'currency', + * currency, + * } ).format( price ); + * } + * + * const { withSelect } = wp.data; + * + * const HammerPriceDisplay = withSelect( ( select, ownProps ) => { + * const { getPrice } = select( 'my-shop' ); + * const { currency } = ownProps; + * + * return { + * price: getPrice( 'hammer', currency ), + * }; + * } )( PriceDisplay ); + * + * // Rendered in the application: + * // + * // + * ``` + * 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 ) => pure( + ( ownProps ) => { + const mapSelect = + ( select, registry ) => mapSelectToProps( + select, + ownProps, + registry + ); + const mergeProps = useSelect( mapSelect ); + return ; + } + ), + '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 8149e661e6a484..3b2049826c63b5 100644 --- a/packages/data/src/components/with-select/test/index.js +++ b/packages/data/src/components/with-select/test/index.js @@ -12,7 +12,7 @@ import { Component } from '@wordpress/element'; /** * Internal dependencies */ -import { withSelect } from '../'; +import withSelect from '../'; import withDispatch from '../../with-dispatch'; import { createRegistry } from '../../../registry'; import { RegistryProvider } from '../../registry-provider'; diff --git a/packages/data/src/components/with-select/with-select-new.js b/packages/data/src/components/with-select/with-select-new.js deleted file mode 100644 index 2476d7a784d887..00000000000000 --- a/packages/data/src/components/with-select/with-select-new.js +++ /dev/null @@ -1,27 +0,0 @@ -/** - * WordPress dependencies - */ -import { createHigherOrderComponent, pure } from '@wordpress/compose'; - -/** - * Internal dependencies - */ -import useSelect from '../use-select'; - -const withSelect = ( mapSelectToProps ) => createHigherOrderComponent( - ( WrappedComponent ) => pure( - ( ownProps ) => { - const mapSelect = - ( select, registry ) => mapSelectToProps( - select, - ownProps, - registry - ); - const mergeProps = useSelect( mapSelect ); - return ; - } - ), - 'withSelect' -); - -export default withSelect; diff --git a/packages/data/src/components/with-select/with-select-old.js b/packages/data/src/components/with-select/with-select-old.js deleted file mode 100644 index d2fd4ae462b3bb..00000000000000 --- a/packages/data/src/components/with-select/with-select-old.js +++ /dev/null @@ -1,208 +0,0 @@ -/** - * 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'; - -/** - * Internal dependencies - */ -import { RegistryConsumer } from '../registry-provider'; -import { AsyncModeConsumer } from '../async-mode-provider'; - -const renderQueue = createQueue(); - -/** - * Higher-order component used to inject state-derived props using registered - * selectors. - * - * @param {Function} mapSelectToProps Function called on every state change, - * expected to return object of props to - * merge with the component's own props. - * - * @example - * ```js - * function PriceDisplay( { price, currency } ) { - * return new Intl.NumberFormat( 'en-US', { - * style: 'currency', - * currency, - * } ).format( price ); - * } - * - * const { withSelect } = wp.data; - * - * const HammerPriceDisplay = withSelect( ( select, ownProps ) => { - * const { getPrice } = select( 'my-shop' ); - * const { currency } = ownProps; - * - * return { - * price: getPrice( 'hammer', currency ), - * }; - * } )( PriceDisplay ); - * - * // Rendered in the application: - * // - * // - * ``` - * 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; - } - - 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' ); - -export default withSelect; diff --git a/packages/data/src/index.js b/packages/data/src/index.js index d9c7c32ea75fc1..46c830289b91d7 100644 --- a/packages/data/src/index.js +++ b/packages/data/src/index.js @@ -9,13 +9,17 @@ import combineReducers from 'turbo-combine-reducers'; import defaultRegistry from './default-registry'; import * as plugins from './plugins'; -export { withSelect } from './components/with-select'; +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 { RegistryProvider, RegistryConsumer } from './components/registry-provider'; +export { + RegistryProvider, + RegistryConsumer, + useRegistry, +} from './components/registry-provider'; +export { default as useSelect } from './components/use-select'; export { AsyncModeProvider as __experimentalAsyncModeProvider, - useAsyncMode as __experimentalUseAsyncMode, } from './components/async-mode-provider'; export { createRegistry } from './registry'; export { plugins }; From cf2e8247de258e57c1dd6a8a815d78db6f25df2a Mon Sep 17 00:00:00 2001 From: Darren Ethier Date: Sat, 25 May 2019 15:11:01 -0400 Subject: [PATCH 22/22] update changelog --- packages/data/CHANGELOG.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/data/CHANGELOG.md b/packages/data/CHANGELOG.md index 533101d02e8a87..391d68abe4c463 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