From 71f38887c13d1b5be6d285beef2cb5208494c7a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ella=20van=C2=A0Durpe?= Date: Wed, 16 Dec 2020 20:02:18 +0200 Subject: [PATCH 1/9] Try: useMergeRefs --- packages/components/src/modal/frame.js | 4 +-- packages/components/src/panel/body.js | 5 ++- packages/components/src/popover/index.js | 14 +++----- packages/compose/README.md | 8 +++++ .../src/hooks/use-callback-ref/index.js | 20 ----------- .../hooks/use-constrained-tabbing/index.js | 8 ++--- .../compose/src/hooks/use-dialog/index.js | 13 +++---- .../src/hooks/use-focus-return/index.js | 9 ++--- .../compose/src/hooks/use-merge-refs/index.js | 35 +++++++++++++++++++ packages/compose/src/index.js | 1 + 10 files changed, 60 insertions(+), 57 deletions(-) delete mode 100644 packages/compose/src/hooks/use-callback-ref/index.js create mode 100644 packages/compose/src/hooks/use-merge-refs/index.js diff --git a/packages/components/src/modal/frame.js b/packages/components/src/modal/frame.js index 8047832a6ec44..c1cbfd2d79a0b 100644 --- a/packages/components/src/modal/frame.js +++ b/packages/components/src/modal/frame.js @@ -2,7 +2,6 @@ * External dependencies */ import classnames from 'classnames'; -import mergeRefs from 'react-merge-refs'; /** * WordPress dependencies @@ -14,6 +13,7 @@ import { useFocusReturn, useFocusOnMount, useConstrainedTabbing, + useMergeRefs, } from '@wordpress/compose'; /** @@ -57,7 +57,7 @@ function ModalFrameContent( {
+
{ diff --git a/packages/compose/README.md b/packages/compose/README.md index 9fd08d2e0235a..2daa3a8d4f994 100644 --- a/packages/compose/README.md +++ b/packages/compose/README.md @@ -282,6 +282,14 @@ _Returns_ - `boolean`: return value of the media query. +# **useMergeRefs** + +Merges refs. + +_Parameters_ + +- _refs_ `Array`: + # **usePrevious** Use something's value from the previous render. diff --git a/packages/compose/src/hooks/use-callback-ref/index.js b/packages/compose/src/hooks/use-callback-ref/index.js deleted file mode 100644 index 0877607027acf..0000000000000 --- a/packages/compose/src/hooks/use-callback-ref/index.js +++ /dev/null @@ -1,20 +0,0 @@ -/** - * External dependencies - */ -import memize from 'memize'; - -/** - * WordPress dependencies - */ -import { useCallback } from '@wordpress/element'; - -function useCallbackRef( callback, deps ) { - return useCallback( - memize( callback, { - maxSize: 1, - } ), - deps - ); -} - -export default useCallbackRef; diff --git a/packages/compose/src/hooks/use-constrained-tabbing/index.js b/packages/compose/src/hooks/use-constrained-tabbing/index.js index 55011151a6a74..b7b3ce3d6eed7 100644 --- a/packages/compose/src/hooks/use-constrained-tabbing/index.js +++ b/packages/compose/src/hooks/use-constrained-tabbing/index.js @@ -3,11 +3,7 @@ */ import { TAB } from '@wordpress/keycodes'; import { focus } from '@wordpress/dom'; - -/** - * Internal dependencies - */ -import useCallbackRef from '../use-callback-ref'; +import { useCallback } from '@wordpress/element'; /** * In Dialogs/modals, the tabbing must be constrained to the content of @@ -31,7 +27,7 @@ import useCallbackRef from '../use-callback-ref'; * ``` */ function useConstrainedTabbing() { - const ref = useCallbackRef( ( node ) => { + const ref = useCallback( ( node ) => { if ( ! node ) { return; } diff --git a/packages/compose/src/hooks/use-dialog/index.js b/packages/compose/src/hooks/use-dialog/index.js index 3e0ab9d29dac8..777ab66b15b69 100644 --- a/packages/compose/src/hooks/use-dialog/index.js +++ b/packages/compose/src/hooks/use-dialog/index.js @@ -1,12 +1,7 @@ -/** - * External dependencies - */ -import mergeRefs from 'react-merge-refs'; - /** * WordPress dependencies */ -import { useRef, useEffect } from '@wordpress/element'; +import { useRef, useEffect, useCallback } from '@wordpress/element'; import { ESCAPE } from '@wordpress/keycodes'; /** @@ -16,7 +11,7 @@ import useConstrainedTabbing from '../use-constrained-tabbing'; import useFocusOnMount from '../use-focus-on-mount'; import useFocusReturn from '../use-focus-return'; import useFocusOutside from '../use-focus-outside'; -import useCallbackRef from '../use-callback-ref'; +import useMergeRefs from '../use-merge-refs'; /** * Returns a ref and props to apply to a dialog wrapper to enable the following behaviors: @@ -36,7 +31,7 @@ function useDialog( options ) { const focusOnMountRef = useFocusOnMount(); const focusReturnRef = useFocusReturn(); const focusOutsideProps = useFocusOutside( options.onClose ); - const closeOnEscapeRef = useCallbackRef( ( node ) => { + const closeOnEscapeRef = useCallback( ( node ) => { if ( ! node ) { return; } @@ -51,7 +46,7 @@ function useDialog( options ) { }, [] ); return [ - mergeRefs( [ + useMergeRefs( [ constrainedTabbingRef, focusReturnRef, focusOnMountRef, diff --git a/packages/compose/src/hooks/use-focus-return/index.js b/packages/compose/src/hooks/use-focus-return/index.js index 7f0d4ddad58c4..40187ff3dce3f 100644 --- a/packages/compose/src/hooks/use-focus-return/index.js +++ b/packages/compose/src/hooks/use-focus-return/index.js @@ -1,12 +1,7 @@ /** * WordPress dependencies */ -import { useRef, useEffect } from '@wordpress/element'; - -/** - * Internal dependencies - */ -import useCallbackRef from '../use-callback-ref'; +import { useRef, useEffect, useCallback } from '@wordpress/element'; /** * When opening modals/sidebars/dialogs, the focus @@ -40,7 +35,7 @@ function useFocusReturn( onFocusReturn ) { onFocusReturnRef.current = onFocusReturn; }, [ onFocusReturn ] ); - return useCallbackRef( ( node ) => { + return useCallback( ( node ) => { if ( node ) { // Set ref to be used when unmounting. ref.current = node; diff --git a/packages/compose/src/hooks/use-merge-refs/index.js b/packages/compose/src/hooks/use-merge-refs/index.js new file mode 100644 index 0000000000000..0fa3a3b9a9776 --- /dev/null +++ b/packages/compose/src/hooks/use-merge-refs/index.js @@ -0,0 +1,35 @@ +/** + * WordPress dependencies + */ +import { useRef, useCallback } from '@wordpress/element'; + +/** + * Merges refs. + * + * @param {Array} refs + */ +export default function useMergeRefs( refs ) { + const lastValue = useRef( null ); + const lastRefs = useRef( refs ); + + return useCallback( ( value ) => { + refs.forEach( ( ref, index ) => { + if ( typeof ref === 'function' ) { + // Only call a ref when it wants to be called: + // - The value changes. + // - The ref callback has changed (e.g. an updated dependency). + if ( + value !== lastValue.current || + lastRefs[ index ] !== ref + ) { + ref( value ); + } + } else if ( ref ) { + ref.current = value; + } + } ); + + lastValue.current = value; + lastRefs.current = refs; + }, refs ); +} diff --git a/packages/compose/src/index.js b/packages/compose/src/index.js index 884fe0e8ce2e2..4434827d5b44a 100644 --- a/packages/compose/src/index.js +++ b/packages/compose/src/index.js @@ -32,3 +32,4 @@ export { default as useAsyncList } from './hooks/use-async-list'; export { default as useWarnOnChange } from './hooks/use-warn-on-change'; export { default as useDebounce } from './hooks/use-debounce'; export { default as useThrottle } from './hooks/use-throttle'; +export { default as useMergeRefs } from './hooks/use-merge-refs'; From 7634d8bf79f86f71667ce407c91914c55c6acb14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ella=20van=C2=A0Durpe?= Date: Wed, 16 Dec 2020 20:18:19 +0200 Subject: [PATCH 2/9] update lock file --- package-lock.json | 8 +++----- packages/components/package.json | 1 - packages/compose/package.json | 1 - 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/package-lock.json b/package-lock.json index 40a17fd567265..0da022aaf7942 100644 --- a/package-lock.json +++ b/package-lock.json @@ -11964,7 +11964,6 @@ "moment": "^2.22.1", "re-resizable": "^6.4.0", "react-dates": "^17.1.1", - "react-merge-refs": "^1.0.0", "react-resize-aware": "^3.1.0", "react-spring": "^8.0.20", "react-use-gesture": "^9.0.0", @@ -12119,7 +12118,6 @@ "lodash": "^4.17.19", "memize": "^1.1.0", "mousetrap": "^1.6.5", - "react-merge-refs": "^1.0.0", "react-resize-aware": "^3.1.0", "use-memo-one": "^1.1.1" } @@ -47127,9 +47125,9 @@ "dev": true }, "react-merge-refs": { - "version": "1.0.0", - "resolved": "https://registry.npmjs.org/react-merge-refs/-/react-merge-refs-1.0.0.tgz", - "integrity": "sha512-VkvWuCR5VoTjb+VYUcOjkFo66HDv1Hw8VjKcwQtWr2lJnT8g7epRRyfz8+Zkl2WhwqNeqR0gIe0XYrBa9ePeXg==" + "version": "1.1.0", + "resolved": "https://registry.npmjs.org/react-merge-refs/-/react-merge-refs-1.1.0.tgz", + "integrity": "sha512-alTKsjEL0dKH/ru1Iyn7vliS2QRcBp9zZPGoWxUOvRGWPUYgjo+V01is7p04It6KhgrzhJGnIj9GgX8W4bZoCQ==" }, "react-moment-proptypes": { "version": "1.7.0", diff --git a/packages/components/package.json b/packages/components/package.json index 5a31809d62164..25c868d8c702e 100644 --- a/packages/components/package.json +++ b/packages/components/package.json @@ -59,7 +59,6 @@ "moment": "^2.22.1", "re-resizable": "^6.4.0", "react-dates": "^17.1.1", - "react-merge-refs": "^1.0.0", "react-resize-aware": "^3.1.0", "react-spring": "^8.0.20", "react-use-gesture": "^9.0.0", diff --git a/packages/compose/package.json b/packages/compose/package.json index cb7f062559d3c..6f6a7006fa3a0 100644 --- a/packages/compose/package.json +++ b/packages/compose/package.json @@ -36,7 +36,6 @@ "lodash": "^4.17.19", "memize": "^1.1.0", "mousetrap": "^1.6.5", - "react-merge-refs": "^1.0.0", "react-resize-aware": "^3.1.0", "use-memo-one": "^1.1.1" }, From 1fc90c03a8b49f92afda97e2132aa42c51057112 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ella=20van=C2=A0Durpe?= Date: Wed, 16 Dec 2020 20:20:04 +0200 Subject: [PATCH 3/9] Rebase fixes --- packages/compose/src/hooks/use-focus-on-mount/index.js | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/packages/compose/src/hooks/use-focus-on-mount/index.js b/packages/compose/src/hooks/use-focus-on-mount/index.js index 54492d8b140f7..b0eb7184b6c44 100644 --- a/packages/compose/src/hooks/use-focus-on-mount/index.js +++ b/packages/compose/src/hooks/use-focus-on-mount/index.js @@ -1,14 +1,9 @@ /** * WordPress dependencies */ -import { useRef, useEffect } from '@wordpress/element'; +import { useRef, useEffect, useCallback } from '@wordpress/element'; import { focus } from '@wordpress/dom'; -/** - * Internal dependencies - */ -import useCallbackRef from '../use-callback-ref'; - /** * Hook used to focus the first tabbable element on mount. * @@ -36,7 +31,7 @@ export default function useFocusOnMount( focusOnMount = 'firstElement' ) { focusOnMountRef.current = focusOnMount; }, [ focusOnMount ] ); - return useCallbackRef( ( node ) => { + return useCallback( ( node ) => { if ( ! node || focusOnMountRef.current === false ) { return; } From f17968cd82c7134f5e576194823a7f17970cd762 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ella=20van=C2=A0Durpe?= Date: Wed, 16 Dec 2020 20:54:34 +0200 Subject: [PATCH 4/9] Add docs --- packages/compose/README.md | 8 ++++++-- packages/compose/src/hooks/use-merge-refs/index.js | 13 ++++++++++--- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/packages/compose/README.md b/packages/compose/README.md index 2daa3a8d4f994..170041003d8ae 100644 --- a/packages/compose/README.md +++ b/packages/compose/README.md @@ -284,11 +284,15 @@ _Returns_ # **useMergeRefs** -Merges refs. +Merges refs into one ref callback. Ensures the merged ref callbacks are only +called when it changes or when the ref value changes. If you don't wish a ref +callback to be called on every render, wrap it with `useCallback( ref, [] )`. +Dependencies can be added, but when a dependency changes, the ref callback +will be called with the same node. _Parameters_ -- _refs_ `Array`: +- _refs_ `Array<(RefObject|RefCallback)>`: The refs to be merged. # **usePrevious** diff --git a/packages/compose/src/hooks/use-merge-refs/index.js b/packages/compose/src/hooks/use-merge-refs/index.js index 0fa3a3b9a9776..716b6767d93f6 100644 --- a/packages/compose/src/hooks/use-merge-refs/index.js +++ b/packages/compose/src/hooks/use-merge-refs/index.js @@ -3,10 +3,17 @@ */ import { useRef, useCallback } from '@wordpress/element'; +/** @typedef {import('@wordpress/element').RefObject} RefObject */ +/** @typedef {import('@wordpress/element').RefCallback} RefCallback */ + /** - * Merges refs. + * Merges refs into one ref callback. Ensures the merged ref callbacks are only + * called when it changes or when the ref value changes. If you don't wish a ref + * callback to be called on every render, wrap it with `useCallback( ref, [] )`. + * Dependencies can be added, but when a dependency changes, the ref callback + * will be called with the same node. * - * @param {Array} refs + * @param {Array} refs The refs to be merged. */ export default function useMergeRefs( refs ) { const lastValue = useRef( null ); @@ -24,7 +31,7 @@ export default function useMergeRefs( refs ) { ) { ref( value ); } - } else if ( ref ) { + } else if ( ref && ref.hasOwnProperty( 'current' ) ) { ref.current = value; } } ); From fa4cd010353b7a716323a3aac91f3b6a85fc6006 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ella=20van=C2=A0Durpe?= Date: Wed, 16 Dec 2020 21:48:42 +0200 Subject: [PATCH 5/9] Clearer documentation --- packages/compose/README.md | 14 +++++++---- .../compose/src/hooks/use-merge-refs/index.js | 24 ++++++++++++------- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/packages/compose/README.md b/packages/compose/README.md index 170041003d8ae..de6183d328ff8 100644 --- a/packages/compose/README.md +++ b/packages/compose/README.md @@ -285,15 +285,21 @@ _Returns_ # **useMergeRefs** Merges refs into one ref callback. Ensures the merged ref callbacks are only -called when it changes or when the ref value changes. If you don't wish a ref -callback to be called on every render, wrap it with `useCallback( ref, [] )`. -Dependencies can be added, but when a dependency changes, the ref callback -will be called with the same node. +called when it changes (as a result of a `useCallback` dependency update) or +when the ref value changes. If you don't wish a ref callback to be called on +every render, wrap it with `useCallback( ref, [] )`. +Dependencies can be added, but when a dependency changes, the old ref +callback will be called with `null` and the new ref callback will be called +with the same node. _Parameters_ - _refs_ `Array<(RefObject|RefCallback)>`: The refs to be merged. +_Returns_ + +- `RefCallback`: The merged ref callback. + # **usePrevious** Use something's value from the previous render. diff --git a/packages/compose/src/hooks/use-merge-refs/index.js b/packages/compose/src/hooks/use-merge-refs/index.js index 716b6767d93f6..683ff0bebb745 100644 --- a/packages/compose/src/hooks/use-merge-refs/index.js +++ b/packages/compose/src/hooks/use-merge-refs/index.js @@ -8,12 +8,16 @@ import { useRef, useCallback } from '@wordpress/element'; /** * Merges refs into one ref callback. Ensures the merged ref callbacks are only - * called when it changes or when the ref value changes. If you don't wish a ref - * callback to be called on every render, wrap it with `useCallback( ref, [] )`. - * Dependencies can be added, but when a dependency changes, the ref callback - * will be called with the same node. + * called when it changes (as a result of a `useCallback` dependency update) or + * when the ref value changes. If you don't wish a ref callback to be called on + * every render, wrap it with `useCallback( ref, [] )`. + * Dependencies can be added, but when a dependency changes, the old ref + * callback will be called with `null` and the new ref callback will be called + * with the same node. * * @param {Array} refs The refs to be merged. + * + * @return {RefCallback} The merged ref callback. */ export default function useMergeRefs( refs ) { const lastValue = useRef( null ); @@ -22,12 +26,14 @@ export default function useMergeRefs( refs ) { return useCallback( ( value ) => { refs.forEach( ( ref, index ) => { if ( typeof ref === 'function' ) { - // Only call a ref when it wants to be called: - // - The value changes. - // - The ref callback has changed (e.g. an updated dependency). + // Only call a ref callback if it has changes, e.g. a dependency + // change in `useCallback`, EXCEPT if the value changes, then + // the ref callback must always be called. + // Any other ref callbacks WON'T be called if it doesn't change, + // e.g. if no `useCallback` dependencies change. if ( - value !== lastValue.current || - lastRefs[ index ] !== ref + lastRefs[ index ] !== ref || + value !== lastValue.current ) { ref( value ); } From 20c897acca4a87f23f65a26d601b4d211693adf7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ella=20van=C2=A0Durpe?= Date: Thu, 17 Dec 2020 01:08:43 +0200 Subject: [PATCH 6/9] wip --- .../compose/src/hooks/use-merge-refs/index.js | 62 +++-- .../src/hooks/use-merge-refs/test/index.js | 249 ++++++++++++++++++ 2 files changed, 292 insertions(+), 19 deletions(-) create mode 100644 packages/compose/src/hooks/use-merge-refs/test/index.js diff --git a/packages/compose/src/hooks/use-merge-refs/index.js b/packages/compose/src/hooks/use-merge-refs/index.js index 683ff0bebb745..864d460a96adf 100644 --- a/packages/compose/src/hooks/use-merge-refs/index.js +++ b/packages/compose/src/hooks/use-merge-refs/index.js @@ -1,7 +1,7 @@ /** * WordPress dependencies */ -import { useRef, useCallback } from '@wordpress/element'; +import { useRef, useCallback, useLayoutEffect } from '@wordpress/element'; /** @typedef {import('@wordpress/element').RefObject} RefObject */ /** @typedef {import('@wordpress/element').RefCallback} RefCallback */ @@ -20,29 +20,53 @@ import { useRef, useCallback } from '@wordpress/element'; * @return {RefCallback} The merged ref callback. */ export default function useMergeRefs( refs ) { - const lastValue = useRef( null ); - const lastRefs = useRef( refs ); + const element = useRef( null ); + const didElementChange = useRef( false ); + const previousRefs = useRef( refs ); + const currentRefs = useRef( refs ); - return useCallback( ( value ) => { + // Update on render before the ref callback is called, so the ref callback + // always has access to the current refs. + currentRefs.current = refs; + + // If any of the refs change, call the previous ref with `null` and the new + // ref with the node, except when the element changes in the same cycle, in + // which case the ref callbacks will already have been called. + useLayoutEffect( () => { refs.forEach( ( ref, index ) => { + if ( + typeof ref === 'function' && + ref !== previousRefs.current[ index ] && + didElementChange.current === false + ) { + previousRefs.current[ index ]( null ); + ref( element.current ); + } + } ); + + previousRefs.current = refs; + didElementChange.current = false; + }, refs ); + + // There should be no dependencies so that `callback` is only called when + // the node changes. + return useCallback( ( value ) => { + // Update the element so it can be used when calling ref callbacks on a + // dependency change. + element.current = value; + didElementChange.current = true; + + // When an element changes, the current ref callback should be called + // with the new element and the previous one with `null`. + const refsToUpdate = value ? currentRefs.current : previousRefs.current; + + // Update the latest refs. + refsToUpdate.forEach( ( ref ) => { if ( typeof ref === 'function' ) { - // Only call a ref callback if it has changes, e.g. a dependency - // change in `useCallback`, EXCEPT if the value changes, then - // the ref callback must always be called. - // Any other ref callbacks WON'T be called if it doesn't change, - // e.g. if no `useCallback` dependencies change. - if ( - lastRefs[ index ] !== ref || - value !== lastValue.current - ) { - ref( value ); - } + ref( value ); } else if ( ref && ref.hasOwnProperty( 'current' ) ) { ref.current = value; } } ); - - lastValue.current = value; - lastRefs.current = refs; - }, refs ); + }, [] ); } diff --git a/packages/compose/src/hooks/use-merge-refs/test/index.js b/packages/compose/src/hooks/use-merge-refs/test/index.js new file mode 100644 index 0000000000000..e37cd974d1fde --- /dev/null +++ b/packages/compose/src/hooks/use-merge-refs/test/index.js @@ -0,0 +1,249 @@ +/** + * External dependencies + */ +import ReactDOM from 'react-dom'; + +/** + * WordPress dependencies + */ +import { useCallback } from '@wordpress/element'; + +/** + * Internal dependencies + */ +import useMergeRefs from '../'; + +describe( 'useMergeRefs', () => { + beforeAll( () => { + const rootElement = document.createElement( 'div' ); + rootElement.id = 'root'; + document.body.appendChild( rootElement ); + } ); + + it( 'should work', () => { + const renderCallback = jest.fn(); + const refCallback1 = jest.fn(); + + function MergedRefs() { + renderCallback(); + return ( +
+ ); + } + + const rootElement = document.getElementById( 'root' ); + + ReactDOM.render( , rootElement ); + + expect( renderCallback ).toHaveBeenCalledTimes( 1 ); + expect( refCallback1 ).toHaveBeenCalledTimes( 1 ); + expect( refCallback1 ).toHaveBeenCalledWith( + rootElement.firstElementChild + ); + + ReactDOM.render( , rootElement ); + + expect( renderCallback ).toHaveBeenCalledTimes( 2 ); + expect( refCallback1 ).toHaveBeenCalledTimes( 1 ); + expect( refCallback1 ).toHaveBeenCalledWith( + rootElement.firstElementChild + ); + } ); + + it( 'should work with 2', () => { + const refCallback1 = jest.fn(); + const refCallback2 = jest.fn(); + + function MergedRefs() { + return ( +
+ ); + } + + const rootElement = document.getElementById( 'root' ); + + ReactDOM.render( , rootElement ); + + expect( refCallback1 ).toHaveBeenCalledTimes( 1 ); + expect( refCallback1 ).toHaveBeenCalledWith( + rootElement.firstElementChild + ); + expect( refCallback2 ).toHaveBeenCalledTimes( 1 ); + expect( refCallback2 ).toHaveBeenCalledWith( + rootElement.firstElementChild + ); + } ); + + it( 'should work for node change', () => { + const renderCallback = jest.fn(); + + function refCallback1( value ) { + refCallback1.history = refCallback1.history || []; + refCallback1.history.push( value ); + } + + function MergedRefs( { tagName: TagName = 'div' } ) { + renderCallback(); + return ( + + ); + } + + const rootElement = document.getElementById( 'root' ); + + ReactDOM.render( , rootElement ); + + const originalElement = rootElement.firstElementChild; + + expect( renderCallback ).toHaveBeenCalledTimes( 1 ); + expect( refCallback1.history ).toEqual( [ originalElement ] ); + + ReactDOM.render( , rootElement ); + + expect( renderCallback ).toHaveBeenCalledTimes( 2 ); + expect( refCallback1.history ).toEqual( [ + originalElement, + null, + rootElement.firstElementChild, + ] ); + } ); + + it( 'should work with 2 different deps', () => { + function renderCallback( args ) { + renderCallback.history.push( args ); + } + + renderCallback.history = []; + + function MergedRefs( { count } ) { + function refCallback1( value ) { + refCallback1.history.push( value ); + } + + refCallback1.history = []; + + function refCallback2( value ) { + refCallback2.history.push( value ); + } + + refCallback2.history = []; + + renderCallback( [ refCallback1.history, refCallback2.history ] ); + + return ( +
+ ); + } + + const rootElement = document.getElementById( 'root' ); + + ReactDOM.render( , rootElement ); + + const originalElement = rootElement.firstElementChild; + + expect( renderCallback.history ).toEqual( [ + [ [ originalElement ], [ originalElement ] ], + ] ); + + ReactDOM.render( , rootElement ); + + expect( renderCallback.history ).toEqual( [ + [ + // Expect the "old" callback 1 to be called with the node. + [ originalElement ], + // Expect the "old" callback 2 to be called with the node, then + // `null`. + [ originalElement, null ], + ], + [ + // Don't expect the "new" callback 1 to be called. + // No dependencies or node have changed. + [], + // Expect the "new" callback 2 to be called with the node since + // a dependency has updated. + [ originalElement ], + ], + ] ); + } ); + + it( 'should simultaneously update node and dependencies', () => { + function renderCallback( args ) { + renderCallback.history.push( args ); + } + + renderCallback.history = []; + + function MergedRefs( { count, tagName: TagName = 'div' } ) { + function refCallback1( value ) { + refCallback1.history.push( value ); + } + + refCallback1.history = []; + + function refCallback2( value ) { + refCallback2.history.push( value ); + } + + refCallback2.history = []; + + renderCallback( [ refCallback1.history, refCallback2.history ] ); + + return ( + + ); + } + + const rootElement = document.getElementById( 'root' ); + + ReactDOM.render( , rootElement ); + + const originalElement = rootElement.firstElementChild; + + expect( renderCallback.history ).toEqual( [ + [ [ originalElement ], [ originalElement ] ], + ] ); + + ReactDOM.render( + , + rootElement + ); + + expect( renderCallback.history ).toEqual( [ + [ + // Expect the "old" callback 1 to be called with the node, + // then null and then the new node. + [ originalElement, null, rootElement.firstElementChild ], + // Expect the "old" callback 2 to be called with the node, then + // `null`. + [ originalElement, null ], + ], + [ + // Don't expect the "new" callback 1 to be called since + // No dependencies or node have changed. + [], + // Expect the "new" callback 2 to be called with the node since + // a dependency has updated. + [ rootElement.firstElementChild ], + ], + ] ); + } ); +} ); From 620c5e9334f0f0da16f69a93833ff135b2646b35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ella=20van=C2=A0Durpe?= Date: Thu, 17 Dec 2020 10:28:00 +0200 Subject: [PATCH 7/9] wip --- .../compose/src/hooks/use-merge-refs/index.js | 6 ++- .../src/hooks/use-merge-refs/test/index.js | 42 +++++++++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/packages/compose/src/hooks/use-merge-refs/index.js b/packages/compose/src/hooks/use-merge-refs/index.js index 864d460a96adf..0a6d012b2b5a8 100644 --- a/packages/compose/src/hooks/use-merge-refs/index.js +++ b/packages/compose/src/hooks/use-merge-refs/index.js @@ -34,12 +34,14 @@ export default function useMergeRefs( refs ) { // which case the ref callbacks will already have been called. useLayoutEffect( () => { refs.forEach( ( ref, index ) => { + const previousRef = previousRefs.current[ index ]; + if ( typeof ref === 'function' && - ref !== previousRefs.current[ index ] && + ref !== previousRef && didElementChange.current === false ) { - previousRefs.current[ index ]( null ); + previousRef( null ); ref( element.current ); } } ); diff --git a/packages/compose/src/hooks/use-merge-refs/test/index.js b/packages/compose/src/hooks/use-merge-refs/test/index.js index e37cd974d1fde..3da6b1e991a7c 100644 --- a/packages/compose/src/hooks/use-merge-refs/test/index.js +++ b/packages/compose/src/hooks/use-merge-refs/test/index.js @@ -246,4 +246,46 @@ describe( 'useMergeRefs', () => { ], ] ); } ); + + it( 'should unmount', () => { + function renderCallback( args ) { + renderCallback.history.push( args ); + } + + renderCallback.history = []; + + function MergedRefs() { + function refCallback1( value ) { + refCallback1.history.push( value ); + } + + refCallback1.history = []; + + renderCallback( [ refCallback1.history ] ); + + return ( +
+ ); + } + + const rootElement = document.getElementById( 'root' ); + + ReactDOM.render( , rootElement ); + + const originalElement = rootElement.firstElementChild; + + expect( renderCallback.history ).toEqual( [ [ [ originalElement ] ] ] ); + + ReactDOM.render( null, rootElement ); + + expect( renderCallback.history ).toEqual( [ + [ + // Expect the "old" callback 1 to be called with the node, + // then null and then the new node. + [ originalElement, null ], + ], + ] ); + } ); } ); From 620598f6c3656b8d00e15464f8baf126c6603258 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ella=20van=C2=A0Durpe?= Date: Thu, 17 Dec 2020 17:39:08 +0200 Subject: [PATCH 8/9] Add docs for unit tests --- .../src/hooks/use-merge-refs/test/index.js | 318 +++++++----------- 1 file changed, 124 insertions(+), 194 deletions(-) diff --git a/packages/compose/src/hooks/use-merge-refs/test/index.js b/packages/compose/src/hooks/use-merge-refs/test/index.js index 3da6b1e991a7c..0c156abe0c0a5 100644 --- a/packages/compose/src/hooks/use-merge-refs/test/index.js +++ b/packages/compose/src/hooks/use-merge-refs/test/index.js @@ -14,141 +14,135 @@ import { useCallback } from '@wordpress/element'; import useMergeRefs from '../'; describe( 'useMergeRefs', () => { - beforeAll( () => { - const rootElement = document.createElement( 'div' ); - rootElement.id = 'root'; - document.body.appendChild( rootElement ); - } ); - - it( 'should work', () => { - const renderCallback = jest.fn(); - const refCallback1 = jest.fn(); - - function MergedRefs() { - renderCallback(); - return ( -
- ); + // Setup + // ===== + // + // A component with two merged ref callbacks. The second has a dependency, + // the first does not. We expect the one with the dependency to be called + // with null in the old function and the node in the new function. We don't + // expect the first ref callback to be called **unless** the node changes. + // There's a prop controlling the tag name, which can be used to trigger a + // node change. In that case we expect both ref callbacks to be called with + // null in the old function and the **new** node in the new function. + // + // The history of all functions is recorded. Note that a new function is + // created on every render, which will all be tracked. Some functions are + // never expected to be called on subsequent renders if no callback + // dependency updates! + + function renderCallback( args ) { + renderCallback.history.push( args ); + } + + renderCallback.history = []; + + function MergedRefs( { count, tagName: TagName = 'div' } ) { + function refCallback1( value ) { + refCallback1.history.push( value ); } - const rootElement = document.getElementById( 'root' ); + refCallback1.history = []; - ReactDOM.render( , rootElement ); + function refCallback2( value ) { + refCallback2.history.push( value ); + } - expect( renderCallback ).toHaveBeenCalledTimes( 1 ); - expect( refCallback1 ).toHaveBeenCalledTimes( 1 ); - expect( refCallback1 ).toHaveBeenCalledWith( - rootElement.firstElementChild - ); + refCallback2.history = []; - ReactDOM.render( , rootElement ); + renderCallback( [ refCallback1.history, refCallback2.history ] ); - expect( renderCallback ).toHaveBeenCalledTimes( 2 ); - expect( refCallback1 ).toHaveBeenCalledTimes( 1 ); - expect( refCallback1 ).toHaveBeenCalledWith( - rootElement.firstElementChild + return ( + ); + } + + beforeEach( () => { + const rootElement = document.createElement( 'div' ); + rootElement.id = 'root'; + document.body.appendChild( rootElement ); } ); - it( 'should work with 2', () => { - const refCallback1 = jest.fn(); - const refCallback2 = jest.fn(); - - function MergedRefs() { - return ( -
- ); - } + afterEach( () => { + // Reset all history and DOM. + renderCallback.history = []; + document.body.innerHTML = ''; + } ); + it( 'should work', () => { const rootElement = document.getElementById( 'root' ); ReactDOM.render( , rootElement ); - expect( refCallback1 ).toHaveBeenCalledTimes( 1 ); - expect( refCallback1 ).toHaveBeenCalledWith( - rootElement.firstElementChild - ); - expect( refCallback2 ).toHaveBeenCalledTimes( 1 ); - expect( refCallback2 ).toHaveBeenCalledWith( - rootElement.firstElementChild - ); - } ); - - it( 'should work for node change', () => { - const renderCallback = jest.fn(); - - function refCallback1( value ) { - refCallback1.history = refCallback1.history || []; - refCallback1.history.push( value ); - } - - function MergedRefs( { tagName: TagName = 'div' } ) { - renderCallback(); - return ( - - ); - } + const originalElement = rootElement.firstElementChild; - const rootElement = document.getElementById( 'root' ); + // Render 1: both initial callback functions should be called with node. + expect( renderCallback.history ).toEqual( [ + [ [ originalElement ], [ originalElement ] ], + ] ); ReactDOM.render( , rootElement ); - const originalElement = rootElement.firstElementChild; - - expect( renderCallback ).toHaveBeenCalledTimes( 1 ); - expect( refCallback1.history ).toEqual( [ originalElement ] ); + // Render 2: the new callback functions should not be called! There has + // been no dependency change. + expect( renderCallback.history ).toEqual( [ + [ [ originalElement ], [ originalElement ] ], + [ [], [] ], + ] ); - ReactDOM.render( , rootElement ); + ReactDOM.render( null, rootElement ); - expect( renderCallback ).toHaveBeenCalledTimes( 2 ); - expect( refCallback1.history ).toEqual( [ - originalElement, - null, - rootElement.firstElementChild, + // Unmount: the initial callback functions should receive null. + expect( renderCallback.history ).toEqual( [ + [ + [ originalElement, null ], + [ originalElement, null ], + ], + [ [], [] ], ] ); } ); - it( 'should work with 2 different deps', () => { - function renderCallback( args ) { - renderCallback.history.push( args ); - } + it( 'should work for node change', () => { + const rootElement = document.getElementById( 'root' ); - renderCallback.history = []; + ReactDOM.render( , rootElement ); - function MergedRefs( { count } ) { - function refCallback1( value ) { - refCallback1.history.push( value ); - } + const originalElement = rootElement.firstElementChild; - refCallback1.history = []; + ReactDOM.render( , rootElement ); - function refCallback2( value ) { - refCallback2.history.push( value ); - } + const newElement = rootElement.firstElementChild; - refCallback2.history = []; + // After a render with the original element and a second render with the + // new element, expect the initial callback functions to be called with + // the original element, then null, then the new element. + // Again, the new callback functions should not be called! There has + // been no dependency change. + expect( renderCallback.history ).toEqual( [ + [ + [ originalElement, null, newElement ], + [ originalElement, null, newElement ], + ], + [ [], [] ], + ] ); - renderCallback( [ refCallback1.history, refCallback2.history ] ); + ReactDOM.render( null, rootElement ); - return ( -
- ); - } + // Unmount: the initial callback functions should receive null. + expect( renderCallback.history ).toEqual( [ + [ + [ originalElement, null, newElement, null ], + [ originalElement, null, newElement, null ], + ], + [ [], [] ], + ] ); + } ); + it( 'should work with dependencies', () => { const rootElement = document.getElementById( 'root' ); ReactDOM.render( , rootElement ); @@ -161,57 +155,28 @@ describe( 'useMergeRefs', () => { ReactDOM.render( , rootElement ); + // After a second render with a dependency change, expect the inital + // callback function to be called with null and the new callback + // function to be called with the original node. Note that for callback + // one no dependencies have changed. + expect( renderCallback.history ).toEqual( [ + [ [ originalElement ], [ originalElement, null ] ], + [ [], [ originalElement ] ], + ] ); + + ReactDOM.render( null, rootElement ); + + // Unmount: current callback functions should be called with null. expect( renderCallback.history ).toEqual( [ [ - // Expect the "old" callback 1 to be called with the node. - [ originalElement ], - // Expect the "old" callback 2 to be called with the node, then - // `null`. + [ originalElement, null ], [ originalElement, null ], ], - [ - // Don't expect the "new" callback 1 to be called. - // No dependencies or node have changed. - [], - // Expect the "new" callback 2 to be called with the node since - // a dependency has updated. - [ originalElement ], - ], + [ [], [ originalElement, null ] ], ] ); } ); it( 'should simultaneously update node and dependencies', () => { - function renderCallback( args ) { - renderCallback.history.push( args ); - } - - renderCallback.history = []; - - function MergedRefs( { count, tagName: TagName = 'div' } ) { - function refCallback1( value ) { - refCallback1.history.push( value ); - } - - refCallback1.history = []; - - function refCallback2( value ) { - refCallback2.history.push( value ); - } - - refCallback2.history = []; - - renderCallback( [ refCallback1.history, refCallback2.history ] ); - - return ( - - ); - } - const rootElement = document.getElementById( 'root' ); ReactDOM.render( , rootElement ); @@ -227,65 +192,30 @@ describe( 'useMergeRefs', () => { rootElement ); + const newElement = rootElement.firstElementChild; + + // Both the node changes and the dependencies update for the second + // callback, so expect the old callback function to be called with null + // and the new callback function to be called with the **new** node. + // For the first callback, we expect the initial function to be called + // with null and then the new node since no dependencies have changed. expect( renderCallback.history ).toEqual( [ [ - // Expect the "old" callback 1 to be called with the node, - // then null and then the new node. - [ originalElement, null, rootElement.firstElementChild ], - // Expect the "old" callback 2 to be called with the node, then - // `null`. + [ originalElement, null, newElement ], [ originalElement, null ], ], - [ - // Don't expect the "new" callback 1 to be called since - // No dependencies or node have changed. - [], - // Expect the "new" callback 2 to be called with the node since - // a dependency has updated. - [ rootElement.firstElementChild ], - ], + [ [], [ newElement ] ], ] ); - } ); - - it( 'should unmount', () => { - function renderCallback( args ) { - renderCallback.history.push( args ); - } - - renderCallback.history = []; - - function MergedRefs() { - function refCallback1( value ) { - refCallback1.history.push( value ); - } - - refCallback1.history = []; - - renderCallback( [ refCallback1.history ] ); - - return ( -
- ); - } - - const rootElement = document.getElementById( 'root' ); - - ReactDOM.render( , rootElement ); - - const originalElement = rootElement.firstElementChild; - - expect( renderCallback.history ).toEqual( [ [ [ originalElement ] ] ] ); ReactDOM.render( null, rootElement ); + // Unmount: current callback functions should be called with null. expect( renderCallback.history ).toEqual( [ [ - // Expect the "old" callback 1 to be called with the node, - // then null and then the new node. + [ originalElement, null, newElement, null ], [ originalElement, null ], ], + [ [], [ newElement, null ] ], ] ); } ); } ); From b3d1a12ea1e237da432b65f8cbeb8a49a15c2f07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ella=20van=C2=A0Durpe?= Date: Mon, 8 Feb 2021 14:42:31 +0200 Subject: [PATCH 9/9] Convert new additions --- package-lock.json | 11 ++--------- packages/block-editor/package.json | 1 - .../block-editor/src/components/iframe/index.js | 4 ++-- packages/edit-post/package.json | 1 - .../src/components/visual-editor/index.js | 14 +++----------- packages/interface/package.json | 4 ++-- .../src/components/interface-skeleton/index.js | 9 +++------ 7 files changed, 12 insertions(+), 32 deletions(-) diff --git a/package-lock.json b/package-lock.json index 0da022aaf7942..d4f6796acd4ce 100644 --- a/package-lock.json +++ b/package-lock.json @@ -11835,7 +11835,6 @@ "lodash": "^4.17.19", "memize": "^1.1.0", "react-autosize-textarea": "^7.1.0", - "react-merge-refs": "^1.0.0", "react-spring": "^8.0.19", "redux-multi": "^0.1.12", "rememo": "^3.0.0", @@ -12336,7 +12335,6 @@ "classnames": "^2.2.5", "lodash": "^4.17.19", "memize": "^1.1.0", - "react-merge-refs": "^1.0.0", "rememo": "^3.0.0" } }, @@ -12558,6 +12556,7 @@ "requires": { "@babel/runtime": "^7.12.5", "@wordpress/components": "file:packages/components", + "@wordpress/compose": "file:packages/compose", "@wordpress/data": "file:packages/data", "@wordpress/deprecated": "file:packages/deprecated", "@wordpress/element": "file:packages/element", @@ -12566,8 +12565,7 @@ "@wordpress/plugins": "file:packages/plugins", "@wordpress/viewport": "file:packages/viewport", "classnames": "^2.2.5", - "lodash": "^4.17.19", - "react-merge-refs": "^1.0.0" + "lodash": "^4.17.19" } }, "@wordpress/is-shallow-equal": { @@ -47124,11 +47122,6 @@ "integrity": "sha512-fBASbA6LnOU9dOU2eW7aQ8xmYBSXUIWr+UmF9b1efZBazGNO+rcXT/icdKnYm2pTwcRylVUYwW7H1PHfLekVzA==", "dev": true }, - "react-merge-refs": { - "version": "1.1.0", - "resolved": "https://registry.npmjs.org/react-merge-refs/-/react-merge-refs-1.1.0.tgz", - "integrity": "sha512-alTKsjEL0dKH/ru1Iyn7vliS2QRcBp9zZPGoWxUOvRGWPUYgjo+V01is7p04It6KhgrzhJGnIj9GgX8W4bZoCQ==" - }, "react-moment-proptypes": { "version": "1.7.0", "resolved": "https://registry.npmjs.org/react-moment-proptypes/-/react-moment-proptypes-1.7.0.tgz", diff --git a/packages/block-editor/package.json b/packages/block-editor/package.json index a322430668d7f..629f9db31c5e7 100644 --- a/packages/block-editor/package.json +++ b/packages/block-editor/package.json @@ -60,7 +60,6 @@ "lodash": "^4.17.19", "memize": "^1.1.0", "react-autosize-textarea": "^7.1.0", - "react-merge-refs": "^1.0.0", "react-spring": "^8.0.19", "redux-multi": "^0.1.12", "rememo": "^3.0.0", diff --git a/packages/block-editor/src/components/iframe/index.js b/packages/block-editor/src/components/iframe/index.js index 32366d1e3863e..cc0b350cc0f7b 100644 --- a/packages/block-editor/src/components/iframe/index.js +++ b/packages/block-editor/src/components/iframe/index.js @@ -1,7 +1,6 @@ /** * External dependencies */ -import mergeRefs from 'react-merge-refs'; import { compact, map } from 'lodash'; import tinycolor from 'tinycolor2'; @@ -16,6 +15,7 @@ import { forwardRef, } from '@wordpress/element'; import { __ } from '@wordpress/i18n'; +import { useMergeRefs } from '@wordpress/compose'; /** * Internal dependencies @@ -211,7 +211,7 @@ function Iframe( { contentRef, editorStyles, children, head, ...props }, ref ) { return (