From e5487393455e2ff4c0f9b9d0ad1c2a1fcaf9ec5d Mon Sep 17 00:00:00 2001 From: jdecroock Date: Thu, 20 Feb 2025 10:56:31 +0100 Subject: [PATCH 1/5] Should handle swapping insertion --- src/diff/children.js | 2 +- test/browser/keys.test.js | 34 ++++++++++++++++++++++++++++++++++ test/browser/render.test.js | 29 +++++++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 1 deletion(-) diff --git a/src/diff/children.js b/src/diff/children.js index c40b4d5b1e..c37d9c4873 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -252,7 +252,7 @@ function constructNewChildrenArray( const isMounting = oldVNode == NULL || oldVNode._original === NULL; if (isMounting) { - if (matchingIndex == -1) { + if (matchingIndex == -1 && oldChildrenLength !== newChildrenLength) { skew--; } diff --git a/test/browser/keys.test.js b/test/browser/keys.test.js index 8d5a185b91..3fef36b85b 100644 --- a/test/browser/keys.test.js +++ b/test/browser/keys.test.js @@ -943,4 +943,38 @@ describe('keys', () => { expect(scratch.innerHTML).to.eq(`
${expected(sorted)}
`); }); + + it('should handle keyed replacements', () => { + const actions = []; + class Comp extends Component { + componentDidMount() { + actions.push('mounted ' + this.props.i); + } + render() { + return
Hello
; + } + } + + const App = props => { + return ( +
+ + {false} + + +
+ ); + }; + + render(, scratch); + expect(actions).to.deep.equal(['mounted 1', 'mounted 2', 'mounted 3']); + + render(, scratch); + expect(actions).to.deep.equal([ + 'mounted 1', + 'mounted 2', + 'mounted 3', + 'mounted 1' + ]); + }); }); diff --git a/test/browser/render.test.js b/test/browser/render.test.js index bb006c6868..87ee33ecb4 100644 --- a/test/browser/render.test.js +++ b/test/browser/render.test.js @@ -1967,4 +1967,33 @@ describe('render()', () => { 'Test

Test

\n' ); }); + + it('should not remount components when replacing a component with a falsy value in-between', () => { + const actions = []; + class Comp extends Component { + componentDidMount() { + actions.push('mounted ' + this.props.i); + } + render() { + return
Hello
; + } + } + + const App = props => { + return ( +
+ {props.y === '1' ? :
} + {false} + + +
+ ); + }; + + render(, scratch); + expect(actions).to.deep.equal(['mounted 1', 'mounted 2', 'mounted 3']); + + render(, scratch); + expect(actions).to.deep.equal(['mounted 1', 'mounted 2', 'mounted 3']); + }); }); From ab1d4206ecea18e004acf737976acefde7c7443f Mon Sep 17 00:00:00 2001 From: jdecroock Date: Thu, 20 Feb 2025 10:58:52 +0100 Subject: [PATCH 2/5] Optimize further --- src/diff/children.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/diff/children.js b/src/diff/children.js index c37d9c4873..82be357e21 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -252,8 +252,12 @@ function constructNewChildrenArray( const isMounting = oldVNode == NULL || oldVNode._original === NULL; if (isMounting) { - if (matchingIndex == -1 && oldChildrenLength !== newChildrenLength) { - skew--; + if (matchingIndex == -1) { + if (newChildrenLength > oldChildrenLength) { + skew--; + } else if (newChildrenLength < oldChildrenLength) { + skew++; + } } // If we are mounting a DOM VNode, mark it for insertion From 9f448ae5f3e56ef41684cee732b2d9983e0f0dfc Mon Sep 17 00:00:00 2001 From: jdecroock Date: Thu, 20 Feb 2025 11:35:33 +0100 Subject: [PATCH 3/5] Handle more edge cases --- src/diff/children.js | 3 +- test/browser/keys.test.js | 147 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 149 insertions(+), 1 deletion(-) diff --git a/src/diff/children.js b/src/diff/children.js index 82be357e21..16ff51ef98 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -253,6 +253,7 @@ function constructNewChildrenArray( if (isMounting) { if (matchingIndex == -1) { + //skew--; if (newChildrenLength > oldChildrenLength) { skew--; } else if (newChildrenLength < oldChildrenLength) { @@ -411,7 +412,7 @@ function findMatchingIndex( (oldVNode != NULL && (oldVNode._flags & MATCHED) == 0 ? 1 : 0); if ( - oldVNode === NULL || + (oldVNode === NULL && !childVNode.key) || (oldVNode && key == oldVNode.key && type === oldVNode.type && diff --git a/test/browser/keys.test.js b/test/browser/keys.test.js index 3fef36b85b..73c8911c20 100644 --- a/test/browser/keys.test.js +++ b/test/browser/keys.test.js @@ -977,4 +977,151 @@ describe('keys', () => { 'mounted 1' ]); }); + + it('should handle hole prepend', () => { + const actions = []; + class Comp extends Component { + componentDidMount() { + actions.push('mounted ' + this.props.i); + } + render() { + return
Hello
; + } + } + + const App = props => { + return props.y === '2' ? ( +
+ + + +
+ ) : ( +
+ {null} + + + +
+ ); + }; + + render(, scratch); + expect(actions).to.deep.equal(['mounted 1', 'mounted 2', 'mounted 3']); + + render(, scratch); + expect(actions).to.deep.equal(['mounted 1', 'mounted 2', 'mounted 3']); + }); + + it('should handle hole replace', () => { + const actions = []; + class Comp extends Component { + componentDidMount() { + actions.push('mounted ' + this.props.i); + } + render() { + return
Hello
; + } + } + + const App = props => { + return props.y === '1' ? ( +
+ + + +
+ ) : ( +
+ + {null} + +
+ ); + }; + + render(, scratch); + expect(actions).to.deep.equal(['mounted 1', 'mounted 2', 'mounted 3']); + + render(, scratch); + expect(actions).to.deep.equal(['mounted 1', 'mounted 2', 'mounted 3']); + + render(, scratch); + expect(actions).to.deep.equal([ + 'mounted 1', + 'mounted 2', + 'mounted 3', + 'mounted 2' + ]); + }); + + it('should handle hole insert', () => { + const actions = []; + class Comp extends Component { + componentDidMount() { + actions.push('mounted ' + this.props.i); + } + render() { + return
Hello
; + } + } + + const App = props => { + return props.y === '2' ? ( +
+ + + +
+ ) : ( +
+ + + {null} + +
+ ); + }; + + render(, scratch); + expect(actions).to.deep.equal(['mounted 1', 'mounted 2', 'mounted 3']); + + render(, scratch); + expect(actions).to.deep.equal(['mounted 1', 'mounted 2', 'mounted 3']); + }); + + it('should handle hole apppend', () => { + const actions = []; + class Comp extends Component { + componentDidMount() { + actions.push('mounted ' + this.props.i); + } + render() { + return
Hello
; + } + } + + const App = props => { + return props.y === '2' ? ( +
+ + + +
+ ) : ( +
+ + + + {null} +
+ ); + }; + + render(, scratch); + expect(actions).to.deep.equal(['mounted 1', 'mounted 2', 'mounted 3']); + + render(, scratch); + expect(actions).to.deep.equal(['mounted 1', 'mounted 2', 'mounted 3']); + }); }); From 01b42a96c54a4c9a8d8c81f62c99c21547e4e4c9 Mon Sep 17 00:00:00 2001 From: Jovi De Croock Date: Thu, 20 Feb 2025 11:48:49 +0100 Subject: [PATCH 4/5] Correctness --- src/diff/children.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/diff/children.js b/src/diff/children.js index 16ff51ef98..3531e7d830 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -253,7 +253,6 @@ function constructNewChildrenArray( if (isMounting) { if (matchingIndex == -1) { - //skew--; if (newChildrenLength > oldChildrenLength) { skew--; } else if (newChildrenLength < oldChildrenLength) { @@ -412,7 +411,7 @@ function findMatchingIndex( (oldVNode != NULL && (oldVNode._flags & MATCHED) == 0 ? 1 : 0); if ( - (oldVNode === NULL && !childVNode.key) || + (oldVNode === NULL && childVNode.key == null) || (oldVNode && key == oldVNode.key && type === oldVNode.type && From 725f2d29aa62b7de5ff63e08116caea3f5f4e1b1 Mon Sep 17 00:00:00 2001 From: jdecroock Date: Fri, 21 Feb 2025 09:13:51 +0100 Subject: [PATCH 5/5] Add explanatory comment --- src/diff/children.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/diff/children.js b/src/diff/children.js index 3531e7d830..dc98a7ae01 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -253,6 +253,20 @@ function constructNewChildrenArray( if (isMounting) { if (matchingIndex == -1) { + // When the array of children is growing we need to decrease the skew + // as we are adding a new element to the array. + // Example: + // [1, 2, 3] --> [0, 1, 2, 3] + // oldChildren newChildren + // + // The new element is at index 0, so our skew is 0, + // we need to decrease the skew as we are adding a new element. + // The decrease will cause us to compare the element at position 1 + // with value 1 with the element at position 0 with value 0. + // + // A linear concept is applied when the array is shrinking, + // if the length is unchanged we can assume that no skew + // changes are needed. if (newChildrenLength > oldChildrenLength) { skew--; } else if (newChildrenLength < oldChildrenLength) {