Eagerly unmount placeholders (#4090)

* Eagerly unmount placeholders

* Fix case when unmounting fragment with multiple dom children

---------

Co-authored-by: Jovi De Croock <decroockjovi@gmail.com>

src/diff/children.js

@@ -2,6 +2,7 @@ import { diff, unmount, applyRef } from './index';
 import { createVNode, Fragment } from '../create-element';
 import { EMPTY_OBJ, EMPTY_ARR } from '../constants';
 import { isArray } from '../util';
+import { getDomSibling } from '../component';
 
 /**
  * Diff the children of a virtual node
@@ -107,6 +108,15 @@ export function diffChildren(
 		// Terser removes the `continue` here and wraps the loop body
 		// in a `if (childVNode) { ... } condition
 		if (childVNode == null) {
+			oldVNode = oldChildren[i];
+			if (oldVNode && oldVNode.key == null && oldVNode._dom) {
+				if (oldVNode._dom == oldDom) {
+					oldDom = getDomSibling(oldVNode);
+				}
+
+				unmount(oldVNode, oldVNode, false);
+			}
+
 			continue;
 		}
 

test/browser/fragments.test.js

@@ -1328,14 +1328,14 @@ describe('Fragment', () => {
 			'rendering from true to false'
 		);
 		expectDomLogToBe([
+			// Remove 1 & 2 (replaced with null)
+			'<li>0.remove()',
+			'<li>1.remove()',
 			// Mount 3 & 4
 			'<li>.appendChild(#text)',
-			'<ol>0122.appendChild(<li>3)',
+			'<ol>22.appendChild(<li>3)',
 			'<li>.appendChild(#text)',
-			'<ol>01223.appendChild(<li>4)',
-			// Remove 1 & 2 (replaced with null)
-			'<li>0.remove()',
-			'<li>1.remove()'
+			'<ol>223.appendChild(<li>4)'
 		]);
 
 		clearLog();
@@ -1399,14 +1399,14 @@ describe('Fragment', () => {
 			'rendering from true to false'
 		);
 		expectDomLogToBe([
+			// Remove 1 & 2 (replaced with null)
+			'<li>0.remove()',
+			'<li>1.remove()',
 			// Mount 4 & 5
 			'<li>.appendChild(#text)',
-			'<ol>0123.appendChild(<li>4)',
+			'<ol>23.appendChild(<li>4)',
 			'<li>.appendChild(#text)',
-			'<ol>01234.appendChild(<li>5)',
-			// Remove 1 & 2 (replaced with null)
-			'<li>0.remove()',
-			'<li>1.remove()'
+			'<ol>234.appendChild(<li>5)'
 		]);
 
 		clearLog();
@@ -2609,11 +2609,9 @@ describe('Fragment', () => {
 		rerender();
 		expect(scratch.innerHTML).to.equal(bottom);
 		expectDomLogToBe([
-			'<div>top panelNavigationContent.insertBefore(<div>Navigation, <div>top panel)',
-			'<div>Navigationtop panelContent.insertBefore(<div>Content, <div>top panel)',
+			'<div>top panel.remove()',
 			'<div>.appendChild(#text)',
-			'<div>NavigationContenttop panel.insertBefore(<div>bottom panel, <div>top panel)',
-			'<div>top panel.remove()'
+			'<div>NavigationContent.appendChild(<div>bottom panel)'
 		]);
 
 		clearLog();

test/browser/placeholders.test.js

@@ -1,4 +1,4 @@
-import { createElement, Component, render, createRef } from 'preact';
+import { createElement, Component, render, createRef, Fragment } from 'preact';
 import { setupRerender } from 'preact/test-utils';
 import { setupScratch, teardown } from '../_util/helpers';
 import { logCall, clearLog, getLog } from '../_util/logCall';
@@ -305,4 +305,116 @@ describe('null placeholders', () => {
 			'#text.remove()'
 		]);
 	});
+
+	it('when set through the children prop (#4074)', () => {
+		/** @type {(state: { value: boolean }) => void} */
+		let setState;
+		const Iframe = () => {
+			// Using a div here to make debugging tests in devtools a little less
+			// noisy. The dom ops still assert that the iframe isn't moved.
+			//
+			// return <iframe src="https://codesandbox.io/s/runtime-silence-no4zx" />;
+			return <div>Iframe</div>;
+		};
+
+		const Test2 = () => <div>Test2</div>;
+		const Test3 = () => <div>Test3</div>;
+
+		class App extends Component {
+			constructor(props) {
+				super(props);
+				this.state = { value: true };
+				setState = this.setState.bind(this);
+			}
+
+			render(props, state) {
+				return (
+					<div>
+						<Test2 />
+						{state.value && <Test3 />}
+						{props.children}
+					</div>
+				);
+			}
+		}
+
+		render(
+			<App>
+				<Iframe />
+			</App>,
+			scratch
+		);
+
+		expect(scratch.innerHTML).to.equal(
+			'<div><div>Test2</div><div>Test3</div><div>Iframe</div></div>'
+		);
+		clearLog();
+		setState({ value: false });
+		rerender();
+
+		expect(scratch.innerHTML).to.equal(
+			'<div><div>Test2</div><div>Iframe</div></div>'
+		);
+		expect(getLog()).to.deep.equal(['<div>Test3.remove()']);
+	});
+
+	it('when set through the children prop when removing a Fragment with multiple DOM children (#4074)', () => {
+		/** @type {(state: { value: boolean }) => void} */
+		let setState;
+		const Iframe = () => {
+			// Using a div here to make debugging tests in devtools a little less
+			// noisy. The dom ops still assert that the iframe isn't moved.
+			//
+			// return <iframe src="https://codesandbox.io/s/runtime-silence-no4zx" />;
+			return <div>Iframe</div>;
+		};
+
+		const Test2 = () => <div>Test2</div>;
+		const Test34 = () => (
+			<Fragment>
+				<div>Test3</div>
+				<div>Test4</div>
+			</Fragment>
+		);
+
+		class App extends Component {
+			constructor(props) {
+				super(props);
+				this.state = { value: true };
+				setState = this.setState.bind(this);
+			}
+
+			render(props, state) {
+				return (
+					<div>
+						<Test2 />
+						{state.value && <Test34 />}
+						{props.children}
+					</div>
+				);
+			}
+		}
+
+		render(
+			<App>
+				<Iframe />
+			</App>,
+			scratch
+		);
+
+		expect(scratch.innerHTML).to.equal(
+			'<div><div>Test2</div><div>Test3</div><div>Test4</div><div>Iframe</div></div>'
+		);
+		clearLog();
+		setState({ value: false });
+		rerender();
+
+		expect(scratch.innerHTML).to.equal(
+			'<div><div>Test2</div><div>Iframe</div></div>'
+		);
+		expect(getLog()).to.deep.equal([
+			'<div>Test3.remove()',
+			'<div>Test4.remove()'
+		]);
+	});
 });

test/browser/render.test.js

@@ -1101,8 +1101,8 @@ describe('render()', () => {
 			'<div><iframe src="https://codesandbox.io/s/runtime-silence-no4zx"></iframe></div>'
 		);
 		expect(getLog()).to.deep.equal([
-			'<div>Test2.remove()',
-			'<div>Test3.remove()'
+			'<div>Test3.remove()',
+			'<div>Test2.remove()'
 		]);
 	});