From 983d0a370d03fe31a9d748c4cac81cb3a5d0f362 Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Thu, 9 May 2019 17:24:02 +0100 Subject: [PATCH 1/2] Refactor fill component to avoid relying on componentWillUpdate and use React Hooks instead --- packages/components/src/slot-fill/fill.js | 87 ++++++++++------------- packages/components/src/slot-fill/slot.js | 2 +- 2 files changed, 40 insertions(+), 49 deletions(-) diff --git a/packages/components/src/slot-fill/fill.js b/packages/components/src/slot-fill/fill.js index 7c2b3fb0ad5a2..f4ebb6b93dbae 100644 --- a/packages/components/src/slot-fill/fill.js +++ b/packages/components/src/slot-fill/fill.js @@ -6,7 +6,7 @@ import { isFunction } from 'lodash'; /** * WordPress dependencies */ -import { Component, createPortal } from '@wordpress/element'; +import { createPortal, useEffect, useRef, useState } from '@wordpress/element'; /** * Internal dependencies @@ -15,64 +15,55 @@ import { Consumer } from './context'; let occurrences = 0; -class FillComponent extends Component { - constructor() { - super( ...arguments ); - this.occurrence = ++occurrences; - } - - componentDidMount() { - const { registerFill } = this.props; - - registerFill( this.props.name, this ); - } - - componentWillUpdate() { - if ( ! this.occurrence ) { - this.occurrence = ++occurrences; +function FillComponent( { name, getSlot, children, registerFill, unregisterFill } ) { + // Random state used to rerender the component if needed, ideally we don't need this + const [ , updateRerenderState ] = useState( {} ); + const rerender = () => updateRerenderState( {} ); + + const ref = useRef( { + name, + children, + } ); + + useEffect( () => { + ref.current.occurence = ++occurrences; + ref.current.resetOccurrence = () => { + ref.current.occurence = null; + }; + ref.current.forceUpdate = rerender; + registerFill( name, ref.current ); + return () => unregisterFill( name, ref.current ); + }, [] ); + + useEffect( () => { + if ( ! ref.current.occurence ) { + ref.current.occurence = ++occurrences; } - const { getSlot } = this.props; - const slot = getSlot( this.props.name ); + ref.current.children = children; + const slot = getSlot( name ); if ( slot && ! slot.props.bubblesVirtually ) { slot.forceUpdate(); } - } + }, [ children ] ); - componentWillUnmount() { - const { unregisterFill } = this.props; - - unregisterFill( this.props.name, this ); - } + useEffect( () => { + unregisterFill( ref.current.name, ref.current ); + ref.current.name = name; + registerFill( name, ref.current ); + }, [ name ] ); - componentDidUpdate( prevProps ) { - const { name, unregisterFill, registerFill } = this.props; + const slot = getSlot( name ); - if ( prevProps.name !== name ) { - unregisterFill( prevProps.name, this ); - registerFill( name, this ); - } + if ( ! slot || ! slot.node || ! slot.props.bubblesVirtually ) { + return null; } - resetOccurrence() { - this.occurrence = null; + // If a function is passed as a child, provide it with the fillProps. + if ( isFunction( children ) ) { + children = children( slot.props.fillProps ); } - render() { - const { name, getSlot } = this.props; - let { children } = this.props; - const slot = getSlot( name ); - - if ( ! slot || ! slot.node || ! slot.props.bubblesVirtually ) { - return null; - } - - // If a function is passed as a child, provide it with the fillProps. - if ( isFunction( children ) ) { - children = children( slot.props.fillProps ); - } - - return createPortal( children, slot.node ); - } + return createPortal( children, slot.node ); } const Fill = ( props ) => ( diff --git a/packages/components/src/slot-fill/slot.js b/packages/components/src/slot-fill/slot.js index 68f11b1395b83..6de69f74f865a 100644 --- a/packages/components/src/slot-fill/slot.js +++ b/packages/components/src/slot-fill/slot.js @@ -64,7 +64,7 @@ class SlotComponent extends Component { const fills = map( getFills( name, this ), ( fill ) => { const fillKey = fill.occurrence; - const fillChildren = isFunction( fill.props.children ) ? fill.props.children( fillProps ) : fill.props.children; + const fillChildren = isFunction( fill.children ) ? fill.children( fillProps ) : fill.children; return Children.map( fillChildren, ( child, childIndex ) => { if ( ! child || isString( child ) ) { From 7c40b023b5c2fbf32a32fc2d86cdef567986ee38 Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Mon, 20 May 2019 09:54:02 +0100 Subject: [PATCH 2/2] Fix the reordering issue --- packages/components/src/slot-fill/context.js | 3 +-- packages/components/src/slot-fill/fill.js | 23 ++++++++++---------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/packages/components/src/slot-fill/context.js b/packages/components/src/slot-fill/context.js index a39ac94013e0a..7ee0d9806c400 100644 --- a/packages/components/src/slot-fill/context.js +++ b/packages/components/src/slot-fill/context.js @@ -97,13 +97,12 @@ class SlotFillProvider extends Component { if ( this.slots[ name ] !== slotInstance ) { return []; } - return sortBy( this.fills[ name ], 'occurrence' ); } resetFillOccurrence( name ) { forEach( this.fills[ name ], ( instance ) => { - instance.resetOccurrence(); + instance.occurrence = undefined; } ); } diff --git a/packages/components/src/slot-fill/fill.js b/packages/components/src/slot-fill/fill.js index f4ebb6b93dbae..a0c2876643f04 100644 --- a/packages/components/src/slot-fill/fill.js +++ b/packages/components/src/slot-fill/fill.js @@ -6,7 +6,7 @@ import { isFunction } from 'lodash'; /** * WordPress dependencies */ -import { createPortal, useEffect, useRef, useState } from '@wordpress/element'; +import { createPortal, useLayoutEffect, useRef, useState } from '@wordpress/element'; /** * Internal dependencies @@ -25,20 +25,17 @@ function FillComponent( { name, getSlot, children, registerFill, unregisterFill children, } ); - useEffect( () => { - ref.current.occurence = ++occurrences; - ref.current.resetOccurrence = () => { - ref.current.occurence = null; - }; + if ( ! ref.current.occurrence ) { + ref.current.occurrence = ++occurrences; + } + + useLayoutEffect( () => { ref.current.forceUpdate = rerender; registerFill( name, ref.current ); return () => unregisterFill( name, ref.current ); }, [] ); - useEffect( () => { - if ( ! ref.current.occurence ) { - ref.current.occurence = ++occurrences; - } + useLayoutEffect( () => { ref.current.children = children; const slot = getSlot( name ); if ( slot && ! slot.props.bubblesVirtually ) { @@ -46,7 +43,11 @@ function FillComponent( { name, getSlot, children, registerFill, unregisterFill } }, [ children ] ); - useEffect( () => { + useLayoutEffect( () => { + if ( name === ref.current.name ) { + // ignore initial effect + return; + } unregisterFill( ref.current.name, ref.current ); ref.current.name = name; registerFill( name, ref.current );