fix(engine-core): add abstraction for moving vfragments

[ekashida]

Details

Implements an abstraction for moving vfragment-based nodes around and fixes both reported issues around insertion when the vfragment-based nodes are the reference node. Note that we already had logic for mounting/patching/removing.

Fixes #3396
Fixes #3377

Does this pull request introduce a breaking change?

No

Does this pull request introduce an observable change?

Yes

GUS work item

W-12630370

[ekashida]

Downstream repo at salesforce-experience-platform-emu/lds-lightning-platform currently has a broken build.

[jye-sf]

Requested to add or verify we have tests for a specific use case.

[nolanlawson]

It seems strange to me that the diffing algo would have knowledge of fragment start and end text positions. My understanding is that the goal of fragments was to insert the text nodes and just let the diffing algo act accordingly.

Looking at how Vue does it, they seem to insert both the leading and trailing text nodes and then patch the content in before the trailing one. Can we use a similar technique?

[jye-sf]

@nolanlawson Patching the content in before the trailing anchor already works fine, and we're doing it the way you're suggesting (the way Vue does it). The problem here is not with patching/mounting the internal content of the fragment itself, it's with how the diffing algo deals with the VFragment node along with its sibling nodes. It's the difference between patch(fragmentNode) and patch(VNode{ children: [node, node, fragment, node] }). The first works fine. The latter is what we're trying to solve.

Thanks for linking how Vue does vdom. From what I can see, Vue's diffing algo actually does have knowledge of fragment start and end node positions:

• They always hold a reference to both the leading and trailing node.
• Their diffing algo is then designed around that base functionality. For operations that need to go before a VNode, they just use vnode.elm, which will always reference the leading element of the node. For operations that need to go after a VNode, they abstract that into its own function that specifically targets the trailing anchor and not the leading .elm.

This hits a good middle ground and I agree we should use a similar technique: fragment-specific start/end nodes are abstracted away into the base VNode definition itself (vnode.elm always leading, vnode.anchor for fragment tails, helper function to grab the next node element) and then the diffing algo just does its thing with minimal fragment-specific logic. I think we're being misguided trying to stick to a single-element-per-vnode approach.

cc - @ekashida

What do either of you think?

[jye-sf]

#3405 (comment)

[nolanlawson]

@jye-sf Nice research! Yeah that makes sense to me. If VFragment can't just be "any old VNode," then let's go whole-hog and keep a reference to its start and end anchors explicitly.

[ekashida]

That should be ‘Node | VFragment’

…

On Mon, Mar 27, 2023 at 12:21 Nolan Lawson ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In ***@***.***/engine-core/src/framework/rendering.ts <#3405 (comment)>: > + if (isVFragment(elmToMove)) { + insertFragment(elmToMove, parent, oldStartVnode.elm!, renderer); + } else { + insertNode(elmToMove.elm!, parent, oldStartVnode.elm!, renderer); + } I took a stab at it, and I think it's possible with just an isVFragment() check? Unless I'm missing the issue with this approach: 0a18c80 <0a18c80> — Reply to this email directly, view it on GitHub <#3405 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABAW6O6XGCQRBUXOLIMXI3W6HSCVANCNFSM6AAAAAAWBJALDA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[nolanlawson]

That should be ‘Node | VFragment’

I tested 0a18c80, and it compiles with TypeScript and passes the Karma tests. It looks like VNode (not Node) is correct there?

[ekashida]

@nolanlawson Ah, I see what you mean.

[nolanlawson]

One small nitpick, otherwise LGTM!

[ekashida]

@jye-sf another review please!

[jye-sf]

Minor nit: we could save a check here if trailing was on VNode rather than Vfragment because we can do

anchor = renderer.nextSibling(oldEndVnode.trailing ? oldEndVnode.trailing.elm : oldEndVnode.elm)

but I don't think we need to hold up this PR for that change. We can consider that when refactoring.