From 13638e77f164f63ddbd1b0610169226fc27f8898 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Tue, 24 Oct 2023 08:15:27 -0600 Subject: [PATCH 1/3] Manually track child index in parent In `getDomSibling` we do a `indexOf` search in the parent VNode to find the location of the vnode in the parent's children array. However, when rerendering a component we copy the VNode to make the oldVNode. This copy if the VNode means the `indexOf` search won't work cuz that copy doesn't exist the parent's children array. So calls to `getDomSibling(oldVNode)` would not return the correct result. This PR fixes this by manually tracking the index the childVNode is in the parent's children array so we no longer need to do the indexOf search. --- mangle.json | 1 + src/component.js | 4 ++-- src/diff/children.js | 4 +++- src/internal.d.ts | 1 + 4 files changed, 7 insertions(+), 3 deletions(-) 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..c0271c12d3 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; } } 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 { From 97f0465b8b0cd8ac527f229d8fc7d0faf912c2b4 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Wed, 25 Oct 2023 14:16:29 -0600 Subject: [PATCH 2/3] Add more tests to capture more situations we need to address --- src/component.js | 6 +++ test/browser/fragments.test.js | 78 ++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+) diff --git a/src/component.js b/src/component.js index c0271c12d3..9d8bfb0a49 100644 --- a/src/component.js +++ b/src/component.js @@ -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/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 ( + + +