Manually track children's index & fix parent pointers when rerenderin…

…g components (#4170)

When rerendering a component we copy the component's VNode to make the oldVNode. However, copying the VNode breaks two connections in the virtual node tree: 

1) The `_parent` pointer of children of the `oldVNode` point to the original VNode, not the copied one. But in this situation, they should point to the `oldVNode` since they belong to the old tree and not the new tree we are about to rerender/reconstruct.

2) the copied VNode (`oldVNode`) doesn't exist in its parent's children array, so in `getDomSibling`, the `indexOf` search we do on a VNode's parent won't work. So calls to `getDomSibling(oldVNode)` would not return the correct result. 

Previously we discovered this issue when using null placeholders in components that setState. So the current fixes address that particular scenario by:

1) Before calling getDomSibling, we set the child's parent to `oldVNode` (called `oldParentVNode` at this point in `diffChildren`). Before that fix, parent pointer of the child VNode would point to newParentVNode, which is incorrect. In diffChildren, we set `newParentVNode._children = []` and fill it as we loop through new children, meaning child VNode would never occur in newParentVNode._children.

2) We return `_nextDom` instead of `_dom` in `getDomSibling` if it is set. This fix coincidentally worked for the test cases we had because there was only one component with no sibling components. In this situation, `getDomSibling` would return a DOM node that is about to be unmounted and so all DOM after it would be re-appended. In the situation where a component has a sibling component that also returns a Fragment, returning `_nextDom` would prevent us from seeing this new sibling component and it's dom node and and so we would incorrectly append children after it's DOM. This situation is captured in a new test case.

Now, having a deeper understanding of the issue, this PR proposes two fixes to address this problems more holistically.

1) To address the first issue, after copying the componet's VNode to `oldVNode`, we loop through the children and set their _parent pointer to `oldVNode`.

2) Instead of using `indexOf` in `getDomSibling` we will track a VNode's index in it's parent's children array as a property on the VNode, removing the need for `indexOf` in `getDomSibling`. Note: this doesn't yet fully fix the second issue ("`oldVNode` doesn't exist in the parent's children array"), but it works around that issue by relying on the fact that in this situation the VNode it was copied from will be at the same index in the parent's children array and so works for `getDomSibling`. A future PR will attempt to address this problem more directly.

So in summary, tl;dr:

1) After copying a component's VNode to rerender it, update the children to point to the copied VNode.
2) Manually track a child's index in its parent array to remove the dependence on `indexOf` in `getDomSibling`.

Also note: #4162 also adds an `_index` property to VNodes so adding that property has multiple benefits, besides this one PR

jsx-runtime/src/index.js

@@ -53,6 +53,7 @@ function createVNode(type, props, key, isStaticChildren, __source, __self) {
 		_hydrating: null,
 		constructor: undefined,
 		_original: --vnodeId,
+		_index: -1,
 		__source,
 		__self
 	};

mangle.json

@@ -54,6 +54,7 @@
       "$_dom": "__e",
       "$_hydrating": "__h",
       "$_component": "__c",
+      "$_index": "__i",
       "$__html": "__html",
       "$_parent": "__",
       "$_pendingError": "__E",

src/component.js

@@ -91,7 +91,7 @@ export function getDomSibling(vnode, childIndex) {
 	if (childIndex == null) {
 		// Use childIndex==null as a signal to resume the search from the vnode's sibling
 		return vnode._parent
-			? getDomSibling(vnode._parent, vnode._parent._children.indexOf(vnode) + 1)
+			? getDomSibling(vnode._parent, vnode._index + 1)
 			: null;
 	}
 
@@ -103,7 +103,7 @@ export function getDomSibling(vnode, childIndex) {
 			// Since updateParentDomPointers keeps _dom pointer correct,
 			// we can rely on _dom to tell us if this subtree contains a
 			// rendered DOM node, and what the first rendered DOM node is
-			return sibling._nextDom || sibling._dom;
+			return sibling._dom;
 		}
 	}
 
@@ -130,6 +130,12 @@ function renderComponent(component) {
 		const oldVNode = assign({}, vnode);
 		oldVNode._original = vnode._original + 1;
 
+		if (oldVNode._children) {
+			oldVNode._children.forEach(child => {
+				if (child) child._parent = oldVNode;
+			});
+		}
+
 		diff(
 			parentDom,
 			vnode,

src/create-element.js

@@ -72,7 +72,8 @@ export function createVNode(type, props, key, ref, original) {
 		_component: null,
 		_hydrating: null,
 		constructor: undefined,
-		_original: original == null ? ++vnodeId : original
+		_original: original == null ? ++vnodeId : original,
+		_index: -1
 	};
 
 	// Only invoke the vnode hook if this was *not* a direct copy:

src/diff/children.js

@@ -40,7 +40,9 @@ export function diffChildren(
 ) {
 	let i,
 		j,
+		/** @type {import('../internal').VNode} */
 		oldVNode,
+		/** @type {import('../internal').VNode} */
 		childVNode,
 		newDom,
 		firstChildDom,
@@ -111,7 +113,6 @@ export function diffChildren(
 			oldVNode = oldChildren[i];
 			if (oldVNode && oldVNode.key == null && oldVNode._dom) {
 				if (oldVNode._dom == oldDom) {
-					oldVNode._parent = oldParentVNode;
 					oldDom = getDomSibling(oldVNode);
 				}
 
@@ -124,6 +125,7 @@ export function diffChildren(
 
 		childVNode._parent = newParentVNode;
 		childVNode._depth = newParentVNode._depth + 1;
+		childVNode._index = i;
 
 		let skewedIndex = i + skew;
 		const matchingIndex = findMatchingIndex(

src/internal.d.ts

@@ -122,6 +122,7 @@ export interface VNode<P = {}> extends preact.VNode<P> {
 	_hydrating: boolean | null;
 	constructor: undefined;
 	_original: number;
+	_index: number;
 }
 
 export interface Component<P = {}, S = {}> extends preact.Component<P, S> {

test/browser/fragments.test.js

@@ -677,6 +677,84 @@ describe('Fragment', () => {
 		);
 	});
 
+	it('should preserve order for fragment switching with sibling DOM', () => {
+		/** @type {() => void} */
+		let set;
+		class Foo extends Component {
+			constructor(props) {
+				super(props);
+				this.state = { isLoading: true, data: null };
+				set = this.setState.bind(this);
+			}
+			render(props, { isLoading, data }) {
+				return (
+					<Fragment>
+						<div>HEADER</div>
+						{isLoading ? <div>Loading...</div> : null}
+						{data ? <div>Content: {data}</div> : null}
+						<div>FOOTER</div>
+					</Fragment>
+				);
+			}
+		}
+
+		render(<Foo />, scratch);
+		expect(scratch.innerHTML).to.equal(
+			'<div>HEADER</div><div>Loading...</div><div>FOOTER</div>'
+		);
+
+		set({ isLoading: false, data: 2 });
+		rerender();
+		expect(scratch.innerHTML).to.equal(
+			'<div>HEADER</div><div>Content: 2</div><div>FOOTER</div>'
+		);
+	});
+
+	it('should preserve order for fragment switching with sibling Components', () => {
+		/** @type {() => void} */
+		let set;
+		class Foo extends Component {
+			constructor(props) {
+				super(props);
+				this.state = { isLoading: true, data: null };
+				set = this.setState.bind(this);
+			}
+			render(props, { isLoading, data }) {
+				return (
+					<Fragment>
+						<div>HEADER</div>
+						{isLoading ? <div>Loading...</div> : null}
+						{data ? <div>Content: {data}</div> : null}
+					</Fragment>
+				);
+			}
+		}
+
+		function Footer() {
+			return <div>FOOTER</div>;
+		}
+
+		function App() {
+			return (
+				<Fragment>
+					<Foo />
+					<Footer />
+				</Fragment>
+			);
+		}
+
+		render(<App />, scratch);
+		expect(scratch.innerHTML).to.equal(
+			'<div>HEADER</div><div>Loading...</div><div>FOOTER</div>'
+		);
+
+		set({ isLoading: false, data: 2 });
+		rerender();
+		expect(scratch.innerHTML).to.equal(
+			'<div>HEADER</div><div>Content: 2</div><div>FOOTER</div>'
+		);
+	});
+
 	it('should preserve order for nested fragment switching w/ child return', () => {
 		let set;
 		const Wrapper = ({ children }) => <Fragment>{children}</Fragment>;