Components: Try to make the order of fills stable in regular slots (#…

…29287) * Components: Try to make the order of fills stable in legacy slots * Update the package lock file * Tests: Clarify rerender behavior for fills
WordPress · Mar 4, 2021 · f5b36f3 · f5b36f3
1 parent 4b52595
commit f5b36f3
Show file tree

Hide file tree

Showing 8 changed files with 102 additions and 27 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git a/packages/components/src/slot-fill/context.js b/packages/components/src/slot-fill/context.js
@@ -1,7 +1,7 @@
 /**
  * External dependencies
  */
-import { sortBy, forEach, without } from 'lodash';
+import { without } from 'lodash';
 
 /**
  * WordPress dependencies
@@ -95,7 +95,6 @@ class SlotFillProvider extends Component {
 
 	unregisterFill( name, instance ) {
 		this.fills[ name ] = without( this.fills[ name ], instance );
-		this.resetFillOccurrence( name );
 		this.forceUpdateSlot( name );
 	}
 
@@ -109,19 +108,13 @@ class SlotFillProvider extends Component {
 		if ( this.slots[ name ] !== slotInstance ) {
 			return [];
 		}
-		return sortBy( this.fills[ name ], 'occurrence' );
+		return this.fills[ name ];
 	}
 
 	hasFills( name ) {
 		return this.fills[ name ] && !! this.fills[ name ].length;
 	}
 
-	resetFillOccurrence( name ) {
-		forEach( this.fills[ name ], ( instance ) => {
-			instance.occurrence = undefined;
-		} );
-	}
-
 	forceUpdateSlot( name ) {
 		const slot = this.getSlot( name );
 

diff --git a/packages/components/src/slot-fill/fill.js b/packages/components/src/slot-fill/fill.js
@@ -13,8 +13,6 @@ import { createPortal, useLayoutEffect, useRef } from '@wordpress/element';
  */
 import { Consumer, useSlot } from './context';
 
-let occurrences = 0;
-
 function FillComponent( { name, children, registerFill, unregisterFill } ) {
 	const slot = useSlot( name );
 
@@ -23,10 +21,6 @@ function FillComponent( { name, children, registerFill, unregisterFill } ) {
 		children,
 	} );
 
-	if ( ! ref.current.occurrence ) {
-		ref.current.occurrence = ++occurrences;
-	}
-
 	useLayoutEffect( () => {
 		registerFill( name, ref.current );
 		return () => unregisterFill( name, ref.current );

diff --git a/packages/components/src/slot-fill/slot.js b/packages/components/src/slot-fill/slot.js
@@ -62,7 +62,6 @@ class SlotComponent extends Component {
 		const { children, name, fillProps = {}, getFills } = this.props;
 
 		const fills = map( getFills( name, this ), ( fill ) => {
-			const fillKey = fill.occurrence;
 			const fillChildren = isFunction( fill.children )
 				? fill.children( fillProps )
 				: fill.children;
@@ -72,7 +71,7 @@ class SlotComponent extends Component {
 					return child;
 				}
 
-				const childKey = `${ fillKey }---${ child.key || childIndex }`;
+				const childKey = child.key || childIndex;
 				return cloneElement( child, { key: childKey } );
 			} );
 		} ).filter(

diff --git a/packages/components/src/slot-fill/test/__snapshots__/slot.js.snap b/packages/components/src/slot-fill/test/__snapshots__/slot.js.snap
@@ -150,12 +150,31 @@ exports[`Slot should render empty Fills without HTML wrapper when render props u
 </div>
 `;
 
-exports[`Slot should render in expected order 1`] = `
+exports[`Slot should render in expected order when fills always mounted 1`] = `
 <div>
   <div>
-    first
+    first (rerendered)
     second
+    third
+    fourth (new)
   </div>
+</div>
+`;
+
+exports[`Slot should render in expected order when fills unmounted 1`] = `
+<div>
+  <div>
+    second
+    third
+    first (rerendered)
+    fourth (new)
+  </div>
+  <button
+    type="button"
+  />
+  <button
+    type="button"
+  />
   <button
     type="button"
   />

diff --git a/packages/components/src/slot-fill/test/slot.js b/packages/components/src/slot-fill/test/slot.js
@@ -167,7 +167,68 @@ describe( 'Slot', () => {
 		expect( container ).toMatchSnapshot();
 	} );
 
-	it( 'should render in expected order', () => {
+	it( 'should render in expected order when fills always mounted', () => {
+		const { container, rerender } = render(
+			<Provider>
+				<div key="slot">
+					<Slot name="egg" />
+				</div>
+			</Provider>
+		);
+
+		rerender(
+			<Provider>
+				<div key="slot">
+					<Slot name="egg" />
+				</div>
+				<Fill name="egg" key="first">
+					first
+				</Fill>
+				<Fill name="egg" key="second">
+					second
+				</Fill>
+			</Provider>
+		);
+
+		rerender(
+			<Provider>
+				<div key="slot">
+					<Slot name="egg" />
+				</div>
+				<Fill name="egg" key="first" />
+				<Fill name="egg" key="second">
+					second
+				</Fill>
+				<Fill name="egg" key="third">
+					third
+				</Fill>
+			</Provider>
+		);
+
+		rerender(
+			<Provider>
+				<div key="slot">
+					<Slot name="egg" />
+				</div>
+				<Fill name="egg" key="first">
+					first (rerendered)
+				</Fill>
+				<Fill name="egg" key="second">
+					second
+				</Fill>
+				<Fill name="egg" key="third">
+					third
+				</Fill>
+				<Fill name="egg" key="fourth">
+					fourth (new)
+				</Fill>
+			</Provider>
+		);
+
+		expect( container ).toMatchSnapshot();
+	} );
+
+	it( 'should render in expected order when fills unmounted', () => {
 		const { container, rerender } = render(
 			<Provider>
 				<div key="slot">
@@ -192,6 +253,7 @@ describe( 'Slot', () => {
 					<Slot name="egg" />
 				</div>
 				<Filler name="egg" key="second" text="second" />
+				<Filler name="egg" key="third" text="third" />
 			</Provider>
 		);
 
@@ -200,8 +262,10 @@ describe( 'Slot', () => {
 				<div key="slot">
 					<Slot name="egg" />
 				</div>
-				<Filler name="egg" key="first" text="first" />
+				<Filler name="egg" key="first" text="first (rerendered)" />
 				<Filler name="egg" key="second" text="second" />
+				<Filler name="egg" key="third" text="third" />
+				<Filler name="egg" key="fourth" text="fourth (new)" />
 			</Provider>
 		);
 

diff --git a/packages/plugins/package.json b/packages/plugins/package.json
@@ -27,7 +27,8 @@
 		"@wordpress/element": "file:../element",
 		"@wordpress/hooks": "file:../hooks",
 		"@wordpress/icons": "file:../icons",
-		"lodash": "^4.17.19"
+		"lodash": "^4.17.19",
+		"memize": "^1.1.0"
 	},
 	"publishConfig": {
 		"access": "public"

diff --git a/packages/plugins/src/components/plugin-area/index.js b/packages/plugins/src/components/plugin-area/index.js
@@ -2,6 +2,7 @@
  * External dependencies
  */
 import { map } from 'lodash';
+import memoize from 'memize';
 
 /**
  * WordPress dependencies
@@ -54,6 +55,12 @@ class PluginArea extends Component {
 		super( ...arguments );
 
 		this.setPlugins = this.setPlugins.bind( this );
+		this.memoizedContext = memoize( ( name, icon ) => {
+			return {
+				name,
+				icon,
+			};
+		} );
 		this.state = this.getCurrentPluginsState();
 	}
 
@@ -64,10 +71,7 @@ class PluginArea extends Component {
 				( { icon, name, render } ) => {
 					return {
 						Plugin: render,
-						context: {
-							name,
-							icon,
-						},
+						context: this.memoizedContext( name, icon ),
 					};
 				}
 			),