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

Fix broken order of nodes during hydration #6283

Closed
wants to merge 3 commits into from
Closed

Fix broken order of nodes during hydration #6283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3d05e18
fix broken order of nodes during hydration
haveyaseen May 2, 2021
78564db
add test for hydration of nested element with expression sibling
haveyaseen May 2, 2021
95235f2
add test for hydration of nested element with if-block element sibling
haveyaseen May 2, 2021
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
4 changes: 2 additions & 2 deletions src/runtime/internal/dom.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export function append(target: Node, node: Node) {
if (is_hydrating) {
nodes_to_detach.delete(node);
}
if (node.parentNode !== target) {
if (node.parentNode !== target || node.parentNode !== null) {
Copy link
Member

Choose a reason for hiding this comment

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

hmm..reading from face value, this doesn't seems like a correct fix, there's maybe something wrong earlier in the hydration process / rather in the append method.

adding || node.parent !== null seems to defeat the purpose of doing node.parentNode !== target check

Copy link
Member

@benmccann benmccann May 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, that would defeat the check and so I agree the code shouldn't be written this way. It's also worth noting though that the check was introduced along with the bug in #6204 so we may not need to keep the check as is. If you remove this check and the one in insert to go back to how the code was originally then the tests will still pass. I don't think that either change would be the correct change though. The tests all pass with either change, which probably shows a shortcoming in the tests

Copy link
Member

Choose a reason for hiding this comment

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

the alternate of this is

if (node.parentNode !== target || node.nextSibling !== null) {

taking care the case where node parent is already correct, but the order is wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In insert the following passes as well:

if (
	node.parentNode !== target ||
	(!anchor && node.nextSibling !== null) ||
	(anchor && node.nextSibling !== anchor)
) {

Definitely looks much more sensible than my initial attempt, although I don't know there still are other edge cases not caught by the tests. Would there be any good way to test the idempotence property more directly?

target.appendChild(node);
}
}
Expand All @@ -31,7 +31,7 @@ export function insert(target: Node, node: Node, anchor?: Node) {
if (is_hydrating) {
nodes_to_detach.delete(node);
}
if (node.parentNode !== target || (anchor && node.nextSibling !== anchor)) {
if (node.parentNode !== target || node.parentNode !== null || (anchor && node.nextSibling !== anchor)) {
target.insertBefore(node, anchor || null);
}
}
Expand Down
4 changes: 4 additions & 0 deletions test/hydration/samples/element-nested-sibling/_after.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<p>
<span>1</span>
<code>2</code>
</p>
4 changes: 4 additions & 0 deletions test/hydration/samples/element-nested-sibling/_before.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<p>
<span>1</span>
<code>2</code>
</p>
19 changes: 19 additions & 0 deletions test/hydration/samples/element-nested-sibling/_config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
export default {
snapshot(target) {
const p = target.querySelector('p');

return {
p,
span: p.querySelector('span'),
code: p.querySelector('code')
};
},

test(assert, target, snapshot) {
const p = target.querySelector('p');

assert.equal(p, snapshot.p);
assert.equal(p.querySelector('span'), snapshot.span);
assert.equal(p.querySelector('code'), snapshot.code);
}
};
6 changes: 6 additions & 0 deletions test/hydration/samples/element-nested-sibling/main.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<p>
<span>1</span>
{#if true}
<code>2</code>
{/if}
</p>
1 change: 1 addition & 0 deletions test/hydration/samples/expression-sibling/_after.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<p>1 2 <span>3</span></p>
1 change: 1 addition & 0 deletions test/hydration/samples/expression-sibling/_before.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<p>1 2 <span>3</span></p>
19 changes: 19 additions & 0 deletions test/hydration/samples/expression-sibling/_config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
export default {
snapshot(target) {
const p = target.querySelector('p');

return {
p,
text: p.childNodes[0],
span: p.querySelector('span')
};
},

test(assert, target, snapshot) {
const p = target.querySelector('p');

assert.equal(p, snapshot.p);
assert.equal(p.childNodes[0], snapshot.text);
assert.equal(p.querySelector('span'), snapshot.span);
}
};
1 change: 1 addition & 0 deletions test/hydration/samples/expression-sibling/main.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<p>{1} 2 <span>3</span></p>