Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Components: refactor SlotFill to pass exhaustive-deps #44403

Merged
merged 10 commits into from
Oct 20, 2022
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Contributor

@ciampo ciampo Oct 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This CHANGELOG entry got added under the wrong version, maybe we can move it as part of another exhaustive-deps related PR ?


## 21.2.0 (2022-10-05)

Expand Down
8 changes: 4 additions & 4 deletions packages/components/src/slot-fill/bubbles-virtually/fill.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
18 changes: 10 additions & 8 deletions packages/components/src/slot-fill/bubbles-virtually/slot.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,19 +19,21 @@ function Slot(
{ name, fillProps = {}, as: Component = 'div', ...props },
forwardedRef
) {
const registry = useContext( SlotFillContext );
const { registerSlot, unregisterSlot, ...registry } =
useContext( SlotFillContext );
const ref = useRef();

useLayoutEffect( () => {
registry.registerSlot( name, ref, fillProps );
registerSlot( name, ref, fillProps );
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 ] );
// 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 ] );
chad1008 marked this conversation as resolved.
Show resolved Hide resolved
// fillProps may be an update that interacts with the layout, so we
// useLayoutEffect.
useLayoutEffect( () => {
Expand Down
24 changes: 15 additions & 9 deletions packages/components/src/slot-fill/bubbles-virtually/use-slot.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 {
Expand Down
14 changes: 12 additions & 2 deletions packages/components/src/slot-fill/fill.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,22 @@ function FillComponent( { name, children, registerFill, unregisterFill } ) {
} );

useLayoutEffect( () => {
registerFill( name, ref.current );
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
}, [] );

useLayoutEffect( () => {
ref.current.children = children;
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( () => {
Expand All @@ -39,6 +46,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 ) {
Expand Down
3 changes: 3 additions & 0 deletions packages/components/src/slot-fill/use-slot.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down