-
Notifications
You must be signed in to change notification settings - Fork 393
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
chore: use comment nodes for VFragment bookends #3846
Conversation
cbaa660
to
5e31425
Compare
FYI for anyone wondering: CI is failing due to an unrelated issue. There's an incompatibility between our version of |
packages/@lwc/integration-karma/test/light-dom/scoped-slot/if-block/index.spec.js
Show resolved
Hide resolved
packages/@lwc/engine-server/src/__tests__/fixtures/scoped-slots/basic/expected.html
Show resolved
Hide resolved
packages/@lwc/integration-karma/test/api/CustomElementConstructor-getter/index.spec.js
Show resolved
Hide resolved
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.
Is it confirmed we're releasing this as 5.0.0?
I think a v5.0 will be necessary, since there are breaking changes here. I'm not sure there are any downstreams that have consumed v4.0 that will be effected. But it seems like the "correct" thing to do. Since our internal 248 release is still in the hardening phase, we can still use the corresponding API version number (60) and associate it with v5.0. |
I don't like it but I also can't think of an alternative right now. This might be just an edge case, but I wonder in the future if it might make sense to hold off on releasing the next OSS major release externally until our internal FF, in case we have further bugfixes that need to be released through that release's API version.
Might be worth discussing at release planning. Thanks for getting this fix out so quickly! |
FWIW, this PR looks good to me. LWC v5.0.0 and API versioning seem like the pragmatic choice. The only additional information I would love to have is:
FWIW, for #1: |
For #2, I ran a subset of the benchmarks locally as a sanity check. All the tests timed out after 15 minutes, unable to determine a statistically significant change. I might run the full suite over night, at some point, to see if any perf regressions sneaked through. For #1, I was aware of Vue's use of comment nodes in SSR, but quite unaware of Svelte's complete non-use of bookends. That's interesting, and possibly worth investigating further. This was something of a hot issue, so I didn't dig into alternatives as deeply as I might have otherwise. I'd love to understand what other frameworks are doing, too. |
FWIW, I ran all the benchmarks last night, 200 samples with 15 min timeout. No difference except 1% on the hydration benchmark. So no big deal! click to see
|
Details
Prior to this PR, empty text nodes were placed at each end of a VFragment. This allows for efficient insertion and removal of elements relative to the fragment, which may have sibling nodes not belonging to the fragment. These text nodes are essential bookends for fragments like those found in scoped slots or
lwc:if
.When rendered on the server, these empty text nodes are rendered using a zero-width space character. This ensures that a text node is actually present after HTML is parsed by the browser. For example,
<div /><div/>
will be parsed by the browser (post-SSR, pre-hydration) as two<div>
s, even if an empty text node was present in between those<div>
s in the VDOM.This introduces two problems:
inline
orinline-block
contexts.The solution introduced here is to use comment nodes in place of empty text nodes. This clutters the Elements panel in the dev console somewhat, with seemingly extraneous comment nodes. However, this fix results in no performance degradation. HTML will be gzipped in transit, so the multiple comment nodes shouldn't impact network transit size. And we need not introduce special handling to the diffing algo that could easily result in a performance hit.
Does this pull request introduce a breaking change?
See description of the breaking change in #3649.
Does this pull request introduce an observable change?
Comment nodes will be present in the DOM where they were not before. Additionally, this involves a compiler change that will require an LWC minor-version bump.
GUS work item
W-14408593