Components: Handle multiple Slots by same name (#12882)

WordPress · Jan 22, 2019 · 527e25c · 527e25c
1 parent 0628573
commit 527e25c
Show file tree

Hide file tree

Showing 6 changed files with 128 additions and 4 deletions.
diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md
@@ -5,6 +5,10 @@
 - `withFilters` has been optimized to avoid binding hook handlers for each mounted instance of the component, instead using a single centralized hook delegator.
 - `withFilters` has been optimized to reuse a single shared component definition for all filtered instances of the component.
 
+### Bug Fixes
+
+- Resolves a conflict where two instance of Slot would produce an inconsistent or duplicated rendering output.
+
 ## 7.0.5 (2019-01-03)
 
 ## 7.0.4 (2018-12-12)

diff --git a/packages/components/src/slot-fill/context.js b/packages/components/src/slot-fill/context.js
@@ -41,12 +41,21 @@ class SlotFillProvider extends Component {
 	}
 
 	registerSlot( name, slot ) {
+		const previousSlot = this.slots[ name ];
 		this.slots[ name ] = slot;
 		this.forceUpdateFills( name );
 
 		// Sometimes the fills are registered after the initial render of slot
 		// But before the registerSlot call, we need to rerender the slot
 		this.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.forceUpdate();
+		}
 	}
 
 	registerFill( name, instance ) {
@@ -57,7 +66,14 @@ class SlotFillProvider extends Component {
 		this.forceUpdateSlot( name );
 	}
 
-	unregisterSlot( name ) {
+	unregisterSlot( name, instance ) {
+		// 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 ( this.slots[ name ] !== instance ) {
+			return;
+		}
+
 		delete this.slots[ name ];
 		this.forceUpdateFills( name );
 	}
@@ -75,7 +91,13 @@ class SlotFillProvider extends Component {
 		return this.slots[ name ];
 	}
 
-	getFills( name ) {
+	getFills( name, slotInstance ) {
+		// Fills should only be returned for the current instance of the slot
+		// in which they occupy.
+		if ( this.slots[ name ] !== slotInstance ) {
+			return [];
+		}
+
 		return sortBy( this.fills[ name ], 'occurrence' );
 	}
 

diff --git a/packages/components/src/slot-fill/fill.js b/packages/components/src/slot-fill/fill.js
@@ -62,7 +62,7 @@ class FillComponent extends Component {
 		let { children } = this.props;
 		const slot = getSlot( name );
 
-		if ( ! slot || ! slot.props.bubblesVirtually ) {
+		if ( ! slot || ! slot.node || ! slot.props.bubblesVirtually ) {
 			return null;
 		}
 

diff --git a/packages/components/src/slot-fill/slot.js b/packages/components/src/slot-fill/slot.js
@@ -63,7 +63,7 @@ class SlotComponent extends Component {
 			return <div ref={ this.bindNode } />;
 		}
 
-		const fills = map( getFills( name ), ( fill ) => {
+		const fills = map( getFills( name, this ), ( fill ) => {
 			const fillKey = fill.occurrence;
 			const fillChildren = isFunction( fill.props.children ) ? fill.props.children( fillProps ) : fill.props.children;
 

diff --git a/packages/components/src/slot-fill/test/__snapshots__/slot.js.snap b/packages/components/src/slot-fill/test/__snapshots__/slot.js.snap
@@ -1,5 +1,59 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
+exports[`Slot bubblesVirtually false should subsume another slot by the same name 1`] = `
+Array [
+  <div
+    data-position="first"
+  />,
+  <div
+    data-position="second"
+  >
+    Content
+  </div>,
+]
+`;
+
+exports[`Slot bubblesVirtually false should subsume another slot by the same name 2`] = `
+Array [
+  <div
+    data-position="first"
+  />,
+  <div
+    data-position="second"
+  >
+    Content
+  </div>,
+]
+`;
+
+exports[`Slot bubblesVirtually true should subsume another slot by the same name 1`] = `
+Array [
+  <div
+    data-position="first"
+  >
+    <div />
+  </div>,
+  <div
+    data-position="second"
+  >
+    <div />
+  </div>,
+]
+`;
+
+exports[`Slot bubblesVirtually true should subsume another slot by the same name 2`] = `
+Array [
+  <div
+    data-position="first"
+  />,
+  <div
+    data-position="second"
+  >
+    <div />
+  </div>,
+]
+`;
+
 exports[`Slot should re-render Slot when not bubbling virtually 1`] = `
 Array [
   <div>

diff --git a/packages/components/src/slot-fill/test/slot.js b/packages/components/src/slot-fill/test/slot.js
@@ -209,4 +209,48 @@ describe( 'Slot', () => {
 
 		expect( testRenderer.toJSON() ).toMatchSnapshot();
 	} );
+
+	[ false, true ].forEach( ( bubblesVirtually ) => {
+		describe( 'bubblesVirtually ' + bubblesVirtually, () => {
+			it( 'should subsume another slot by the same name', () => {
+				const testRenderer = ReactTestRenderer.create(
+					<Provider>
+						<div data-position="first">
+							<Slot name="egg" bubblesVirtually={ bubblesVirtually } />
+						</div>
+						<div data-position="second"></div>
+						<Fill name="egg">Content</Fill>
+					</Provider>
+				);
+
+				testRenderer.update(
+					<Provider>
+						<div data-position="first">
+							<Slot name="egg" bubblesVirtually={ bubblesVirtually } />
+						</div>
+						<div data-position="second">
+							<Slot name="egg" bubblesVirtually={ bubblesVirtually } />
+						</div>
+						<Fill name="egg">Content</Fill>
+					</Provider>
+				);
+
+				expect( testRenderer.toJSON() ).toMatchSnapshot();
+
+				testRenderer.update(
+					<Provider>
+						<div data-position="first"></div>
+						<div data-position="second">
+							<Slot name="egg" bubblesVirtually={ bubblesVirtually } />
+						</div>
+						<Fill name="egg">Content</Fill>
+					</Provider>
+				);
+
+				expect( testRenderer.toJSON() ).toMatchSnapshot();
+
+				expect( testRenderer.getInstance().slots ).toHaveProperty( 'egg' );
+			} );
+		} );
+	} );
 } );