Skip to content

Commit

Permalink
Merge pull request #4172 from preactjs/improve-place-child
Browse files Browse the repository at this point in the history
# Improve place child

When unmounting DOM, we need to move `oldDom`/`_nextDom` past the unmounted DOM so the diff can continue by comparing the next VNodes to DOM nodes that are mounted. We did this in the unmount loop but we missed a couple other places where this needs to happen as well.

Let's say we have the following components:
```jsx
function Conditional({ condition }) {
  return condition ? (
      <>
        <p>2</p>
        <p>3</p>
      </>
    ) : null;
  }

function App() { return <>... <Conditional/> ... </>; }
```

Here, Conditional will switch from a group of DOM nodes and `null`. When doing this, diffChildren would hit our null placeholder code which does update oldDom past the unmounted DOM, but it doesn't update `_nextDom`, so `App` wouldn't see this move and try to continue diffing from an unmounted DOM node.

Further, even if we set `_nextDOM`, diffChildren on App wouldn't see it because `Conditional` returned `null`. Since Conditional returned `null`, it would return `null` for `newDom`,  meaning diffChildren on App would skip over all the code that was previously under a `if (newDom)` check, including the code that reads `_nextDom`. This PR also fixes that issue so even if a component returns `null`, we update the skewed index, and will read `_nextDom` if necessary.

Also, the test "should efficiently unmount & mount components that conditionally return null with null first child" contains an additional edge case where Conditional switching to null, matches it's previous null first child. We need to ensure we detect we still need to move oldDom and update `_nextDom`.

Previously we attempted to handle all of these cases by checking `oldDom.parentNode !== parentDom` in `placeChild`. However, in our entire test case, this was only occurred when oldDom was unmounted. So this update prevents unmounted oldDom from ever reaching `placeChild` removing the need for that condition entirely, dramatically simplifying `placeChild`.
  • Loading branch information
andrewiggins committed Oct 28, 2023
2 parents 8e5eac5 + 137da3c commit 9956916
Show file tree
Hide file tree
Showing 3 changed files with 175 additions and 71 deletions.
10 changes: 6 additions & 4 deletions compat/test/browser/suspense.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,9 @@ describe('suspense', () => {

const FuncWrapper = props => <div id="func-wrapper">{props.children}</div>;

const [Suspender, suspend] = createSuspender(() => <div>Hello</div>);
const [Suspender, suspend] = createSuspender(() => (
<div id="initial-contents">Hello</div>
));

render(
<Suspense fallback={<div>Suspended...</div>}>
Expand All @@ -322,18 +324,18 @@ describe('suspense', () => {
);

expect(scratch.innerHTML).to.eql(
`<div id="class-wrapper"><div id="func-wrapper"><div>Hello</div></div></div>`
`<div id="class-wrapper"><div id="func-wrapper"><div id="initial-contents">Hello</div></div></div>`
);

const [resolve] = suspend();
rerender();

expect(scratch.innerHTML).to.eql(`<div>Suspended...</div>`);

return resolve(() => <div>Hello2</div>).then(() => {
return resolve(() => <div id="resolved-contents">Hello2</div>).then(() => {
rerender();
expect(scratch.innerHTML).to.eql(
`<div id="class-wrapper"><div id="func-wrapper"><div>Hello2</div></div></div>`
`<div id="class-wrapper"><div id="func-wrapper"><div id="resolved-contents">Hello2</div></div></div>`
);
});
});
Expand Down
132 changes: 68 additions & 64 deletions src/diff/children.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}
}

Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
Expand Down
104 changes: 101 additions & 3 deletions test/browser/fragments.test.js
Original file line number Diff line number Diff line change
@@ -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 */
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -3227,4 +3228,101 @@ describe('Fragment', () => {
expect(scratch.innerHTML).to.equal([div('A'), div(1), div(2)].join(''));
expectDomLogToBe(['<div>B.remove()', '<div>2A1.appendChild(<div>2)']);
});

it('should efficiently unmount & mount components that conditionally return null', () => {
function Conditional({ condition }) {
return condition ? (
<Fragment>
<p>2</p>
<p>3</p>
</Fragment>
) : 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 (
<Fragment>
<p>1</p>
<Conditional condition={condition} />
{condition ? null : <p>A</p>}
<p>4</p>
</Fragment>
);
}
}

render(<App />, 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([
'<p>2.remove()',
'<p>3.remove()',
'<p>.appendChild(#text)',
'<div>14.insertBefore(<p>A, <p>4)'
]);
});

it('should efficiently unmount & mount components that conditionally return null with null first child', () => {
function Conditional({ condition }) {
return condition ? (
<Fragment>
{null}
<p>3</p>
</Fragment>
) : 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 (
<Fragment>
<p>1</p>
<Conditional condition={condition} />
{condition ? null : <p>A</p>}
<p>4</p>
</Fragment>
);
}
}

render(<App />, 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([
'<p>3.remove()',
'<p>.appendChild(#text)',
'<div>14.insertBefore(<p>A, <p>4)'
]);
});
});

0 comments on commit 9956916

Please sign in to comment.