From 52a7aa54b639a0fdbf1854053a85bd468ece98a1 Mon Sep 17 00:00:00 2001 From: chad1008 <13856531+chad1008@users.noreply.github.com> Date: Thu, 22 Sep 2022 16:14:02 -0400 Subject: [PATCH 01/10] Fill: destructuture slot methods for cleaner depenendencies --- .../components/src/slot-fill/bubbles-virtually/fill.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/components/src/slot-fill/bubbles-virtually/fill.js b/packages/components/src/slot-fill/bubbles-virtually/fill.js index bbbbc1f8add8eb..0353a3179bbfe9 100644 --- a/packages/components/src/slot-fill/bubbles-virtually/fill.js +++ b/packages/components/src/slot-fill/bubbles-virtually/fill.js @@ -28,18 +28,18 @@ function useForceUpdate() { } export default function Fill( { name, children } ) { - const slot = useSlot( name ); + const { registerFill, unregisterFill, ...slot } = useSlot( name ); const ref = useRef( { rerender: useForceUpdate() } ); useEffect( () => { // We register fills so we can keep track of their existance. // Some Slot implementations need to know if there're already fills // registered so they can choose to render themselves or not. - slot.registerFill( ref ); + registerFill( ref ); return () => { - slot.unregisterFill( ref ); + unregisterFill( ref ); }; - }, [ slot.registerFill, slot.unregisterFill ] ); + }, [ registerFill, unregisterFill ] ); if ( ! slot.ref || ! slot.ref.current ) { return null; From 62485e9caefec1f6d59d3e8696211de59a740fd9 Mon Sep 17 00:00:00 2001 From: chad1008 <13856531+chad1008@users.noreply.github.com> Date: Thu, 22 Sep 2022 17:04:05 -0400 Subject: [PATCH 02/10] SlotFill: update bubbles-virtually/slot.js to fix `layoutEffect` eslint warning --- .../src/slot-fill/bubbles-virtually/slot.js | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/packages/components/src/slot-fill/bubbles-virtually/slot.js b/packages/components/src/slot-fill/bubbles-virtually/slot.js index 1333c9cf2fd936..00277b7cc4dcdb 100644 --- a/packages/components/src/slot-fill/bubbles-virtually/slot.js +++ b/packages/components/src/slot-fill/bubbles-virtually/slot.js @@ -19,19 +19,22 @@ function Slot( { name, fillProps = {}, as: Component = 'div', ...props }, forwardedRef ) { - const registry = useContext( SlotFillContext ); + const { registerSlot, unregisterSlot, ...registry } = + useContext( SlotFillContext ); const ref = useRef(); + // We don't fillProps in the deps of the layout effect below because we don't want to + // unregister and register the slot whenever fillProps change. Doing so would + // cause the fill to be re-mounted, and we are only considering the initial value + // of fillProps. Instead, we store that initial value in a ref, but don't update it. + const fillPropsRef = useRef( fillProps ); + useLayoutEffect( () => { - registry.registerSlot( name, ref, fillProps ); + registerSlot( name, ref, fillPropsRef.current ); return () => { - registry.unregisterSlot( name, ref ); + unregisterSlot( name, ref ); }; - // We are not including fillProps in the deps because we don't want to - // unregister and register the slot whenever fillProps change, which would - // cause the fill to be re-mounted. We are only considering the initial value - // of fillProps. - }, [ registry.registerSlot, registry.unregisterSlot, name ] ); + }, [ registerSlot, unregisterSlot, name ] ); // fillProps may be an update that interacts with the layout, so we // useLayoutEffect. useLayoutEffect( () => { From 0330e090c39d62fc4b464c30893a3c64c70c4067 Mon Sep 17 00:00:00 2001 From: chad1008 <13856531+chad1008@users.noreply.github.com> Date: Thu, 22 Sep 2022 17:09:03 -0400 Subject: [PATCH 03/10] SlotFill: fix comment typo --- packages/components/src/slot-fill/bubbles-virtually/slot.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/components/src/slot-fill/bubbles-virtually/slot.js b/packages/components/src/slot-fill/bubbles-virtually/slot.js index 00277b7cc4dcdb..9b8aa1118368d9 100644 --- a/packages/components/src/slot-fill/bubbles-virtually/slot.js +++ b/packages/components/src/slot-fill/bubbles-virtually/slot.js @@ -23,7 +23,7 @@ function Slot( useContext( SlotFillContext ); const ref = useRef(); - // We don't fillProps in the deps of the layout effect below because we don't want to + // We want don't fillProps in the deps of the layout effect below because we don't want to // unregister and register the slot whenever fillProps change. Doing so would // cause the fill to be re-mounted, and we are only considering the initial value // of fillProps. Instead, we store that initial value in a ref, but don't update it. From 1666ab9807c4115acf600059f2510308335f5867 Mon Sep 17 00:00:00 2001 From: chad1008 <13856531+chad1008@users.noreply.github.com> Date: Mon, 10 Oct 2022 14:56:16 -0400 Subject: [PATCH 04/10] SlotFill: destructure variable in `useSlot` for cleaner dependencies --- .../slot-fill/bubbles-virtually/use-slot.js | 24 ++++++++++++------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/packages/components/src/slot-fill/bubbles-virtually/use-slot.js b/packages/components/src/slot-fill/bubbles-virtually/use-slot.js index 2d07a2c36ae079..8c5a0182ea0743 100644 --- a/packages/components/src/slot-fill/bubbles-virtually/use-slot.js +++ b/packages/components/src/slot-fill/bubbles-virtually/use-slot.js @@ -15,7 +15,13 @@ import { useCallback, useContext } from '@wordpress/element'; import SlotFillContext from './slot-fill-context'; export default function useSlot( name ) { - const registry = useContext( SlotFillContext ); + const { + updateSlot: registryUpdateSlot, + unregisterSlot: registryUnregisterSlot, + registerFill: registryRegisterFill, + unregisterFill: registryUnregisterFill, + ...registry + } = useContext( SlotFillContext ); const slots = useSnapshot( registry.slots, { sync: true } ); // The important bit here is that this call ensures // the hook only causes a re-render if the slot @@ -24,30 +30,30 @@ export default function useSlot( name ) { const updateSlot = useCallback( ( fillProps ) => { - registry.updateSlot( name, fillProps ); + registryUpdateSlot( name, fillProps ); }, - [ name, registry.updateSlot ] + [ name, registryUpdateSlot ] ); const unregisterSlot = useCallback( ( slotRef ) => { - registry.unregisterSlot( name, slotRef ); + registryUnregisterSlot( name, slotRef ); }, - [ name, registry.unregisterSlot ] + [ name, registryUnregisterSlot ] ); const registerFill = useCallback( ( fillRef ) => { - registry.registerFill( name, fillRef ); + registryRegisterFill( name, fillRef ); }, - [ name, registry.registerFill ] + [ name, registryRegisterFill ] ); const unregisterFill = useCallback( ( fillRef ) => { - registry.unregisterFill( name, fillRef ); + registryUnregisterFill( name, fillRef ); }, - [ name, registry.unregisterFill ] + [ name, registryUnregisterFill ] ); return { From b1dd769bd43638a898536d0a82cea550ef3b5cd3 Mon Sep 17 00:00:00 2001 From: chad1008 <13856531+chad1008@users.noreply.github.com> Date: Wed, 12 Oct 2022 16:20:03 -0400 Subject: [PATCH 05/10] add ignore statements to slot-fill/fill.js until they can be more fully refactored --- packages/components/src/slot-fill/fill.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/packages/components/src/slot-fill/fill.js b/packages/components/src/slot-fill/fill.js index 77188ab8a16144..eda7fe16d4172e 100644 --- a/packages/components/src/slot-fill/fill.js +++ b/packages/components/src/slot-fill/fill.js @@ -21,7 +21,13 @@ function FillComponent( { name, children, registerFill, unregisterFill } ) { useLayoutEffect( () => { registerFill( name, ref.current ); + // Ignore reason: The ref.current value will likely change before the cleanup is run. + // This may be intentional, but could be impacted by future refactors of the other useLayoutEffects in this component. + // eslint-disable-next-line react-hooks/exhaustive-deps return () => unregisterFill( name, ref.current ); + // Ignore reason: the useLayoutEffects here are written to fire at specific times, and introducing new dependencies could cause unexpected changes in behavior. + // We'll leave them as-is until a more detailed investigation/refactor can be performed. + // eslint-disable-next-line react-hooks/exhaustive-deps }, [] ); useLayoutEffect( () => { @@ -29,6 +35,9 @@ function FillComponent( { name, children, registerFill, unregisterFill } ) { if ( slot ) { slot.forceUpdate(); } + // Ignore reason: the useLayoutEffects here are written to fire at specific times, and introducing new dependencies could cause unexpected changes in behavior. + // We'll leave them as-is until a more detailed investigation/refactor can be performed. + // eslint-disable-next-line react-hooks/exhaustive-deps }, [ children ] ); useLayoutEffect( () => { @@ -39,6 +48,9 @@ function FillComponent( { name, children, registerFill, unregisterFill } ) { unregisterFill( ref.current.name, ref.current ); ref.current.name = name; registerFill( name, ref.current ); + // Ignore reason: the useLayoutEffects here are written to fire at specific times, and introducing new dependencies could cause unexpected changes in behavior. + // We'll leave them as-is until a more detailed investigation/refactor can be performed. + // eslint-disable-next-line react-hooks/exhaustive-deps }, [ name ] ); if ( ! slot || ! slot.node ) { From 6203e2ae859f4e8f1ca6a1c646cbb8da2cb8c304 Mon Sep 17 00:00:00 2001 From: chad1008 <13856531+chad1008@users.noreply.github.com> Date: Wed, 12 Oct 2022 16:22:07 -0400 Subject: [PATCH 06/10] add ignore statements to slot-fill/use-slot.js until they can be more fully refactored --- packages/components/src/slot-fill/use-slot.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/components/src/slot-fill/use-slot.js b/packages/components/src/slot-fill/use-slot.js index 96016e267a681e..99e7138e120259 100644 --- a/packages/components/src/slot-fill/use-slot.js +++ b/packages/components/src/slot-fill/use-slot.js @@ -26,6 +26,9 @@ const useSlot = ( name ) => { } ); return unsubscribe; + // Ignore reason: Modifying this dep array could introduce unexpected changes in behavior, + // so we'll leave it as=is until the hook can be properly refactored for exhaustive-deps. + // eslint-disable-next-line react-hooks/exhaustive-deps }, [ name ] ); return slot; From 2ff281bccaf83aa64e744086adb618c3db97bb8d Mon Sep 17 00:00:00 2001 From: chad1008 <13856531+chad1008@users.noreply.github.com> Date: Thu, 13 Oct 2022 12:17:08 -0400 Subject: [PATCH 07/10] Components: update CHANGELOG --- packages/components/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index 18a230c301d46d..2bff5497d6afcf 100644 --- a/packages/components/CHANGELOG.md +++ b/packages/components/CHANGELOG.md @@ -23,6 +23,7 @@ - `withFocusReturn`: Refactor tests to `@testing-library/react` ([#45012](https://github.com/WordPress/gutenberg/pull/45012)). - `ToolsPanel`: updated to satisfy `react/exhaustive-deps` eslint rule ([#45028](https://github.com/WordPress/gutenberg/pull/45028)) - `Tooltip`: updated to ignore `react/exhaustive-deps` eslint rule ([#45043](https://github.com/WordPress/gutenberg/pull/45043)) +- `SlotFill`: updated to satisfy `react/exhaustive-deps` eslint rule ([#44403](https://github.com/WordPress/gutenberg/pull/44403)) ## 21.2.0 (2022-10-05) From 503c950b6ab2129e63fd0cbfd70252a9229442ec Mon Sep 17 00:00:00 2001 From: chad1008 <13856531+chad1008@users.noreply.github.com> Date: Mon, 17 Oct 2022 12:35:34 -0400 Subject: [PATCH 08/10] remove fillProps ref in favor of eslint ignore for now --- .../components/src/slot-fill/bubbles-virtually/slot.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/components/src/slot-fill/bubbles-virtually/slot.js b/packages/components/src/slot-fill/bubbles-virtually/slot.js index 9b8aa1118368d9..6ae1c9731917e0 100644 --- a/packages/components/src/slot-fill/bubbles-virtually/slot.js +++ b/packages/components/src/slot-fill/bubbles-virtually/slot.js @@ -25,15 +25,15 @@ function Slot( // We want don't fillProps in the deps of the layout effect below because we don't want to // unregister and register the slot whenever fillProps change. Doing so would - // cause the fill to be re-mounted, and we are only considering the initial value - // of fillProps. Instead, we store that initial value in a ref, but don't update it. - const fillPropsRef = useRef( fillProps ); - + // cause the fill to be re-mounted. useLayoutEffect( () => { - registerSlot( name, ref, fillPropsRef.current ); + registerSlot( name, ref, fillProps ); return () => { unregisterSlot( name, ref ); }; + // Ignore reason: see above. + // Also, please see: https://github.com/WordPress/gutenberg/pull/44403#discussion_r994415973 + // eslint-disable-next-line react-hooks/exhaustive-deps }, [ registerSlot, unregisterSlot, name ] ); // fillProps may be an update that interacts with the layout, so we // useLayoutEffect. From 76910a088210cd324b9b2939133cb31b6cf64a0d Mon Sep 17 00:00:00 2001 From: chad1008 <13856531+chad1008@users.noreply.github.com> Date: Wed, 19 Oct 2022 15:04:36 -0400 Subject: [PATCH 09/10] consolidate bubbles-virtually/slot.js comment --- .../components/src/slot-fill/bubbles-virtually/slot.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/components/src/slot-fill/bubbles-virtually/slot.js b/packages/components/src/slot-fill/bubbles-virtually/slot.js index 6ae1c9731917e0..ef7ad56cc68bac 100644 --- a/packages/components/src/slot-fill/bubbles-virtually/slot.js +++ b/packages/components/src/slot-fill/bubbles-virtually/slot.js @@ -23,16 +23,15 @@ function Slot( useContext( SlotFillContext ); const ref = useRef(); - // We want don't fillProps in the deps of the layout effect below because we don't want to - // unregister and register the slot whenever fillProps change. Doing so would - // cause the fill to be re-mounted. useLayoutEffect( () => { registerSlot( name, ref, fillProps ); return () => { unregisterSlot( name, ref ); }; - // Ignore reason: see above. - // Also, please see: https://github.com/WordPress/gutenberg/pull/44403#discussion_r994415973 + // Ignore reason: We don't want to unregister and register the slot whenever + // `fillProps` change, which would cause the fill to be re-mounted. Instead, + // we can just update the slot (see hook below). + // For more context, see https://github.com/WordPress/gutenberg/pull/44403#discussion_r994415973 // eslint-disable-next-line react-hooks/exhaustive-deps }, [ registerSlot, unregisterSlot, name ] ); // fillProps may be an update that interacts with the layout, so we From fbb48f2e9d8e4cd45dadec2b767ac03ccf14feff Mon Sep 17 00:00:00 2001 From: chad1008 <13856531+chad1008@users.noreply.github.com> Date: Wed, 19 Oct 2022 15:49:18 -0400 Subject: [PATCH 10/10] use ref to store FillComponent name and children props for useLayoutEffect cleanup --- packages/components/src/slot-fill/fill.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/components/src/slot-fill/fill.js b/packages/components/src/slot-fill/fill.js index eda7fe16d4172e..745439a275dd1e 100644 --- a/packages/components/src/slot-fill/fill.js +++ b/packages/components/src/slot-fill/fill.js @@ -20,11 +20,9 @@ function FillComponent( { name, children, registerFill, unregisterFill } ) { } ); useLayoutEffect( () => { - registerFill( name, ref.current ); - // Ignore reason: The ref.current value will likely change before the cleanup is run. - // This may be intentional, but could be impacted by future refactors of the other useLayoutEffects in this component. - // eslint-disable-next-line react-hooks/exhaustive-deps - return () => unregisterFill( name, ref.current ); + const refValue = ref.current; + registerFill( name, refValue ); + return () => unregisterFill( name, refValue ); // Ignore reason: the useLayoutEffects here are written to fire at specific times, and introducing new dependencies could cause unexpected changes in behavior. // We'll leave them as-is until a more detailed investigation/refactor can be performed. // eslint-disable-next-line react-hooks/exhaustive-deps