From cff2df5a8e25054872f343d24c72c434a8e02e9a Mon Sep 17 00:00:00 2001
From: Andre Wiggins <459878+andrewiggins@users.noreply.github.com>
Date: Thu, 26 Oct 2023 06:43:01 -0700
Subject: [PATCH] Manually track children's index & fix parent pointers when
rerendering 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 | 1 +
mangle.json | 1 +
src/component.js | 10 ++++-
src/create-element.js | 3 +-
src/diff/children.js | 4 +-
src/internal.d.ts | 1 +
test/browser/fragments.test.js | 78 ++++++++++++++++++++++++++++++++++
7 files changed, 94 insertions(+), 4 deletions(-)
diff --git a/jsx-runtime/src/index.js b/jsx-runtime/src/index.js
index b578b9da7e..0bdfd38c5a 100644
--- a/jsx-runtime/src/index.js
+++ b/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
};
diff --git a/mangle.json b/mangle.json
index 22b96e72bf..76e9fb5956 100644
--- a/mangle.json
+++ b/mangle.json
@@ -54,6 +54,7 @@
"$_dom": "__e",
"$_hydrating": "__h",
"$_component": "__c",
+ "$_index": "__i",
"$__html": "__html",
"$_parent": "__",
"$_pendingError": "__E",
diff --git a/src/component.js b/src/component.js
index 88817c2d10..9d8bfb0a49 100644
--- a/src/component.js
+++ b/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,
diff --git a/src/create-element.js b/src/create-element.js
index b55be840ad..71c44bbcb7 100644
--- a/src/create-element.js
+++ b/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:
diff --git a/src/diff/children.js b/src/diff/children.js
index 49a2e0c360..096e4d7bb9 100644
--- a/src/diff/children.js
+++ b/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(
diff --git a/src/internal.d.ts b/src/internal.d.ts
index 57faa0ded0..12b53d1d5c 100644
--- a/src/internal.d.ts
+++ b/src/internal.d.ts
@@ -122,6 +122,7 @@ export interface VNode extends preact.VNode
{
_hydrating: boolean | null;
constructor: undefined;
_original: number;
+ _index: number;
}
export interface Component
extends preact.Component {
diff --git a/test/browser/fragments.test.js b/test/browser/fragments.test.js
index f04a80d3a7..6136c028b4 100644
--- a/test/browser/fragments.test.js
+++ b/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 (
+
+ HEADER
+ {isLoading ? Loading...
: null}
+ {data ? Content: {data}
: null}
+ FOOTER
+
+ );
+ }
+ }
+
+ render(, scratch);
+ expect(scratch.innerHTML).to.equal(
+ 'HEADER
Loading...
FOOTER
'
+ );
+
+ set({ isLoading: false, data: 2 });
+ rerender();
+ expect(scratch.innerHTML).to.equal(
+ 'HEADER
Content: 2
FOOTER
'
+ );
+ });
+
+ 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 (
+
+ HEADER
+ {isLoading ? Loading...
: null}
+ {data ? Content: {data}
: null}
+
+ );
+ }
+ }
+
+ function Footer() {
+ return FOOTER
;
+ }
+
+ function App() {
+ return (
+
+
+
+
+ );
+ }
+
+ render(, scratch);
+ expect(scratch.innerHTML).to.equal(
+ 'HEADER
Loading...
FOOTER
'
+ );
+
+ set({ isLoading: false, data: 2 });
+ rerender();
+ expect(scratch.innerHTML).to.equal(
+ 'HEADER
Content: 2
FOOTER
'
+ );
+ });
+
it('should preserve order for nested fragment switching w/ child return', () => {
let set;
const Wrapper = ({ children }) => {children};