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
[ssr] Fix hydration bug with attribute bindings on void elements #2952
Conversation
🦋 Changeset detectedLatest commit: a3a1294 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📊 Tachometer Benchmark ResultsSummarynop-update
render
update
update-reflect
Resultslit-element-list
render
update
update-reflect
lit-html-kitchen-sink
render
update
nop-update
lit-html-repeat
render
update
lit-html-template-heavy
render
update
reactive-element-list
render
update
update-reflect
|
render() { | ||
return html`<void-element-host></void-element-host>`; | ||
}, | ||
expectations: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No action necessary now, but it might be nice to have some check that this actually hydrated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added confirmation that we actually hydrated, by making sure if we change the property, the attribute updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
The alternate approach looks interesting but feels not worth it compared to this solution. Checking against a set of void elements would be useful if we're trying not to rely on the parser's hoisting behavior (is this stable behavior?) and place the marker as sibling ourselves to make it stable, but that also seems unnecessarily extra and breaking for the moment.
Background
In SSR, when an element has an attribute binding, we insert a
<!--lit-node 0-->
comment to tellhydrate()
about the attribute binding.However, if that element was a void element, such as
<input>
or anything from this list, then when the HTML parser encounters it, all of the children it has will be hoisted up as siblings.For example:
Is SSR'd as:
But parsed by the browser as:
This means when
hydrate()
encounters a case like this, we need to checkpreviousSibling
instead ofparentElement
, to find the element to receive the attribute binding.Bug
We already accounted for void elements where we actually create the attribute parts:
lit/packages/lit-html/src/experimental-hydrate.ts
Line 340 in ac35699
But we did not account for it where we remove the
defer-hydration
attribute:lit/packages/lit-html/src/experimental-hydrate.ts
Line 161 in ac35699
This actually only crashed
hydrate()
if the void element was an immediate child of a shadow root, because in every other case,element.parentElement
would return something, even if it wasn't the correct node.Fix
We now also account for void elements when we remove the
defer-hydration
attribute in the same way that we already did for creating attribute parts.Note as part of this I switched from
previousSibling
topreviousElementSibling
, so that we can assumeremoveAttribute
is defined. Seems like this should be safe?Hopefully fixes #2946
cc @daKmoR
Alternative idea
I also thought about an alternative way to handle void elements generally, which is to detect them up-front during SSR rendering, since they are a small fixed set of tag names, so that we get an explicit signal about whether we need to check the parent or the sibling during hydration, instead of doing the previousSibling -> parentElement fallback. See 66b702b for how that would look. This could potentially have slightly better performance during hydration, since it replaces a DOM call with a string check. Not sure it's worth it though, and I think it would be a breaking change because it changes the rendering scheme.