diff --git a/compat/test/browser/suspense.test.js b/compat/test/browser/suspense.test.js index 7d511474b7..a7d3c8b7a5 100644 --- a/compat/test/browser/suspense.test.js +++ b/compat/test/browser/suspense.test.js @@ -308,7 +308,9 @@ describe('suspense', () => { const FuncWrapper = props =>
{props.children}
; - const [Suspender, suspend] = createSuspender(() =>
Hello
); + const [Suspender, suspend] = createSuspender(() => ( +
Hello
+ )); render( Suspended...}> @@ -322,7 +324,7 @@ describe('suspense', () => { ); expect(scratch.innerHTML).to.eql( - `
Hello
` + `
Hello
` ); const [resolve] = suspend(); @@ -330,10 +332,10 @@ describe('suspense', () => { expect(scratch.innerHTML).to.eql(`
Suspended...
`); - return resolve(() =>
Hello2
).then(() => { + return resolve(() =>
Hello2
).then(() => { rerender(); expect(scratch.innerHTML).to.eql( - `
Hello2
` + `
Hello2
` ); }); }); diff --git a/src/diff/children.js b/src/diff/children.js index c40120178e..251c6b63d2 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -120,6 +120,13 @@ export function diffChildren( if (oldVNode && oldVNode.key == null && oldVNode._dom) { if (oldVNode._dom == oldDom) { oldDom = getDomSibling(oldVNode); + + if (typeof newParentVNode.type == 'function') { + // If the parent VNode is a component/fragment, make sure its diff + // continues with a DOM node that is still mounted in case this loop + // exits here because the rest of the new children are `null`. + newParentVNode._nextDom = oldDom; + } } unmount(oldVNode, oldVNode, false); @@ -171,75 +178,75 @@ export function diffChildren( refQueue.push(j, childVNode._component || newDom, childVNode); } - if (newDom != null) { - if (firstChildDom == null) { - firstChildDom = newDom; - } + if (firstChildDom == null && newDom != null) { + firstChildDom = newDom; + } - let isMounting = oldVNode === EMPTY_OBJ || oldVNode._original === null; - if (isMounting) { - if (matchingIndex == -1) { + let isMounting = oldVNode === EMPTY_OBJ || oldVNode._original === null; + if (isMounting) { + if (matchingIndex == -1) { + skew--; + } + } else if (matchingIndex !== skewedIndex) { + if (matchingIndex === skewedIndex + 1) { + skew++; + } else if (matchingIndex > skewedIndex) { + if (remainingOldChildren > newChildrenLength - skewedIndex) { + skew += matchingIndex - skewedIndex; + } else { + // ### Change from keyed: I think this was missing from the algo... skew--; } - } else if (matchingIndex !== skewedIndex) { - if (matchingIndex === skewedIndex + 1) { - skew++; - } else if (matchingIndex > skewedIndex) { - if (remainingOldChildren > newChildrenLength - skewedIndex) { - skew += matchingIndex - skewedIndex; - } else { - // ### Change from keyed: I think this was missing from the algo... - skew--; - } - } else if (matchingIndex < skewedIndex) { - if (matchingIndex == skewedIndex - 1) { - skew = matchingIndex - skewedIndex; - } else { - skew = 0; - } + } else if (matchingIndex < skewedIndex) { + if (matchingIndex == skewedIndex - 1) { + skew = matchingIndex - skewedIndex; } else { skew = 0; } + } else { + skew = 0; } + } - skewedIndex = i + skew; - - if (typeof childVNode.type == 'function') { - if ( - matchingIndex !== skewedIndex || - oldVNode._children === childVNode._children - ) { - oldDom = reorderChildren(childVNode, oldDom, parentDom); - } else if (childVNode._nextDom !== undefined) { - // Only Fragments or components that return Fragment like VNodes will - // have a non-undefined _nextDom. Continue the diff from the sibling - // of last DOM child of this child VNode - oldDom = childVNode._nextDom; - } else { - oldDom = newDom.nextSibling; - } + skewedIndex = i + skew; + + if (typeof childVNode.type == 'function') { + if ( + matchingIndex !== skewedIndex || + oldVNode._children === childVNode._children + ) { + oldDom = reorderChildren(childVNode, oldDom, parentDom); + } else if (childVNode._nextDom !== undefined) { + // Only Fragments or components that return Fragment like VNodes will + // have a non-undefined _nextDom. Continue the diff from the sibling + // of last DOM child of this child VNode + oldDom = childVNode._nextDom; + } else if (newDom) { + oldDom = newDom.nextSibling; + } - // Eagerly cleanup _nextDom. We don't need to persist the value because - // it is only used by `diffChildren` to determine where to resume the diff after - // diffing Components and Fragments. Once we store it the nextDOM local var, we - // can clean up the property - childVNode._nextDom = undefined; - } else if (matchingIndex !== skewedIndex || isMounting) { + // Eagerly cleanup _nextDom. We don't need to persist the value because + // it is only used by `diffChildren` to determine where to resume the diff after + // diffing Components and Fragments. Once we store it the nextDOM local var, we + // can clean up the property + childVNode._nextDom = undefined; + } else if (newDom) { + if (matchingIndex !== skewedIndex || isMounting) { oldDom = placeChild(parentDom, newDom, oldDom); } else { oldDom = newDom.nextSibling; } + } - if (typeof newParentVNode.type == 'function') { - // Because the newParentVNode is Fragment-like, we need to set it's - // _nextDom property to the nextSibling of its last child DOM node. - // - // `oldDom` contains the correct value here because if the last child - // is a Fragment-like, then oldDom has already been set to that child's _nextDom. - // If the last child is a DOM VNode, then oldDom will be set to that DOM - // node's nextSibling. - newParentVNode._nextDom = oldDom; - } + if (typeof newParentVNode.type == 'function') { + // Because the newParentVNode is Fragment-like, we need to set it's + // _nextDom property to the nextSibling of its last child DOM node. + // + // `oldDom` contains the correct value here because if the last child + // is a Fragment-like, then oldDom has already been set to that child's _nextDom. + // If the last child is a DOM VNode, then oldDom will be set to that DOM + // node's nextSibling. + newParentVNode._nextDom = oldDom; } } @@ -251,12 +258,11 @@ export function diffChildren( if ( typeof newParentVNode.type == 'function' && oldChildren[i]._dom != null && - oldChildren[i]._dom == newParentVNode._nextDom + oldChildren[i]._dom == oldDom ) { - // If the newParentVNode.__nextDom points to a dom node that is about to - // be unmounted, then get the next sibling of that vnode and set - // _nextDom to it - + // If oldDom points to a dom node that is about to be unmounted, then + // get the next sibling of that vnode and set _nextDom to it, so the + // parent's diff continues diffing an existing DOM node newParentVNode._nextDom = oldChildren[i]._dom.nextSibling; } @@ -322,10 +328,8 @@ export function toChildArray(children, out) { * @returns {PreactElement} */ function placeChild(parentDom, newDom, oldDom) { - if (oldDom == null || oldDom.parentNode !== parentDom) { - parentDom.insertBefore(newDom, null); - } else if (newDom != oldDom || newDom.parentNode == null) { - parentDom.insertBefore(newDom, oldDom); + if (newDom != oldDom) { + parentDom.insertBefore(newDom, oldDom || null); } return newDom.nextSibling; diff --git a/test/browser/fragments.test.js b/test/browser/fragments.test.js index 6136c028b4..3e5292f2c5 100644 --- a/test/browser/fragments.test.js +++ b/test/browser/fragments.test.js @@ -1,7 +1,7 @@ import { setupRerender } from 'preact/test-utils'; import { createElement, render, Component, Fragment } from 'preact'; import { setupScratch, teardown } from '../_util/helpers'; -import { span, div, ul, ol, li, section } from '../_util/dom'; +import { span, div, ul, ol, li, section, p } from '../_util/dom'; import { logCall, clearLog, getLog } from '../_util/logCall'; /** @jsx createElement */ @@ -647,6 +647,7 @@ describe('Fragment', () => { }); it('should preserve order for fragment switching', () => { + /** @type {(newState: { isLoading: boolean; data: number | null }) => void} */ let set; class Foo extends Component { constructor(props) { @@ -678,7 +679,7 @@ describe('Fragment', () => { }); it('should preserve order for fragment switching with sibling DOM', () => { - /** @type {() => void} */ + /** @type {(newState: { isLoading: boolean; data: number | null }) => void} */ let set; class Foo extends Component { constructor(props) { @@ -711,7 +712,7 @@ describe('Fragment', () => { }); it('should preserve order for fragment switching with sibling Components', () => { - /** @type {() => void} */ + /** @type {(newState: { isLoading: boolean; data: number | null }) => void} */ let set; class Foo extends Component { constructor(props) { @@ -3227,4 +3228,101 @@ describe('Fragment', () => { expect(scratch.innerHTML).to.equal([div('A'), div(1), div(2)].join('')); expectDomLogToBe(['
B.remove()', '
2A1.appendChild(
2)']); }); + + it('should efficiently unmount & mount components that conditionally return null', () => { + function Conditional({ condition }) { + return condition ? ( + +

2

+

3

+
+ ) : null; + } + + /** @type {() => void} */ + let toggle; + class App extends Component { + constructor(props) { + super(props); + this.state = { condition: true }; + toggle = () => + this.setState(prevState => ({ condition: !prevState.condition })); + } + + render() { + const { condition } = this.state; + return ( + +

1

+ + {condition ? null :

A

} +

4

+
+ ); + } + } + + render(, scratch); + expect(scratch.innerHTML).to.equal([1, 2, 3, 4].map(p).join('')); + + clearLog(); + toggle(); + rerender(); + + expect(scratch.innerHTML).to.equal([1, 'A', 4].map(p).join('')); + expectDomLogToBe([ + '

2.remove()', + '

3.remove()', + '

.appendChild(#text)', + '

14.insertBefore(

A,

4)' + ]); + }); + + it('should efficiently unmount & mount components that conditionally return null with null first child', () => { + function Conditional({ condition }) { + return condition ? ( + + {null} +

3

+ + ) : null; + } + + /** @type {() => void} */ + let toggle; + class App extends Component { + constructor(props) { + super(props); + this.state = { condition: true }; + toggle = () => + this.setState(prevState => ({ condition: !prevState.condition })); + } + + render() { + const { condition } = this.state; + return ( + +

1

+ + {condition ? null :

A

} +

4

+
+ ); + } + } + + render(, scratch); + expect(scratch.innerHTML).to.equal([1, 3, 4].map(p).join('')); + + clearLog(); + toggle(); + rerender(); + + expect(scratch.innerHTML).to.equal([1, 'A', 4].map(p).join('')); + expectDomLogToBe([ + '

3.remove()', + '

.appendChild(#text)', + '

14.insertBefore(

A,

4)' + ]); + }); });