Fix edge case where oldChild can become detach causing the recursive …

[arivyiii]

…function to end early

---

This change is 

[kode-toad]

Failed pendo.io/jira-ref check. Please correct Jira references in the following commits:

8436691

[megboehlert]

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @guntherjh and @juliecheng)

[guntherjh]

Fix seems fine. Any chance you can add test coverage?

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @juliecheng)

[arivyiii]

yeah i can add them in.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @juliecheng)

[kode-toad]

Failed pendo.io/jira-ref check. Please correct Jira references in the following commits:

e467a7e

[juliecheng]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @arivyiii)

---

packages/rrdom/test/diff.test.ts line 1188 at r2 (raw file):

    });

    it.only('should maintain correct order when duplicate node is found causing nextSibling traversal desync', () => {

nit this needs to be removed

[juliecheng]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @arivyiii)

---

packages/rrdom/test/diff.test.ts line 1188 at r2 (raw file):

Previously, juliecheng wrote…

nit this needs to be removed

by this i mean only :oops:

[kode-toad]

Failed pendo.io/jira-ref check. Please correct Jira references in the following commits:

466187e

[guntherjh]

Reviewed 1 of 3 files at r2.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @arivyiii and @juliecheng)

---

packages/rrdom/src/diff.ts line 537 at r3 (raw file):

    
      // Insert the new node before the current oldChild to preserve correct order.
      oldTree.insertBefore(newNode, oldChild);

The rest of this function uses handleInsertBefore wrapped in a try/catch. handleInsertBefore calls insertBefore under the hood, but I'm thinking we should follow the existing pattern and use handleInsertBefore wrapped in a try/catch as well.

On that note though, I'm still trying to wrap my head around all that diffChildren and the rest of the diff* functions actually do. I'm starting to think we want this type of check higher up and not part of the recursive while loop 🤔

[arivyiii]

packages/rrdom/src/diff.ts line 537 at r3 (raw file):

Previously, guntherjh (John Henry Gunther) wrote…

The rest of this function uses handleInsertBefore wrapped in a try/catch. handleInsertBefore calls insertBefore under the hood, but I'm thinking we should follow the existing pattern and use handleInsertBefore wrapped in a try/catch as well.

On that note though, I'm still trying to wrap my head around all that diffChildren and the rest of the diff* functions actually do. I'm starting to think we want this type of check higher up and not part of the recursive while loop 🤔

I can try to do my best as least with how the diff and diffChildren is suppose to work.

Diff at a high level is responsible for comparing the nodes using the newTree as the source of truth which represents the virtual DOM. If oldTree and newTree are out of sync the goal is for oldTree to resemble newTree by the end of the diff call. This includes making sure properties and attributes are properly applied to the real DOM.

It can be broken down into three levels of responsibility but I'll focus on the main two since they're related to this bug.

diffBeforeUpdatingChildren is responsible for making sure the current nodes line up. If they do not it creates a new node from newTree and replaces oldTree with it in the DOM.

This is where the issue is occuring for this bug. After replacing the node the original reference to oldTree which was passed by reference becomes outdated. The parent that called diff still holds onto the original oldTree reference which has now been replaced and is no longer part of the DOM. This causes problems because the node becomes detached and no longer has a parentNode or nextSibling. That could potentially break traversal in diffChildren if diff is called from within that function.

diffChildren this is the big one but it's main job is to recursively diff all the children and ensure that all of their nodes line up. the first half of the function does a pretty good job at handling this by comparing the first and last and iterating inwards until it's done comparing, along the process when it finds a diff it handles it by removing the old node and insert a node where the oldNode used to be, which looks to be a much safer operation than the replaceChild one that gets used in the diffBeforeUpdatingChildren.

After it finishes aligning the nodes it will recursively iterate through each of the old nodes and new nodes. This is the part where properties and attributes get attached to the child nodes.

Traversal stops here as soon as either oldChild or newChild becomes null. So even if there is more data left in newChild it assumes at this point that the nodes are aligned and it is safe to bail out of the loop

[guntherjh]

Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @arivyiii and @juliecheng)

---

packages/rrdom/src/diff.ts line 537 at r3 (raw file):

Previously, arivyiii (Allen Ivy) wrote…

I can try to do my best as least with how the diff and diffChildren is suppose to work.

Diff at a high level is responsible for comparing the nodes using the newTree as the source of truth which represents the virtual DOM. If oldTree and newTree are out of sync the goal is for oldTree to resemble newTree by the end of the diff call. This includes making sure properties and attributes are properly applied to the real DOM.

It can be broken down into three levels of responsibility but I'll focus on the main two since they're related to this bug.

diffBeforeUpdatingChildren is responsible for making sure the current nodes line up. If they do not it creates a new node from newTree and replaces oldTree with it in the DOM.

This is where the issue is occuring for this bug. After replacing the node the original reference to oldTree which was passed by reference becomes outdated. The parent that called diff still holds onto the original oldTree reference which has now been replaced and is no longer part of the DOM. This causes problems because the node becomes detached and no longer has a parentNode or nextSibling. That could potentially break traversal in diffChildren if diff is called from within that function.

diffChildren this is the big one but it's main job is to recursively diff all the children and ensure that all of their nodes line up. the first half of the function does a pretty good job at handling this by comparing the first and last and iterating inwards until it's done comparing, along the process when it finds a diff it handles it by removing the old node and insert a node where the oldNode used to be, which looks to be a much safer operation than the replaceChild one that gets used in the diffBeforeUpdatingChildren.

After it finishes aligning the nodes it will recursively iterate through each of the old nodes and new nodes. This is the part where properties and attributes get attached to the child nodes.

Traversal stops here as soon as either oldChild or newChild becomes null. So even if there is more data left in newChild it assumes at this point that the nodes are aligned and it is safe to bail out of the loop

My lack of understanding here is definitely a "me" problem, but I really appreciate the explanation (it helps a ton!). My biggest worry was around how these changes would impact time complexity, but looking a bit closer, I don't think it would have much (if any) impact since the mismatch case continues.

I'm going to keep poking around in here for my own understanding. I'm also seeing some new test failures in diff.test.ts that aren't present in pendo-main. Note I am seeing can build from a html containing nested shadow doms also fail in upstream so I wouldn't worry about that one.

---

packages/rrdom/test/diff.test.ts line 1250 at r3 (raw file):

        expect(finalIds).toEqual(oldList.map(n => String(n.id)));
      });
      

I'd remove these extra spaces