Do not re-insert memoized vnodes that keep their relative order after swap (#4888)

Author: vasylenkoval

src/diff/children.js

@@ -128,11 +128,9 @@ export function diffChildren(
 			firstChildDom = newDom;
 		}
 
-		if (
-			childVNode._flags & INSERT_VNODE ||
-			oldVNode._children === childVNode._children
-		) {
-			oldDom = insert(childVNode, oldDom, parentDom);
+		let shouldPlace = !!(childVNode._flags & INSERT_VNODE);
+		if (shouldPlace || oldVNode._children === childVNode._children) {
+			oldDom = insert(childVNode, oldDom, parentDom, shouldPlace);
 		} else if (typeof childVNode.type == 'function' && result !== UNDEFINED) {
 			oldDom = result;
 		} else if (newDom) {
@@ -342,9 +340,10 @@ function constructNewChildrenArray(
  * @param {VNode} parentVNode
  * @param {PreactElement} oldDom
  * @param {PreactElement} parentDom
+ * @param {boolean} shouldPlace
  * @returns {PreactElement}
  */
-function insert(parentVNode, oldDom, parentDom) {
+function insert(parentVNode, oldDom, parentDom, shouldPlace) {
 	// Note: VNodes in nested suspended trees may be missing _children.
 	if (typeof parentVNode.type == 'function') {
 		let children = parentVNode._children;
@@ -355,16 +354,18 @@ function insert(parentVNode, oldDom, parentDom) {
 				// children's _parent pointer to point to the newVNode (parentVNode
 				// here).
 				children[i]._parent = parentVNode;
-				oldDom = insert(children[i], oldDom, parentDom);
+				oldDom = insert(children[i], oldDom, parentDom, shouldPlace);
 			}
 		}
 
 		return oldDom;
 	} else if (parentVNode._dom != oldDom) {
-		if (oldDom && parentVNode.type && !oldDom.parentNode) {
-			oldDom = getDomSibling(parentVNode);
+		if (shouldPlace) {
+			if (oldDom && parentVNode.type && !oldDom.parentNode) {
+				oldDom = getDomSibling(parentVNode);
+			}
+			parentDom.insertBefore(parentVNode._dom, oldDom || NULL);
 		}
-		parentDom.insertBefore(parentVNode._dom, oldDom || NULL);
 		oldDom = parentVNode._dom;
 	}
 

test/browser/lifecycles/shouldComponentUpdate.test.js

@@ -2,7 +2,7 @@ import { setupRerender } from 'preact/test-utils';
 import { createElement, render, Component, Fragment } from 'preact';
 import { vi } from 'vitest';
 import { setupScratch, teardown } from '../../_util/helpers';
-import { logCall, clearLog } from '../../_util/logCall';
+import { logCall, getLog, clearLog } from '../../_util/logCall';
 
 /** @jsx createElement */
 
@@ -962,4 +962,61 @@ describe('Lifecycle methods', () => {
 			`<div>Before</div><div>Component</div><div>After</div>`
 		);
 	});
+
+	it('should not re-insert memoized items that keep their relative order after swap', () => {
+		class MemoizedItem extends Component {
+			shouldComponentUpdate(nextProps) {
+				return nextProps.value !== this.props.value;
+			}
+			render() {
+				return <div>{this.props.value}</div>;
+			}
+		}
+
+		const App = ({ items }) => (
+			<div>
+				{items.map(value => (
+					<MemoizedItem key={value} value={value} />
+				))}
+			</div>
+		);
+
+		render(<App items={[1, 2, 3, 4, 5, 6, 7]} />, scratch);
+
+		function renderItemsAndAssert({ items, expectedLog }) {
+			clearLog();
+			render(<App items={items} />, scratch);
+			expect(scratch.innerHTML).to.equal(
+				`<div>${items.map(value => `<div>${value}</div>`).join('')}</div>`
+			);
+			expect(getLog()).to.deep.equal(expectedLog);
+		}
+
+		// Swap 1 and 7
+		renderItemsAndAssert({
+			items: [7, 2, 3, 4, 5, 6, 1],
+			expectedLog: [
+				'<div>1234567.insertBefore(<div>7, <div>1)',
+				'<div>7123456.appendChild(<div>1)'
+			]
+		});
+
+		// Swap 2 and 6
+		renderItemsAndAssert({
+			items: [7, 6, 3, 4, 5, 2, 1],
+			expectedLog: [
+				'<div>7234561.insertBefore(<div>6, <div>2)',
+				'<div>7623451.insertBefore(<div>2, <div>1)'
+			]
+		});
+
+		// Swap 3 and 5
+		renderItemsAndAssert({
+			items: [7, 6, 5, 4, 3, 2, 1],
+			expectedLog: [
+				'<div>7634521.insertBefore(<div>5, <div>3)',
+				'<div>7653421.insertBefore(<div>3, <div>2)'
+			]
+		});
+	});
 });