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: rewrite base Slot to functional, unify rerenderable refs #67153

Merged
merged 3 commits into from
Nov 20, 2024
+78 −181
Merged

SlotFill: rewrite base Slot to functional, unify rerenderable refs #67153

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3c6f7f2
SlotFill: rewrite base Slot to functional, unify rerenderable refs
jsnajdr Nov 7, 2024
04db6fd
Add changelog entry
jsnajdr Nov 20, 2024
d3cafb4
Implement rerender with useReducer
jsnajdr Nov 20, 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
25 changes: 4 additions & 21 deletions packages/components/src/slot-fill/bubbles-virtually/fill.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { useObservableValue } from '@wordpress/compose';
import {
useContext,
useRef,
useState,
useEffect,
createPortal,
} from '@wordpress/element';
Expand All @@ -15,26 +14,9 @@ import {
*/
import SlotFillContext from './slot-fill-context';
import StyleProvider from '../../style-provider';
import useForceUpdate from '../use-force-update';
import type { FillComponentProps } from '../types';

function useForceUpdate() {
const [ , setState ] = useState( {} );
const mountedRef = useRef( true );

useEffect( () => {
mountedRef.current = true;
return () => {
mountedRef.current = false;
};
}, [] );

return () => {
if ( mountedRef.current ) {
setState( {} );
}
};
}
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 extracted to a separate module because the useForceUpdate hook is used by two components now. Also with React 18 we no longer need mountedRef because calling setState after unmount is no longer a problem.


export default function Fill( { name, children }: FillComponentProps ) {
const registry = useContext( SlotFillContext );
const slot = useObservableValue( registry.slots, name );
Expand All @@ -45,9 +27,10 @@ export default function Fill( { name, children }: FillComponentProps ) {
// We register fills so we can keep track of their existence.
// Some Slot implementations need to know if there're already fills
// registered so they can choose to render themselves or not.
registry.registerFill( name, ref );
const refValue = ref.current;
registry.registerFill( name, refValue );
return () => {
registry.unregisterFill( name, ref );
registry.unregisterFill( name, refValue );
Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of registering the ref object I'm registering directly the Rerenderable value. Which is also constant.

};
}, [ registry, name ] );

Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -70,7 +70,7 @@ function createSlotRegistry(): SlotFillBubblesVirtuallyContext {
const slotFills = fills.get( name );
if ( slotFills ) {
// Force update fills.
slotFills.forEach( ( fill ) => fill.current.rerender() );
slotFills.forEach( ( fill ) => fill.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.

This is a benefit of registering the Rerenderable value directly: avoids the .current indirection when calling.

}
};

Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/slot-fill/fill.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export default function Fill( { name, children }: FillComponentProps ) {
useLayoutEffect( () => {
ref.current.children = children;
if ( slot ) {
slot.forceUpdate();
slot.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.

forceUpdate was previously a method of the component class, now slot is the Rerenderable instance.

}
}, [ slot, children ] );

Expand Down
25 changes: 8 additions & 17 deletions packages/components/src/slot-fill/provider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
/**
* WordPress dependencies
*/
import type { Component } from '@wordpress/element';
import { useState } from '@wordpress/element';

/**
Expand All @@ -11,20 +10,17 @@ import SlotFillContext from './context';
import type {
FillComponentProps,
BaseSlotFillContext,
BaseSlotComponentProps,
SlotFillProviderProps,
SlotKey,
Rerenderable,
} from './types';

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

function registerSlot(
name: SlotKey,
slot: Component< BaseSlotComponentProps >
) {
function registerSlot( name: SlotKey, slot: Rerenderable ) {
const previousSlot = slots[ name ];
slots[ name ] = slot;
triggerListeners();
Expand All @@ -38,7 +34,7 @@ function createSlotRegistry(): BaseSlotFillContext {
// 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.forceUpdate();
previousSlot.rerender();
}
}

Expand All @@ -47,10 +43,7 @@ function createSlotRegistry(): BaseSlotFillContext {
forceUpdateSlot( name );
}

function unregisterSlot(
name: SlotKey,
instance: Component< BaseSlotComponentProps >
) {
function unregisterSlot( name: SlotKey, instance: Rerenderable ) {
// 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.
Expand All @@ -68,15 +61,13 @@ function createSlotRegistry(): BaseSlotFillContext {
forceUpdateSlot( name );
}

function getSlot(
name: SlotKey
): Component< BaseSlotComponentProps > | undefined {
function getSlot( name: SlotKey ): Rerenderable | undefined {
return slots[ name ];
}

function getFills(
name: SlotKey,
slotInstance: Component< BaseSlotComponentProps >
slotInstance: Rerenderable
): FillComponentProps[] {
// Fills should only be returned for the current instance of the slot
// in which they occupy.
Expand All @@ -90,7 +81,7 @@ function createSlotRegistry(): BaseSlotFillContext {
const slot = getSlot( name );

if ( slot ) {
slot.forceUpdate();
slot.rerender();
Copy link
Member

Choose a reason for hiding this comment

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

Should we also rename the internal forceUpdateSlot function to rerenderSlot or something for consistency?

}
}

Expand Down
133 changes: 48 additions & 85 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 @@ -7,8 +7,10 @@ import type { ReactElement, ReactNode, Key } from 'react';
* WordPress dependencies
*/
import {
useContext,
useEffect,
useRef,
Children,
Component,
cloneElement,
isEmptyElement,
} from '@wordpress/element';
Expand All @@ -17,7 +19,8 @@ import {
* Internal dependencies
*/
import SlotFillContext from './context';
import type { BaseSlotComponentProps, SlotComponentProps } from './types';
import type { SlotComponentProps } from './types';
import useForceUpdate from './use-force-update';

/**
* Whether the argument is a function.
Expand All @@ -29,90 +32,50 @@ function isFunction( maybeFunc: any ): maybeFunc is Function {
return typeof maybeFunc === 'function';
}

class SlotComponent extends Component< BaseSlotComponentProps > {
private isUnmounted: boolean;

constructor( props: BaseSlotComponentProps ) {
super( props );

this.isUnmounted = false;
}

componentDidMount() {
const { registerSlot } = this.props;
this.isUnmounted = false;
registerSlot( this.props.name, this );
}

componentWillUnmount() {
const { unregisterSlot } = this.props;
this.isUnmounted = true;
unregisterSlot( this.props.name, this );
}

componentDidUpdate( prevProps: BaseSlotComponentProps ) {
const { name, unregisterSlot, registerSlot } = this.props;

if ( prevProps.name !== name ) {
unregisterSlot( prevProps.name, this );
registerSlot( name, this );
}
}

forceUpdate() {
if ( this.isUnmounted ) {
return;
}
super.forceUpdate();
}

render() {
const { children, name, fillProps = {}, getFills } = this.props;
const fills: ReactNode[] = ( getFills( name, this ) ?? [] )
.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,
} );
function Slot( props: Omit< SlotComponentProps, 'bubblesVirtually' > ) {
const registry = useContext( SlotFillContext );
const rerender = useForceUpdate();
const ref = useRef( { rerender } );

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

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

const fills: ReactNode[] = ( registry.getFills( name, ref.current ) ?? [] )
.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,
} );
} )
.filter(
// In some cases fills are rendered only when some conditions apply.
// This ensures that we only use non-empty fills when rendering, i.e.,
// it allows us to render wrappers only when the fills are actually present.
( element ) => ! isEmptyElement( element )
);

return <>{ isFunction( children ) ? children( fills ) : fills }</>;
}
} );
} )
.filter(
// In some cases fills are rendered only when some conditions apply.
// This ensures that we only use non-empty fills when rendering, i.e.,
// it allows us to render wrappers only when the fills are actually present.
( element ) => ! isEmptyElement( element )
);

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

const Slot = ( props: Omit< SlotComponentProps, 'bubblesVirtually' > ) => (
<SlotFillContext.Consumer>
{ ( { registerSlot, unregisterSlot, getFills } ) => (
<SlotComponent
{ ...props }
registerSlot={ registerSlot }
unregisterSlot={ unregisterSlot }
getFills={ getFills }
/>
) }
</SlotFillContext.Consumer>
);

export default Slot;
69 changes: 14 additions & 55 deletions packages/components/src/slot-fill/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import type { Component, MutableRefObject, ReactNode, RefObject } from 'react';
import type { ReactNode, RefObject } from 'react';

/**
* WordPress dependencies
Expand Down Expand Up @@ -108,42 +108,17 @@ export type SlotFillProviderProps = {
passthrough?: boolean;
};

export type SlotFillBubblesVirtuallySlotRef = RefObject< HTMLElement >;
export type SlotFillBubblesVirtuallyFillRef = MutableRefObject< {
rerender: () => void;
} >;
export type SlotRef = RefObject< HTMLElement >;
export type Rerenderable = { rerender: () => void };

export type SlotFillBubblesVirtuallyContext = {
slots: ObservableMap<
SlotKey,
{
ref: SlotFillBubblesVirtuallySlotRef;
fillProps: FillProps;
}
>;
fills: ObservableMap< SlotKey, SlotFillBubblesVirtuallyFillRef[] >;
registerSlot: (
name: SlotKey,
ref: SlotFillBubblesVirtuallySlotRef,
fillProps: FillProps
) => void;
unregisterSlot: (
name: SlotKey,
ref: SlotFillBubblesVirtuallySlotRef
) => void;
updateSlot: (
name: SlotKey,
ref: SlotFillBubblesVirtuallySlotRef,
fillProps: FillProps
) => void;
registerFill: (
name: SlotKey,
ref: SlotFillBubblesVirtuallyFillRef
) => void;
unregisterFill: (
name: SlotKey,
ref: SlotFillBubblesVirtuallyFillRef
) => void;
slots: ObservableMap< SlotKey, { ref: SlotRef; fillProps: FillProps } >;
fills: ObservableMap< SlotKey, Rerenderable[] >;
registerSlot: ( name: SlotKey, ref: SlotRef, fillProps: FillProps ) => void;
unregisterSlot: ( name: SlotKey, ref: SlotRef ) => void;
updateSlot: ( name: SlotKey, ref: SlotRef, fillProps: FillProps ) => void;
registerFill: ( name: SlotKey, ref: Rerenderable ) => void;
unregisterFill: ( name: SlotKey, ref: Rerenderable ) => void;
Copy link
Member Author

Choose a reason for hiding this comment

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

With the SlotRef type name, code that uses it is much more tidy.

Comment on lines +120 to +121
Copy link
Member

Choose a reason for hiding this comment

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

Should we rename the ref to instance or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have ideas for a few additional cleanups, I'll use your suggestions there.

I'm considering merging the "base" and "bubbles-virtually" implementations into one. Both continue to be useful, as they differ fundamentally in how they pass React context down the tree. And the current SlotFillProvider renders two context providers nested, and the current Fill renders both types of fills at once. So, both implementations are kind of merged already, but in a clumsy way.


/**
* This helps the provider know if it's using the default context value or not.
Expand All @@ -152,30 +127,14 @@ export type SlotFillBubblesVirtuallyContext = {
};

export type BaseSlotFillContext = {
registerSlot: (
name: SlotKey,
slot: Component< BaseSlotComponentProps >
) => void;
unregisterSlot: (
name: SlotKey,
slot: Component< BaseSlotComponentProps >
) => void;
registerSlot: ( name: SlotKey, slot: Rerenderable ) => void;
unregisterSlot: ( name: SlotKey, slot: Rerenderable ) => void;
registerFill: ( name: SlotKey, instance: FillComponentProps ) => void;
unregisterFill: ( name: SlotKey, instance: FillComponentProps ) => void;
getSlot: (
name: SlotKey
) => Component< BaseSlotComponentProps > | undefined;
getSlot: ( name: SlotKey ) => Rerenderable | undefined;
getFills: (
name: SlotKey,
slotInstance: Component< BaseSlotComponentProps >
slotInstance: Rerenderable
) => FillComponentProps[];
subscribe: ( listener: () => void ) => () => void;
};

export type BaseSlotComponentProps = Pick<
BaseSlotFillContext,
'registerSlot' | 'unregisterSlot' | 'getFills'
> &
Omit< SlotComponentProps, 'bubblesVirtually' > & {
children?: ( fills: ReactNode ) => ReactNode;
};
Loading
Loading