From 00b71c4e0fef0a1404c188afe28dbea01e8a7e03 Mon Sep 17 00:00:00 2001 From: Jovi De Croock Date: Sat, 17 Feb 2024 09:56:28 +0100 Subject: [PATCH 1/3] fix and add test for 4283 --- src/diff/children.js | 1 + test/browser/render.test.js | 53 +++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/src/diff/children.js b/src/diff/children.js index 71879d3166..8e7cead49b 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -252,6 +252,7 @@ function constructNewChildrenArray(newParentVNode, renderResult, oldChildren) { // assume DOM nodes in this subtree are mounted and usable. oldChildren[i] = null; remainingOldChildren--; + skew++; } continue; } diff --git a/test/browser/render.test.js b/test/browser/render.test.js index 3be0393a20..9daf4b6099 100644 --- a/test/browser/render.test.js +++ b/test/browser/render.test.js @@ -1356,4 +1356,57 @@ describe('render()', () => { rerender(); expect(scratch.innerHTML).to.equal('
B
'); }); + + it('should not crash or repeatedly add the same child when replacing a matched vnode with null (mixed dom-types)', () => { + const B = () =>
B
; + + let update; + class App extends Component { + constructor(props) { + super(props); + this.state = { show: true }; + update = () => { + this.setState(state => ({ show: !state.show })); + }; + } + + render() { + if (this.state.show) { + return ( +
+ +
+
+ ); + } + return ( +
+ + {null} + +
+
+ ); + } + } + + render(, scratch); + expect(scratch.innerHTML).to.equal('
B
'); + + update(); + rerender(); + expect(scratch.innerHTML).to.equal( + '
B
' + ); + + update(); + rerender(); + expect(scratch.innerHTML).to.equal('
B
'); + + update(); + rerender(); + expect(scratch.innerHTML).to.equal( + '
B
' + ); + }); }); From 94b0563ac509f89c7251862086a7f4d3625d5a1f Mon Sep 17 00:00:00 2001 From: jdecroock Date: Sat, 17 Feb 2024 12:59:21 +0100 Subject: [PATCH 2/3] less agressive fix --- src/diff/children.js | 3 ++- test/browser/fragments.test.js | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/diff/children.js b/src/diff/children.js index 8e7cead49b..c4faed9cfe 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -238,6 +238,8 @@ function constructNewChildrenArray(newParentVNode, renderResult, oldChildren) { ) { if (oldVNode._dom == newParentVNode._nextDom) { newParentVNode._nextDom = getDomSibling(oldVNode); + } else { + skew++; } unmount(oldVNode, oldVNode, false); @@ -252,7 +254,6 @@ function constructNewChildrenArray(newParentVNode, renderResult, oldChildren) { // assume DOM nodes in this subtree are mounted and usable. oldChildren[i] = null; remainingOldChildren--; - skew++; } continue; } diff --git a/test/browser/fragments.test.js b/test/browser/fragments.test.js index 20fed865e4..4634a4320e 100644 --- a/test/browser/fragments.test.js +++ b/test/browser/fragments.test.js @@ -1304,7 +1304,7 @@ describe('Fragment', () => { 'rendering from true to false' ); expectDomLogToBe( - ['
  • 1.remove()', '
  • 2.remove()'], + ['
  • 1.remove()', '
  • 2.remove()', '
      043.appendChild(
    1. 4)'], 'rendering from true to false' ); From b55824286026b0ba20e818e2ef339bb61553cc33 Mon Sep 17 00:00:00 2001 From: Andre Wiggins <459878+andrewiggins@users.noreply.github.com> Date: Wed, 21 Feb 2024 22:39:43 -0800 Subject: [PATCH 3/3] Match null placeholders using skewed index (#4290) If we've skewed our matching before hitting a null placeholder (e.g. we've inserted or removed an unmatched node) then let's pick up matching null placeholders from the skewedIndex. Note I don't think we need to adjust the skew when we find a null placeholder, we treat it as "matching" the current node. --- src/diff/children.js | 9 ++++----- test/browser/fragments.test.js | 2 +- test/browser/render.test.js | 15 ++++++++------- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/diff/children.js b/src/diff/children.js index c4faed9cfe..ed08514f4d 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -227,9 +227,11 @@ function constructNewChildrenArray(newParentVNode, renderResult, oldChildren) { childVNode = newParentVNode._children[i] = childVNode; } + const skewedIndex = i + skew; + // Handle unmounting null placeholders, i.e. VNode => null in unkeyed children if (childVNode == null) { - oldVNode = oldChildren[i]; + oldVNode = oldChildren[skewedIndex]; if ( oldVNode && oldVNode.key == null && @@ -238,8 +240,6 @@ function constructNewChildrenArray(newParentVNode, renderResult, oldChildren) { ) { if (oldVNode._dom == newParentVNode._nextDom) { newParentVNode._nextDom = getDomSibling(oldVNode); - } else { - skew++; } unmount(oldVNode, oldVNode, false); @@ -252,7 +252,7 @@ function constructNewChildrenArray(newParentVNode, renderResult, oldChildren) { // to unmount this VNode again seeing `_match==true`. Further, // getDomSibling doesn't know about _match and so would incorrectly // assume DOM nodes in this subtree are mounted and usable. - oldChildren[i] = null; + oldChildren[skewedIndex] = null; remainingOldChildren--; } continue; @@ -261,7 +261,6 @@ function constructNewChildrenArray(newParentVNode, renderResult, oldChildren) { childVNode._parent = newParentVNode; childVNode._depth = newParentVNode._depth + 1; - const skewedIndex = i + skew; const matchingIndex = findMatchingIndex( childVNode, oldChildren, diff --git a/test/browser/fragments.test.js b/test/browser/fragments.test.js index 4634a4320e..20fed865e4 100644 --- a/test/browser/fragments.test.js +++ b/test/browser/fragments.test.js @@ -1304,7 +1304,7 @@ describe('Fragment', () => { 'rendering from true to false' ); expectDomLogToBe( - ['
    2. 1.remove()', '
    3. 2.remove()', '
        043.appendChild(
      1. 4)'], + ['
      2. 1.remove()', '
      3. 2.remove()'], 'rendering from true to false' ); diff --git a/test/browser/render.test.js b/test/browser/render.test.js index 9daf4b6099..075b6ad39f 100644 --- a/test/browser/render.test.js +++ b/test/browser/render.test.js @@ -1360,6 +1360,7 @@ describe('render()', () => { it('should not crash or repeatedly add the same child when replacing a matched vnode with null (mixed dom-types)', () => { const B = () =>
        B
        ; + /** @type {() => void} */ let update; class App extends Component { constructor(props) { @@ -1375,38 +1376,38 @@ describe('render()', () => { return (
        -
        +
        C
        ); } return (
        - + A {null} -
        +
        C
        ); } } render(, scratch); - expect(scratch.innerHTML).to.equal('
        B
        '); + expect(scratch.innerHTML).to.equal('
        B
        C
        '); update(); rerender(); expect(scratch.innerHTML).to.equal( - '
        B
        ' + '
        A
        B
        C
        ' ); update(); rerender(); - expect(scratch.innerHTML).to.equal('
        B
        '); + expect(scratch.innerHTML).to.equal('
        B
        C
        '); update(); rerender(); expect(scratch.innerHTML).to.equal( - '
        B
        ' + '
        A
        B
        C
        ' ); }); });