-
Notifications
You must be signed in to change notification settings - Fork 394
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
fix: remove fragment delimiters and update tests #3845
base: master
Are you sure you want to change the base?
Conversation
Makes sense. @jye-sf can you point me to those mentioned changes so I can catch up? |
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.
first pass. Note: this change greatly depends on how/when we generate things (especially fragments) on the compiler (which tbh, i don't remember well)
|
||
// Reorder | ||
elm.beforeItems = []; | ||
elm.afterItems = ['bar']; |
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.
shouldn't matter much, but this is changing the test. Since beforeItems
was previously assigned to foo
, we don't know for sure if the foo
rendered is a new or the one from beforeItems.
@@ -238,7 +238,7 @@ function patchFragment(n1: VFragment, n2: VFragment, parent: ParentNode, rendere | |||
} | |||
|
|||
// Note: not reusing n1.elm, because during patching, it may be patched with another text node. | |||
n2.elm = n2.leading.elm; | |||
n2.elm = n2.leading?.elm; |
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 there a way that the fragment can be empty?
If it could empty, then "leading" might be empty and there might be issues when computing the "anchor" (insert before/after) the fragment in update(Static/Dynamic)Children.
@@ -99,21 +99,34 @@ function st(fragment: Element, key: Key, parts?: VStaticPart[]): VStatic { | |||
|
|||
// [fr]agment node | |||
function fr(key: Key, children: VNodes, stable: 0 | 1): VFragment { | |||
const leading = t(''); |
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.
iirc, these served as a delimiter to updateDynamicChildren (as they will always match), and all insertions/deletions happen within that boundary. Can you think of a case in which you are (dynamically) updating (to add) the children of a fragment, which is in the middle of the elements of the parent? (ex: [div, FragmentWithChildren, div])
@@ -6,18 +6,14 @@ | |||
<x-child-with-for-each> | |||
<ul> | |||
<li> | |||
|
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.
iirc, there is a special handling on SSR and hydration logic to handle these. should we remove that?
Details
Since the initial implementation of VFragments, we've changed the way we reference elements from the vnode (
leading
,trailing
) and implemented VFragment-specific logic for handling slots and diffing. As a result, I'm not sure we still need the empty text nodes.This PR explores whether it's possible to remove the text delimiters and keep everything working. Although our integration tests all pass, I'm unfortunately not 100% confident they cover all possible use cases.
Can anyone else think of any use cases we may want to verify this against? If there are, I'd like to add them to our test suite. Otherwise, our current test suite seems to be telling us we're able to remove the text nodes with no regression.
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
Extra text nodes from VFragments (used for slots and if-elseif-else) are removed and no longer in the DOM.
GUS work item