Skip to content

SlotFill: fix a bug with storing stale fillProps #67000

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.

Sign up for GitHub

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

Merged
jsnajdr merged 4 commits into from
Nov 15, 2024
Merged

SlotFill: fix a bug with storing stale fillProps #67000

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
23d09b2
SlotFill: updateSlot should check the ref
jsnajdr Nov 7, 2024
7bd8715
Slot: store fillProps to a ref to fix effect deps
jsnajdr Nov 7, 2024
4c78204
SlotFillProvider: tidy up API code
jsnajdr Nov 7, 2024
7fa987d
Add changelog entry
jsnajdr Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

- `Popover`: Fix missing label of the headerTitle Close button ([#66813](https://github.com/WordPress/gutenberg/pull/66813)).
- `ToggleGroupControl`: Fix active background for `0` value ([#66855](https://github.com/WordPress/gutenberg/pull/66855)).
- `SlotFill`: Fix a bug where a stale value of `fillProps` could be used ([#67000](https://github.com/WordPress/gutenberg/pull/67000)).

### Enhancements

Expand Down
40 changes: 25 additions & 15 deletions packages/components/src/slot-fill/bubbles-virtually/slot-fill-provider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,22 +34,34 @@ function createSlotRegistry(): SlotFillBubblesVirtuallyContext {

const unregisterSlot: SlotFillBubblesVirtuallyContext[ 'unregisterSlot' ] =
( name, ref ) => {
const slot = slots.get( name );
if ( ! slot ) {
return;
}

// Make sure we're not unregistering a slot registered by another element
// See https://github.com/WordPress/gutenberg/pull/19242#issuecomment-590295412
if ( slots.get( name )?.ref === ref ) {
slots.delete( name );
if ( slot.ref !== ref ) {
return;
}

slots.delete( name );
Copy link
Member Author

@jsnajdr jsnajdr Nov 15, 2024

Choose a reason for hiding this comment

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

Here I'm just tidying up the code, no behavior change. unregisterSlot and updateSlot now do the same ref code and the code looks also the same.

};

const updateSlot: SlotFillBubblesVirtuallyContext[ 'updateSlot' ] = (
name,
ref,
fillProps
) => {
const slot = slots.get( name );
if ( ! slot ) {
return;
}

if ( slot.ref !== ref ) {
return;
}
Copy link
Member Author

@jsnajdr jsnajdr Nov 15, 2024

Choose a reason for hiding this comment

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

This is second critical line of the patch, adding a check if ref matches.


if ( isShallowEqual( slot.fillProps, fillProps ) ) {
return;
}
Expand All @@ -69,20 +81,18 @@ function createSlotRegistry(): SlotFillBubblesVirtuallyContext {
fills.set( name, [ ...( fills.get( name ) || [] ), ref ] );
};

const unregisterFill: SlotFillBubblesVirtuallyContext[ 'registerFill' ] = (
name,
ref
) => {
const fillsForName = fills.get( name );
if ( ! fillsForName ) {
return;
}
const unregisterFill: SlotFillBubblesVirtuallyContext[ 'unregisterFill' ] =
Copy link
Member Author

@jsnajdr jsnajdr Nov 15, 2024

Choose a reason for hiding this comment

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

Here I'm fixing a typo in the type: changing registerFill to unregisterFill. Both functions have the same signature, so the bug didn't have any observable consequences, but worth fixing anyway.

( name, ref ) => {
const fillsForName = fills.get( name );
if ( ! fillsForName ) {
return;
}

fills.set(
name,
fillsForName.filter( ( fillRef ) => fillRef !== ref )
);
};
fills.set(
name,
fillsForName.filter( ( fillRef ) => fillRef !== ref )
);
};

return {
slots,
Expand Down
22 changes: 13 additions & 9 deletions packages/components/src/slot-fill/bubbles-virtually/slot.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,29 +35,33 @@ function Slot(
as,
// `children` is not allowed. However, if it is passed,
// it will be displayed as is, so remove `children`.
// @ts-ignore
children,
...restProps
} = props;

const { registerSlot, unregisterSlot, ...registry } =
useContext( SlotFillContext );

const ref = useRef< HTMLElement >( null );

// 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
const fillPropsRef = useRef( fillProps );
useLayoutEffect( () => {
fillPropsRef.current = fillProps;
}, [ fillProps ] );
Copy link
Contributor

@youknowriad youknowriad Nov 14, 2024

Choose a reason for hiding this comment

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

In general this is an indication that something should have been defined using the new useEvent maybe.

Copy link
Member Author

@jsnajdr jsnajdr Nov 15, 2024

Choose a reason for hiding this comment

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

Unfortunately useEvent is specialized to support only callbacks. Here we have a value that is a regular object.

Copy link
Contributor

@youknowriad youknowriad Nov 15, 2024

Choose a reason for hiding this comment

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

What I was thinking is that we could do something like

const registerSlotEffect = useEvent(() => {
   registerSlot( name, ref, fillProps );
} );

useEffect(() => {
   registerSlotEffect();
}, [ name ] );

Copy link
Member Author

@jsnajdr jsnajdr Nov 15, 2024

Choose a reason for hiding this comment

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

Yes, if we really wanted to use useEvent, it's possible, but it doesn't really improve the code IMO. Also the fillPropsRef.current value is used in two effect hooks, which makes useEvent even less attractive.


useLayoutEffect( () => {
registerSlot( name, ref, fillProps );
registerSlot( name, ref, fillPropsRef.current );
return () => {
unregisterSlot( name, ref );
};
// 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
}, [ registerSlot, unregisterSlot, name ] );
// fillProps may be an update that interacts with the layout, so we
// useLayoutEffect.

useLayoutEffect( () => {
registry.updateSlot( name, fillProps );
registry.updateSlot( name, ref, fillPropsRef.current );
Copy link
Member Author

@jsnajdr jsnajdr Nov 15, 2024

Choose a reason for hiding this comment

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

This is a critical line in the patch, adding a ref param to the updateSlot call.

} );

return (
Expand Down
6 changes: 4 additions & 2 deletions packages/components/src/slot-fill/bubbles-virtually/use-slot.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,10 @@ export default function useSlot( name: SlotKey ) {

const api = useMemo(
() => ( {
updateSlot: ( fillProps: FillProps ) =>
registry.updateSlot( name, fillProps ),
updateSlot: (
Copy link
Contributor

@youknowriad youknowriad Nov 14, 2024

Choose a reason for hiding this comment

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

Where is this one used. or is it not used at all? I see the function arguments modified but I don't see where the calling of the function is updated.

Copy link
Member Author

@jsnajdr jsnajdr Nov 15, 2024

Choose a reason for hiding this comment

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

The updateSlot method is used in the Slot component. Called on every render, in a effect without dependencies, to make sure that the latest fillProps is always synced promptly to the registry.

The bug being fixed is triggered when, for any reason, the Slot is rerendered one more time before unmounting. Then updateSlot would store a stale fillProps value at a time when a new Slot is already being mounted. The unregisterSlot method already has exactly the same ref check.

Copy link
Contributor

@youknowriad youknowriad Nov 15, 2024

Choose a reason for hiding this comment

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

But this one updateSlot that is returned from useSlot, is not used right?

Copy link
Member Author

@jsnajdr jsnajdr Nov 15, 2024

Choose a reason for hiding this comment

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

Oh yes, this is a good observation. The updateSlot and unregisterSlot functions returned from useSlot are not really used. And I think I could get rid of the entire api object and return only the slot. I'll do that in a followup PR.

ref: SlotFillBubblesVirtuallySlotRef,
fillProps: FillProps
) => registry.updateSlot( name, ref, fillProps ),
unregisterSlot: ( ref: SlotFillBubblesVirtuallySlotRef ) =>
registry.unregisterSlot( name, ref ),
registerFill: ( ref: SlotFillBubblesVirtuallyFillRef ) =>
Expand Down
6 changes: 5 additions & 1 deletion packages/components/src/slot-fill/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,11 @@ export type SlotFillBubblesVirtuallyContext = {
name: SlotKey,
ref: SlotFillBubblesVirtuallySlotRef
) => void;
updateSlot: ( name: SlotKey, fillProps: FillProps ) => void;
updateSlot: (
name: SlotKey,
ref: SlotFillBubblesVirtuallySlotRef,
fillProps: FillProps
) => void;
registerFill: (
name: SlotKey,
ref: SlotFillBubblesVirtuallyFillRef
Expand Down
Loading