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.

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

SlotFill: use observableMap everywhere, remove manual rerendering #67400

Merged
merged 2 commits into from
Dec 13, 2024
Merged

SlotFill: use observableMap everywhere, remove manual rerendering #67400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6bc74bc
SlotFill: use observableMap in base version
jsnajdr Nov 28, 2024
441ca28
Add changelog entry
jsnajdr Dec 3, 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
4 changes: 4 additions & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@

- Add new `Badge` component ([#66555](https://github.com/WordPress/gutenberg/pull/66555)).

### Internal

- `SlotFill`: rewrite the non-portal version to use `observableMap` ([#67400](https://github.com/WordPress/gutenberg/pull/67400)).

## 29.0.0 (2024-12-11)

### Breaking Changes
Expand Down
8 changes: 5 additions & 3 deletions packages/components/src/slot-fill/context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,22 @@
/**
* WordPress dependencies
*/
import { observableMap } from '@wordpress/compose';
import { createContext } from '@wordpress/element';

/**
* Internal dependencies
*/
import type { BaseSlotFillContext } from './types';

const initialValue: BaseSlotFillContext = {
slots: observableMap(),
fills: observableMap(),
registerSlot: () => {},
unregisterSlot: () => {},
registerFill: () => {},
unregisterFill: () => {},
getSlot: () => undefined,
getFills: () => [],
subscribe: () => () => {},
Copy link
Member Author

Choose a reason for hiding this comment

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

getSlot and getFills are replaced by simple map lookups: slots.get( name ). The subscribe method is also a method of the observable map now.

updateFill: () => {},
};
export const SlotFillContext = createContext( initialValue );

Expand Down
25 changes: 10 additions & 15 deletions packages/components/src/slot-fill/fill.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,31 +7,26 @@ import { useContext, useLayoutEffect, useRef } from '@wordpress/element';
* Internal dependencies
*/
import SlotFillContext from './context';
import useSlot from './use-slot';
import type { FillComponentProps } from './types';

export default function Fill( { name, children }: FillComponentProps ) {
const registry = useContext( SlotFillContext );
const slot = useSlot( name );
Copy link
Member Author

Choose a reason for hiding this comment

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

The Fill no longer needs the slot value because it no longer calls slot.rerender(). Instead, it updates the value in the fills map (updateFill call) and the relevant slot rerenders automatically because it listens for fills changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

The fact that there were two separate useSlot hooks was definitely confusing. Glad to see that the "non-bubblesVirtually" version is going away

Copy link
Member Author

Choose a reason for hiding this comment

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

The removed useSlot hook was essentially a useSyncExternalStore call. Now when the slot registry uses observableMap, it can be fully replaced by useObservableMap, which is also a thin wrapper around useSyncExternalStore.

const instanceRef = useRef( {} );
const childrenRef = useRef( children );

const ref = useRef( {
name,
children,
} );
Copy link
Member Author

Choose a reason for hiding this comment

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

Here we used to store name in the fills map but that was redundant. name is a key in the map. It doesn't need to be stored also in the value.

useLayoutEffect( () => {
childrenRef.current = children;
}, [ children ] );

useLayoutEffect( () => {
const refValue = ref.current;
refValue.name = name;
registry.registerFill( name, refValue );
return () => registry.unregisterFill( name, refValue );
const instance = instanceRef.current;
registry.registerFill( name, instance, childrenRef.current );
return () => registry.unregisterFill( name, instance );
}, [ registry, name ] );

useLayoutEffect( () => {
ref.current.children = children;
if ( slot ) {
slot.rerender();
}
}, [ slot, children ] );
Copy link
Member Author

Choose a reason for hiding this comment

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

This code used to store the new value of children into a ref.current. But that's a "silent" mutation that doesn't trigger any listeners. That's why the code had to do a manual slot.rerender() call. The new code updates the children using the observableMap.set method (hidden inside the updateFill method). Because the slot listens for fills updates via useObservableValue( fills, name ), it gets notified and rerenders automatically.

registry.updateFill( name, instanceRef.current, childrenRef.current );
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it still necessary to wrap the updateFill call inside a useLayoutEffect, especially given that the hook doesn't have a dependency list anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the purpose of the updateFill is to store the latest children into the fills map in the registry on every rerender of the Fill. It might have been rerendered with new children.

It's an effect because it's a side effect. We want to "commit" the updateFill change only when the output render is really committed to the DOM. That's why it can't be called directly during render.

} );

return null;
}
127 changes: 63 additions & 64 deletions packages/components/src/slot-fill/provider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,103 +8,102 @@ import { useState } from '@wordpress/element';
*/
import SlotFillContext from './context';
import type {
FillComponentProps,
FillInstance,
FillChildren,
BaseSlotInstance,
BaseSlotFillContext,
SlotFillProviderProps,
SlotKey,
Rerenderable,
} from './types';
import { observableMap } from '@wordpress/compose';

function createSlotRegistry(): BaseSlotFillContext {
const slots: Record< SlotKey, Rerenderable > = {};
const fills: Record< SlotKey, FillComponentProps[] > = {};
let listeners: Array< () => void > = [];

function registerSlot( name: SlotKey, slot: Rerenderable ) {
const previousSlot = slots[ name ];
slots[ name ] = slot;
triggerListeners();

// Sometimes the fills are registered after the initial render of slot
// But before the registerSlot call, we need to rerender the slot.
forceUpdateSlot( name );

// If a new instance of a slot is being mounted while another with the
// same name exists, force its update _after_ the new slot has been
// assigned into the instance, such that its own rendering of children
// will be empty (the new Slot will subsume all fills for this name).
if ( previousSlot ) {
previousSlot.rerender();
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

All this code collapses into a simple slots.set call:

  • triggerListeners is built-in in slots.set.
  • the forceUpdateSlot logic is built-in into useSyncExternalStore. It handles the situation when the store is updated after the consumer component is rendered and before it subscribes to updates in an effect.
  • previousSlot.rerender() happens automatically because the old slot calls the currentSlot = useObservableValue( slots, name ) hook and gets notified when a new slot is registered under the same name.

Copy link
Contributor

Choose a reason for hiding this comment

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

I love how elegant this is ! I wonder if we should document it somewhere, though, since otherwise there would be a lot of implicit knowledge required to fully understand the component.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now when we use an observable store for slots and observe it with useSyncExternalStore, there isn't really much to document. You just use a native hook and it does all the work for you.

The previous implementation was convoluted because:

  • the Slot component was not subscribed to changes in the slots registry. Only Fill was. That's why functions in the provider had to manually call slot.rerender() on every relevant event. That's not replaced with useSyncExternalStore/useObservableMap with automatically rerenders the Slot component whenever the entry in slots changes.
  • it manually handled the situation when store was updated between a render (reading a value from store) and the effect that subscribed to updates. Then the update was missed. Before useSyncExternalStore, you needed to be aware of such gotchas. For example, the Redux connect implementation had to handle this, too. But today we have a native hook that does it all for us. It handles it in this code:

https://github.com/facebook/react/blob/main/packages/react-reconciler/src/ReactFiberHooks.js#L1900-L1907


function registerFill( name: SlotKey, instance: FillComponentProps ) {
fills[ name ] = [ ...( fills[ name ] || [] ), instance ];
forceUpdateSlot( name );
const slots = observableMap< SlotKey, BaseSlotInstance >();
const fills = observableMap<
SlotKey,
{ instance: FillInstance; children: FillChildren }[]
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why here (and in types.ts) we stopped using FillComponentProps ?

Copy link
Member Author

Choose a reason for hiding this comment

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

FillComponentProps also includes the name. But we don't need name in the value in the map, because name is already the key. Also FillComponentsProps don't include the instance field.

I should give some nice name to the { instance, children } type. It's similar to FillComponentProps, but not identical.

>();

function registerSlot( name: SlotKey, instance: BaseSlotInstance ) {
slots.set( name, instance );
}

function unregisterSlot( name: SlotKey, instance: Rerenderable ) {
function unregisterSlot( name: SlotKey, instance: BaseSlotInstance ) {
// If a previous instance of a Slot by this name unmounts, do nothing,
// as the slot and its fills should only be removed for the current
// known instance.
if ( slots[ name ] !== instance ) {
if ( slots.get( name ) !== instance ) {
return;
}

delete slots[ name ];
triggerListeners();
slots.delete( name );
}

function unregisterFill( name: SlotKey, instance: FillComponentProps ) {
fills[ name ] =
fills[ name ]?.filter( ( fill ) => fill !== instance ) ?? [];
forceUpdateSlot( name );
function registerFill(
name: SlotKey,
instance: FillInstance,
children: FillChildren
) {
fills.set( name, [
...( fills.get( name ) || [] ),
{ instance, children },
] );
}

function getSlot( name: SlotKey ): Rerenderable | undefined {
return slots[ name ];
function unregisterFill( name: SlotKey, instance: FillInstance ) {
const fillsForName = fills.get( name );
if ( ! fillsForName ) {
return;
}

fills.set(
name,
fillsForName.filter( ( fill ) => fill.instance !== instance )
);
}

function getFills(
function updateFill(
name: SlotKey,
slotInstance: Rerenderable
): FillComponentProps[] {
// Fills should only be returned for the current instance of the slot
// in which they occupy.
if ( slots[ name ] !== slotInstance ) {
return [];
Copy link
Member Author

Choose a reason for hiding this comment

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

This logic where the fills are [] for a slot instance that has been overwritten in the store by a new instance, it has moved into the Slot component.

instance: FillInstance,
children: FillChildren
) {
const fillsForName = fills.get( name );
if ( ! fillsForName ) {
return;
}
return fills[ name ];
}

function forceUpdateSlot( name: SlotKey ) {
const slot = getSlot( name );

if ( slot ) {
slot.rerender();
const fillForInstance = fillsForName.find(
( f ) => f.instance === instance
);
if ( ! fillForInstance ) {
return;
}
}

function triggerListeners() {
listeners.forEach( ( listener ) => listener() );
}

function subscribe( listener: () => void ) {
listeners.push( listener );
if ( fillForInstance.children === children ) {
return;
}

return () => {
listeners = listeners.filter( ( l ) => l !== listener );
};
fills.set(
name,
fillsForName.map( ( f ) => {
if ( f.instance === instance ) {
// Replace with new record with updated `children`.
return { instance, children };
}

return f;
} )
);
}

return {
slots,
fills,
registerSlot,
unregisterSlot,
registerFill,
unregisterFill,
getSlot,
getFills,
subscribe,
updateFill,
};
}

Expand Down
63 changes: 38 additions & 25 deletions packages/components/src/slot-fill/slot.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ import type { ReactElement, ReactNode, Key } from 'react';
/**
* WordPress dependencies
*/
import { useObservableValue } from '@wordpress/compose';
import {
useContext,
useEffect,
useReducer,
useRef,
Children,
cloneElement,
Expand All @@ -32,41 +32,48 @@ function isFunction( maybeFunc: any ): maybeFunc is Function {
return typeof maybeFunc === 'function';
}

function addKeysToChildren( children: ReactNode ) {
return Children.map( children, ( child, childIndex ) => {
if ( ! child || typeof child === 'string' ) {
return child;
}
let childKey: Key = childIndex;
if ( typeof child === 'object' && 'key' in child && child?.key ) {
childKey = child.key;
}

return cloneElement( child as ReactElement, {
key: childKey,
} );
} );
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a simple refactor where the addKeysToChildren function has been extracted from the big Slot component. Without any change.


function Slot( props: Omit< SlotComponentProps, 'bubblesVirtually' > ) {
const registry = useContext( SlotFillContext );
const [ , rerender ] = useReducer( () => [], [] );
const ref = useRef( { rerender } );
const instanceRef = useRef( {} );

const { name, children, fillProps = {} } = props;

useEffect( () => {
const refValue = ref.current;
registry.registerSlot( name, refValue );
return () => registry.unregisterSlot( name, refValue );
const instance = instanceRef.current;
registry.registerSlot( name, instance );
return () => registry.unregisterSlot( name, instance );
}, [ registry, name ] );

const fills: ReactNode[] = ( registry.getFills( name, ref.current ) ?? [] )
let fills = useObservableValue( registry.fills, name ) ?? [];
const currentSlot = useObservableValue( registry.slots, name );

// Fills should only be rendered in the currently registered instance of the slot.
if ( currentSlot !== instanceRef.current ) {
fills = [];
}

const renderedFills = fills
.map( ( fill ) => {
const fillChildren = isFunction( fill.children )
? fill.children( fillProps )
: fill.children;
return Children.map( fillChildren, ( child, childIndex ) => {
if ( ! child || typeof child === 'string' ) {
return child;
}
let childKey: Key = childIndex;
if (
typeof child === 'object' &&
'key' in child &&
child?.key
) {
childKey = child.key;
}

return cloneElement( child as ReactElement, {
key: childKey,
} );
} );
return addKeysToChildren( fillChildren );
} )
.filter(
// In some cases fills are rendered only when some conditions apply.
Expand All @@ -75,7 +82,13 @@ function Slot( props: Omit< SlotComponentProps, 'bubblesVirtually' > ) {
( element ) => ! isEmptyElement( element )
);

return <>{ isFunction( children ) ? children( fills ) : fills }</>;
return (
<>
{ isFunction( children )
? children( renderedFills )
: renderedFills }
</>
);
}

export default Slot;
Loading
Loading