Skip to content

Commit

Permalink
fix(child-diffing): avoid skipping re-orders (#4088)
Browse files Browse the repository at this point in the history
* add test

* fix

* remove unusued import

* see if test is failing because of the tgz missing

* Revert "see if test is failing because of the tgz missing"

This reverts commit 02ab119.
  • Loading branch information
JoviDeCroock authored Aug 3, 2023
1 parent 7940137 commit c294e8b
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 7 deletions.
11 changes: 4 additions & 7 deletions src/diff/children.js
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,6 @@ export function diffChildren(
);

newDom = childVNode._dom;

if ((j = childVNode.ref) && oldVNode.ref != j) {
if (oldVNode.ref) {
applyRef(oldVNode.ref, null, childVNode);
Expand All @@ -158,19 +157,16 @@ export function diffChildren(
}

let isMounting = oldVNode === EMPTY_OBJ || oldVNode._original === null;
let hasMatchingIndex = !isMounting && matchingIndex === skewedIndex;
if (isMounting) {
if (matchingIndex == -1) {
skew--;
}
} else if (matchingIndex !== skewedIndex) {
if (matchingIndex === skewedIndex + 1) {
skew++;
hasMatchingIndex = true;
} else if (matchingIndex > skewedIndex) {
if (remainingOldChildren > newChildrenLength - skewedIndex) {
skew += matchingIndex - skewedIndex;
hasMatchingIndex = true;
} else {
// ### Change from keyed: I think this was missing from the algo...
skew--;
Expand All @@ -187,16 +183,17 @@ export function diffChildren(
}

skewedIndex = i + skew;
hasMatchingIndex =
hasMatchingIndex || (matchingIndex == i && !isMounting);

if (
typeof childVNode.type == 'function' &&
(matchingIndex !== skewedIndex ||
oldVNode._children === childVNode._children)
) {
oldDom = reorderChildren(childVNode, oldDom, parentDom);
} else if (typeof childVNode.type != 'function' && !hasMatchingIndex) {
} else if (
typeof childVNode.type != 'function' &&
(matchingIndex !== skewedIndex || isMounting)
) {
oldDom = placeChild(parentDom, newDom, oldDom);
} else if (childVNode._nextDom !== undefined) {
// Only Fragments or components that return Fragment like VNodes will
Expand Down
63 changes: 63 additions & 0 deletions test/browser/keys.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -752,4 +752,67 @@ describe('keys', () => {
expect(Stateful1Ref).to.not.equal(Stateful1MovedRef);
expect(Stateful2Ref).to.not.equal(Stateful2MovedRef);
});

it('should handle full reorders', () => {
const keys = {
Apple: `Apple_1`,
Orange: `Orange_1`,
Banana: `Banana_1`,
Grape: `Grape_1`,
Kiwi: `Kiwi_1`,
Cherry: `Cherry_1`
};

let sort;

class App extends Component {
order;

state = { items: ['Apple', 'Grape', 'Cherry', 'Orange', 'Banana'] };

sort() {
this.order = this.order === 'ASC' ? 'DESC' : 'ASC';
const items = [...this.state.items].sort((a, b) =>
this.order === 'ASC' ? a.localeCompare(b) : b.localeCompare(a)
);
this.setState({ items });
}

render(_, { items }) {
sort = this.sort.bind(this);
return (
<div>
{items.map(item => (
<div key={keys[item]}>{item}</div>
))}
</div>
);
}
}

const expected = values => {
return values.map(key => `<div>${key}</div>`).join('');
};

render(<App />, scratch);
expect(scratch.innerHTML).to.eq(
`<div>${expected(['Apple', 'Grape', 'Cherry', 'Orange', 'Banana'])}</div>`
);

let sorted = ['Apple', 'Grape', 'Cherry', 'Orange', 'Banana'].sort((a, b) =>
a.localeCompare(b)
);
sort();
rerender();

expect(scratch.innerHTML).to.eq(`<div>${expected(sorted)}</div>`);

sorted = ['Apple', 'Grape', 'Cherry', 'Orange', 'Banana'].sort((a, b) =>
b.localeCompare(a)
);
sort();
rerender();

expect(scratch.innerHTML).to.eq(`<div>${expected(sorted)}</div>`);
});
});

0 comments on commit c294e8b

Please sign in to comment.