From f77b6ee983eaffc18940f860d0ae19fb49e033bb Mon Sep 17 00:00:00 2001 From: Jovi De Croock Date: Thu, 8 Feb 2024 08:31:30 +0100 Subject: [PATCH 1/5] add test for the issue, TODO convert to class components later --- hooks/test/browser/combinations.test.js | 40 +++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/hooks/test/browser/combinations.test.js b/hooks/test/browser/combinations.test.js index 3b7cc51ab1..54aef62c11 100644 --- a/hooks/test/browser/combinations.test.js +++ b/hooks/test/browser/combinations.test.js @@ -477,4 +477,44 @@ describe('combinations', () => { 'anchor effect' ]); }); + + it.only('should not crash', () => { + const B = () =>
B
; + + let update; + const Test = () => { + const [state, setState] = useState(true); + update = () => setState(s => !s); + + if (state) { + 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 bd3769504c093aea0c4d376c2510acc5a555d903 Mon Sep 17 00:00:00 2001 From: jdecroock Date: Thu, 8 Feb 2024 10:24:37 +0100 Subject: [PATCH 2/5] wip --- hooks/test/browser/combinations.test.js | 4 ++++ src/diff/children.js | 12 +++++++++--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/hooks/test/browser/combinations.test.js b/hooks/test/browser/combinations.test.js index 54aef62c11..1684d5c92f 100644 --- a/hooks/test/browser/combinations.test.js +++ b/hooks/test/browser/combinations.test.js @@ -502,18 +502,22 @@ describe('combinations', () => {
); }; + render(, scratch); expect(scratch.innerHTML).to.equal('
B
'); update(); + console.log('--- RENDER'); rerender(); expect(scratch.innerHTML).to.equal('
B
'); update(); + console.log('--- RENDER'); rerender(); expect(scratch.innerHTML).to.equal('
B
'); update(); + console.log('--- RENDER'); rerender(); expect(scratch.innerHTML).to.equal('
B
'); }); diff --git a/src/diff/children.js b/src/diff/children.js index 8256e45d74..58e057570e 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -62,7 +62,6 @@ export function diffChildren( for (i = 0; i < newChildrenLength; i++) { childVNode = newParentVNode._children[i]; - if ( childVNode == null || typeof childVNode == 'boolean' || @@ -98,6 +97,13 @@ export function diffChildren( // Adjust DOM nodes newDom = childVNode._dom; + console.log( + 'compared', + newDom, + childVNode._index, + childVNode && childVNode.type, + oldVNode && oldVNode.type + ); if (childVNode.ref && oldVNode.ref != childVNode.ref) { if (oldVNode.ref) { applyRef(oldVNode.ref, null, childVNode); @@ -113,6 +119,7 @@ export function diffChildren( firstChildDom = newDom; } + console.log(newDom, childVNode._flags & INSERT_VNODE); if ( childVNode._flags & INSERT_VNODE || oldVNode._children === childVNode._children @@ -235,7 +242,7 @@ function constructNewChildrenArray(newParentVNode, renderResult, oldChildren) { if (oldVNode._dom == newParentVNode._nextDom) { newParentVNode._nextDom = getDomSibling(oldVNode); } - + console.log('unmounting', oldVNode.type); unmount(oldVNode, oldVNode, false); // Explicitly nullify this position in oldChildren instead of just @@ -250,7 +257,6 @@ function constructNewChildrenArray(newParentVNode, renderResult, oldChildren) { oldChildren[i] = null; remainingOldChildren--; } - continue; } From e5429654571abdbcf30340ce37999b697381c8a0 Mon Sep 17 00:00:00 2001 From: jdecroock Date: Wed, 14 Feb 2024 12:40:35 +0100 Subject: [PATCH 3/5] correct assertion --- hooks/test/browser/combinations.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hooks/test/browser/combinations.test.js b/hooks/test/browser/combinations.test.js index 1684d5c92f..2347dea79a 100644 --- a/hooks/test/browser/combinations.test.js +++ b/hooks/test/browser/combinations.test.js @@ -514,7 +514,7 @@ describe('combinations', () => { update(); console.log('--- RENDER'); rerender(); - expect(scratch.innerHTML).to.equal('
B
'); + expect(scratch.innerHTML).to.equal('
B
'); update(); console.log('--- RENDER'); From d1f8ee5284083f121ddc7e0f07b43ecdc447b9ac Mon Sep 17 00:00:00 2001 From: jdecroock Date: Fri, 16 Feb 2024 09:31:55 +0100 Subject: [PATCH 4/5] avoid crasshing when replacing existing node --- hooks/test/browser/combinations.test.js | 5 +---- src/diff/children.js | 16 ++++++---------- 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/hooks/test/browser/combinations.test.js b/hooks/test/browser/combinations.test.js index 2347dea79a..638d9efa1f 100644 --- a/hooks/test/browser/combinations.test.js +++ b/hooks/test/browser/combinations.test.js @@ -478,7 +478,7 @@ describe('combinations', () => { ]); }); - it.only('should not crash', () => { + it('should not crash or repeatedly add the same child when replacing a matched vnode with null', () => { const B = () =>
B
; let update; @@ -507,17 +507,14 @@ describe('combinations', () => { expect(scratch.innerHTML).to.equal('
B
'); update(); - console.log('--- RENDER'); rerender(); expect(scratch.innerHTML).to.equal('
B
'); update(); - console.log('--- RENDER'); rerender(); expect(scratch.innerHTML).to.equal('
B
'); update(); - console.log('--- RENDER'); rerender(); expect(scratch.innerHTML).to.equal('
B
'); }); diff --git a/src/diff/children.js b/src/diff/children.js index 58e057570e..1845c8ae26 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -97,13 +97,6 @@ export function diffChildren( // Adjust DOM nodes newDom = childVNode._dom; - console.log( - 'compared', - newDom, - childVNode._index, - childVNode && childVNode.type, - oldVNode && oldVNode.type - ); if (childVNode.ref && oldVNode.ref != childVNode.ref) { if (oldVNode.ref) { applyRef(oldVNode.ref, null, childVNode); @@ -119,7 +112,6 @@ export function diffChildren( firstChildDom = newDom; } - console.log(newDom, childVNode._flags & INSERT_VNODE); if ( childVNode._flags & INSERT_VNODE || oldVNode._children === childVNode._children @@ -238,11 +230,15 @@ function constructNewChildrenArray(newParentVNode, renderResult, oldChildren) { // Handle unmounting null placeholders, i.e. VNode => null in unkeyed children if (childVNode == null) { oldVNode = oldChildren[i]; - if (oldVNode && oldVNode.key == null && oldVNode._dom) { + if ( + oldVNode && + oldVNode.key == null && + oldVNode._dom && + !oldVNode._flags & MATCHED + ) { if (oldVNode._dom == newParentVNode._nextDom) { newParentVNode._nextDom = getDomSibling(oldVNode); } - console.log('unmounting', oldVNode.type); unmount(oldVNode, oldVNode, false); // Explicitly nullify this position in oldChildren instead of just From ddc0255e93a060cda267823e9a29ee1a316c2914 Mon Sep 17 00:00:00 2001 From: jdecroock Date: Fri, 16 Feb 2024 09:37:27 +0100 Subject: [PATCH 5/5] move test to core --- hooks/test/browser/combinations.test.js | 41 --------------------- src/diff/children.js | 2 +- test/browser/render.test.js | 48 +++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 42 deletions(-) diff --git a/hooks/test/browser/combinations.test.js b/hooks/test/browser/combinations.test.js index 638d9efa1f..3b7cc51ab1 100644 --- a/hooks/test/browser/combinations.test.js +++ b/hooks/test/browser/combinations.test.js @@ -477,45 +477,4 @@ describe('combinations', () => { 'anchor effect' ]); }); - - it('should not crash or repeatedly add the same child when replacing a matched vnode with null', () => { - const B = () =>
B
; - - let update; - const Test = () => { - const [state, setState] = useState(true); - update = () => setState(s => !s); - - if (state) { - 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
'); - }); }); diff --git a/src/diff/children.js b/src/diff/children.js index 1845c8ae26..71879d3166 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -234,7 +234,7 @@ function constructNewChildrenArray(newParentVNode, renderResult, oldChildren) { oldVNode && oldVNode.key == null && oldVNode._dom && - !oldVNode._flags & MATCHED + (oldVNode._flags & MATCHED) === 0 ) { if (oldVNode._dom == newParentVNode._nextDom) { newParentVNode._nextDom = getDomSibling(oldVNode); diff --git a/test/browser/render.test.js b/test/browser/render.test.js index 2baf381d3a..3be0393a20 100644 --- a/test/browser/render.test.js +++ b/test/browser/render.test.js @@ -1308,4 +1308,52 @@ describe('render()', () => { expect(divs[1].hasAttribute('role')).to.equal(false); expect(divs[2].hasAttribute('role')).to.equal(false); }); + + it('should not crash or repeatedly add the same child when replacing a matched vnode with null', () => { + 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
'); + }); });