Skip to content

Commit

Permalink
Match null placeholders using skewed index
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
andrewiggins committed Feb 22, 2024
1 parent 94b0563 commit 987b7bb
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 13 deletions.
9 changes: 4 additions & 5 deletions src/diff/children.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Expand All @@ -238,8 +240,6 @@ function constructNewChildrenArray(newParentVNode, renderResult, oldChildren) {
) {
if (oldVNode._dom == newParentVNode._nextDom) {
newParentVNode._nextDom = getDomSibling(oldVNode);
} else {
skew++;
}
unmount(oldVNode, oldVNode, false);

Expand All @@ -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;
Expand All @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion test/browser/fragments.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1304,7 +1304,7 @@ describe('Fragment', () => {
'rendering from true to false'
);
expectDomLogToBe(
['<li>1.remove()', '<li>2.remove()', '<ol>043.appendChild(<li>4)'],
['<li>1.remove()', '<li>2.remove()'],
'rendering from true to false'
);

Expand Down
15 changes: 8 additions & 7 deletions test/browser/render.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = () => <div>B</div>;

/** @type {() => void} */
let update;
class App extends Component {
constructor(props) {
Expand All @@ -1375,38 +1376,38 @@ describe('render()', () => {
return (
<div>
<B />
<div />
<div>C</div>
</div>
);
}
return (
<div>
<span />
<span>A</span>
{null}
<B />
<div />
<div>C</div>
</div>
);
}
}

render(<App />, scratch);
expect(scratch.innerHTML).to.equal('<div><div>B</div><div></div></div>');
expect(scratch.innerHTML).to.equal('<div><div>B</div><div>C</div></div>');

update();
rerender();
expect(scratch.innerHTML).to.equal(
'<div><span></span><div>B</div><div></div></div>'
'<div><span>A</span><div>B</div><div>C</div></div>'
);

update();
rerender();
expect(scratch.innerHTML).to.equal('<div><div>B</div><div></div></div>');
expect(scratch.innerHTML).to.equal('<div><div>B</div><div>C</div></div>');

update();
rerender();
expect(scratch.innerHTML).to.equal(
'<div><span></span><div>B</div><div></div></div>'
'<div><span>A</span><div>B</div><div>C</div></div>'
);
});
});

0 comments on commit 987b7bb

Please sign in to comment.