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.

Sign up for GitHub

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

Jump to bottom

remove _nextDom resetting as it collides with nested fragment switching #3738

Merged
merged 1 commit into from Sep 29, 2022
Merged

remove _nextDom resetting as it collides with nested fragment switching #3738

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 0 additions & 11 deletions src/diff/children.js
View file
Open in desktop
Expand Up @@ -205,17 +205,6 @@ export function diffChildren(
// Remove remaining oldChildren if there are any.
for (i = oldChildrenLength; i--; ) {
if (oldChildren[i] != null) {
if (
typeof newParentVNode.type == 'function' &&
oldChildren[i]._dom != null &&
oldChildren[i]._dom == newParentVNode._nextDom
) {
// 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
newParentVNode._nextDom = getDomSibling(oldParentVNode, i + 1);
}

unmount(oldChildren[i], oldChildren[i]);
}
}
Expand Down
78 changes: 73 additions & 5 deletions test/browser/fragments.test.js
View file
Open in desktop
Expand Up @@ -237,7 +237,9 @@ describe('Fragment', () => {
expectDomLogToBe([
'<div>.appendChild(#text)',
'<div>122.insertBefore(<div>1, <span>1)',
'<span>1.remove()'
'<span>1.remove()',
'<div>122.appendChild(<span>2)',
Copy link
Member

@developit developit Sep 19, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These seem incorrect. We're re-appending the spans even though they're already in the correct position.

The extra insertions is likely what is causing the slowdown in 03_update10th1k_x16.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, this was a performance optimization but it breaks unexpectedly. I can look into building some offset based calculator but generally this optimization is impossible to get right because of several reasons

  • we can have nullish children
  • we can have empty fragments as children

both these conditions increment the i counter meaning we will start looking for a dom-sibling in an erroneous position, hence the optimization fails in the issue-case and multiple others 😅 also this does not account for moved nodes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also do read that the DOM is still correct we just can't prevent these dupe insertions when moving

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, that this seems to point at a shortcoming of the current reconciler algo.

'<div>122.appendChild(<span>2)'
]);
});

Expand Down Expand Up @@ -2658,7 +2660,7 @@ describe('Fragment', () => {
render(<App condition={false} />, scratch);

expect(scratch.innerHTML).to.equal(div([div(1), div('A')]));
expectDomLogToBe(['<div>2.remove()']);
expectDomLogToBe(['<div>2.remove()', '<div>1A.appendChild(<div>A)']);
});

it('should efficiently unmount nested Fragment children', () => {
Expand Down Expand Up @@ -2702,7 +2704,12 @@ describe('Fragment', () => {
render(<App condition={false} />, scratch);

expect(scratch.innerHTML).to.equal(div([div(1), div('A'), div('B')]));
expectDomLogToBe(['<div>2.remove()', '<div>3.remove()']);
expectDomLogToBe([
'<div>2.remove()',
'<div>3.remove()',
'<div>1AB.appendChild(<div>A)',
'<div>1BA.appendChild(<div>B)'
]);
});

it('should efficiently place new children and unmount nested Fragment children', () => {
Expand Down Expand Up @@ -2751,7 +2758,9 @@ describe('Fragment', () => {
'<div>.appendChild(#text)',
'<div>123AB.insertBefore(<div>4, <div>2)',
'<div>2.remove()',
'<div>3.remove()'
'<div>3.remove()',
'<div>14AB.appendChild(<div>A)',
'<div>14BA.appendChild(<div>B)'
]);
});

Expand Down Expand Up @@ -2799,7 +2808,66 @@ describe('Fragment', () => {
'<div>123AB.insertBefore(<span>1, <div>1)',
'<div>2.remove()',
'<div>3.remove()',
'<div>1.remove()'
'<div>1.remove()',
'<div>1AB.appendChild(<div>A)',
'<div>1BA.appendChild(<div>B)'
]);
});

it('should swap nested fragments correctly', () => {
let swap;
class App extends Component {
constructor(props) {
super(props);
this.state = { first: true };
}

render() {
if (this.state.first) {
return (
<Fragment>
{
<Fragment>
<p>1. Original item first paragraph</p>
</Fragment>
}
<p>2. Original item second paragraph</p>
<button onClick={(swap = () => this.setState({ first: false }))}>
Click me
</button>
</Fragment>
);
}
return (
<Fragment>
<p>1. Second item first paragraph</p>
<Fragment>
<p>2. Second item second paragraph</p>
<div />
</Fragment>
<button onClick={(swap = () => this.setState({ first: true }))}>
Click me
</button>
</Fragment>
);
}
}

render(<App />, scratch);
expect(scratch.innerHTML).to.equal(
'<p>1. Original item first paragraph</p><p>2. Original item second paragraph</p><button>Click me</button>'
);

swap();
rerender();
expect(scratch.innerHTML).to.equal(
'<p>1. Second item first paragraph</p><p>2. Second item second paragraph</p><div></div><button>Click me</button>'
);

swap();
rerender();
expect(scratch.innerHTML).to.equal(
'<p>1. Original item first paragraph</p><p>2. Original item second paragraph</p><button>Click me</button>'
);
});
});