Skip to content

[fix] hydration append issue #6602

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

Merged
merged 2 commits into from
Aug 4, 2021
Merged

[fix] hydration append issue #6602

merged 2 commits into from
Aug 4, 2021

Conversation

hbirler
Copy link
Contributor

@hbirler hbirler commented Jul 31, 2021

(Hopefully) Fixes #6561 (issue with keyed each block) and fixes 6463 (hydration duplicates elements in head)

Fix for issue #6561 is in the append_hydration function where an append operation was omitted in a case where node.parentNode === target but node.nextSibling !== null. The corresponding test is runtime/samples/key-block-post-hydrate.

Fix for issue #6463 is based on the comment #6463 (comment). The corresponding test is hydration/samples/head-html.

@Conduitry
Copy link
Member

Why is there a change for how <head> hydration is handled? What was wrong with the old one? Was the data-svelte attribute not correctly applied to the elements that ended up in the head? The new one with the marker comments looks like quite a bit more code.

@hbirler
Copy link
Contributor Author

hbirler commented Aug 3, 2021

In issue #6463, {@html} is used within <svelte:head> to add new nodes to the head. These nodes (and the HTML_TAG_* comment nodes) do not have the data-svelte attribute. I do not know how data-svelte attributes can be added to these nodes (is it okay to add attributes to nodes within {@html}?), so I changed the claim mechanism, since I am more familiar with that. I agree that it would be nicer to just add the data-svelte attribute to these nodes.

}
return result;
}

Copy link
Member

Choose a reason for hiding this comment

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

If you have multiple <svelte:head> elements, wouldn't this be selecting {@html} elements from another <svelte:head> element?

Copy link
Member

Choose a reason for hiding this comment

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

I imagine it would, yeah. This would also be addressed if the HtmlTag class (or, I guess, the HtmlTagHydration class) were responsible for inserting this attribute on the top level elements - since each component would only be removing elements matching its unique scoping class name.

@Conduitry
Copy link
Member

Could you update this to only include the fix for #6561 please? This seems like a more serious regression, and is something with a less controversial fix. We can take a look at the fix for #6463 (which appears to be a much longer-standing bug) in another PR, perhaps by updating how the elements are created in SSR. rather than how they are claimed.

@hbirler hbirler changed the title [fix] hydration & claim [fix] hydration append issue Aug 4, 2021
@Conduitry Conduitry merged commit 100561c into sveltejs:master Aug 4, 2021
@Conduitry
Copy link
Member

Thank you! I will cut a new release with this shortly.

@hbirler
Copy link
Contributor Author

hbirler commented Aug 4, 2021

Thank you for the feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Starting with v3.39.0 sorting a keyed each block messes up the DOM
3 participants