Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recreate unkeyed functional components when they change position. #4550

Merged
merged 3 commits into from
Nov 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/diff/children.js
Original file line number Diff line number Diff line change
Expand Up @@ -397,9 +397,13 @@ function findMatchingIndex(
// remainingOldChildren > 1 if the oldVNode is not already used/matched. Else
// if the oldVNode was null or matched, then there could needs to be at least
// 1 (aka `remainingOldChildren > 0`) children to find and compare against.
//
// If there is an unkeyed functional VNode, that isn't a built-in like our Fragment,
// we should not search as we risk re-using state of an unrelated VNode.
let shouldSearch =
(typeof type !== 'function' || type === Fragment || key) &&
remainingOldChildren >
(oldVNode != null && (oldVNode._flags & MATCHED) === 0 ? 1 : 0);
(oldVNode != null && (oldVNode._flags & MATCHED) === 0 ? 1 : 0);

if (
oldVNode === null ||
Expand Down
71 changes: 71 additions & 0 deletions test/browser/render.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1786,6 +1786,77 @@ describe('render()', () => {
);
});

// #2949
it('should not swap unkeyed chlildren', () => {
class X extends Component {
constructor(props) {
super(props);
this.name = props.name;
}
render() {
return <p>{this.name}</p>;
}
}

function Foo({ condition }) {
return (
<div>
{condition ? '' : <X name="A" />}
{condition ? <X name="B" /> : ''}
</div>
);
}

render(<Foo />, scratch);
expect(scratch.textContent).to.equal('A');

render(<Foo condition />, scratch);
expect(scratch.textContent).to.equal('B');

render(<Foo />, scratch);
expect(scratch.textContent).to.equal('A');
});

it('should retain state for inserted children', () => {
class X extends Component {
constructor(props) {
super(props);
this.name = props.name;
}
render() {
return <p>{this.name}</p>;
}
}

function Foo({ condition }) {
// We swap the prop from A to B but we don't expect this to
// reflect in text-content as we are testing whether the
// state is retained for a skew that matches the original children.
//
// We insert <span /> which should amount to a skew of -1 which should
// make us correctly match the X component.
return condition ? (
<div>
<span />
<X name="B" />
</div>
) : (
<div>
<X name="A" />
</div>
);
}

render(<Foo />, scratch);
expect(scratch.textContent).to.equal('A');

render(<Foo condition />, scratch);
expect(scratch.textContent).to.equal('A');

render(<Foo />, scratch);
expect(scratch.textContent).to.equal('A');
});

it('handle shuffled (stress test)', () => {
function randomize(arr) {
for (let i = arr.length - 1; i > 0; i--) {
Expand Down