Nested fragment nodes and correct implementation of mounDOM #237

sabatavdgiridze · 2024-03-10T07:59:45Z

sabatavdgiridze
Mar 10, 2024

Hi,
I have been trying to understand the source code for the version 2 of the framework. There we try to implement reconciliation algorithm and for that we have added some changes to the mountDOM function - namely, now it accepts the index at the parent element to mount the virtual DOM node to. I could not understand why this function is correct:

function createFragmentNodes(vdom, parentEl, index) {
  const { children } = vdom
  vdom.el = parentEl

  children.forEach((child, i) =>
    mountDOM(child, parentEl, index ? index + i : null)
  )
}

Here if we have the nested fragment nodes inside one another, then we have to change this line:

mountDOM(child, parentEl, index ? index + i : null)

I think the final function would look like this:

function createFragmentNodes(vdom, parentEl, index) {
  const { children } = vdom
  vdom.el = parentEl
  
  let index_count = 0
  children.forEach((child, i) => {
    mountDOM(child, parentEl, index ? index_count : null);
    index_count += child_count(child);
  }
  )
}
// pseudo-code
function child_count(vdom_element) {
  if (vdom.type == fragment) {
    array_sum(vdom.children.map((child) => child_count(child)));
  }
  return 1;
}

Could anyone tell me what mistake am I making here?

angelsolaorbaiceta · 2024-03-10T11:30:42Z

angelsolaorbaiceta
Mar 10, 2024
Maintainer

Hi there!

This is a very good question, and indeed, that case is hard to comprehend.

Exercise

I suggest you do the following exercise: add a console log inside the createFragmentNodes() function like so:

  function createFragmentNodes(vdom, parentEl, index, hostComponent) {
    const { children } = vdom
    vdom.el = parentEl

    children.forEach((child, i) => {
++    console.log(  'Mounting child', child, 'at index', index ? index + i : null)
      mountDOM(child, parentEl, index ? index + i : null, hostComponent)
    })
  }

Then, modify the <body> so that it contains the following:

<body>
  <p>one</p>
  <p>two</p>
  <p>six</p>
</body>

Now run the following code to mount a fragment that contains a fragment inside of itself:

const vdom = hFragment([
  h('p', {}, ['three']),
  hFragment([h('p', {}, ['four']), h('p', {}, ['five'])]),
])

mountDOM(vdom, document.body, 2)

You'll see the nodes are correctly mounted:

<body>
  <p>one</p>
  <p>two</p>
  <p>three</p>
  <p>four</p>
  <p>five</p>
  <p>six</p>
</body>

And in the console you'll see:

Mounting child {
  tag: 'p',
  props: {},
  type: 'element',
  children: [ { type: 'text', value: 'three' } ]
} at index 2

Mounting child {
  type: 'fragment',
  children: [
    { tag: 'p', props: {}, type: 'element', children: [Array] },
    { tag: 'p', props: {}, type: 'element', children: [Array] }
  ]
} at index 3

Mounting child {
  tag: 'p',
  props: {},
  type: 'element',
  children: [ { type: 'text', value: 'four' } ]
} at index 3

Mounting child {
  tag: 'p',
  props: {},
  type: 'element',
  children: [ { type: 'text', value: 'five' } ]
} at index 4

Explanation

The first call mounts the three. Nothing too relevant for this exercise.

Mounting child {
  tag: 'p',
  props: {},
  type: 'element',
  children: [ { type: 'text', value: 'three' } ]
} at index 2

Then comes the internal fragment:

Mounting child {
  type: 'fragment',
  children: [
    { tag: 'p', props: {}, type: 'element', children: [Array] },
    { tag: 'p', props: {}, type: 'element', children: [Array] }
  ]
} at index 3

Here's the key to understanding the result: this call to the mount function doesn't mount anything yet, as we have a fragment, we call the mountDOM() function again with each children.

So we're inside the nested fragment now, and it's time for the four.
This element is mounted at index 3 (as expected).
See how the index is the same in this step as in the previous one, because in the previous step nothing was really appended to the DOM.

Mounting child {
  tag: 'p',
  props: {},
  type: 'element',
  children: [ { type: 'text', value: 'four' } ]
} at index 3

And last goes five, at index 4 (this time the index is incremented, as expected).

Mounting child {
  tag: 'p',
  props: {},
  type: 'element',
  children: [ { type: 'text', value: 'five' } ]
} at index 4

So, the result is:

three is mounted at index = 2
four is mounted at index = 3
five is mounted at index = 4

I hope this helps.
I do recommend you run this in the browser, set breakpoints and convince yourself that the index where the elements are inserted is only incremented when it should.

Having said that, there might be something that I'm doing wrong myself.
If you do believe so, please be so kind as to provide me with a snippet that illustrates where my error is and I'll be more than glad to look at it and learn from it.

Best,
Angel

0 replies

sabatavdgiridze · 2024-03-10T14:43:43Z

sabatavdgiridze
Mar 10, 2024
Author

Hi!
Firstly, thank you for such a thorough reply it was really helpful! However, there remain some problems for me regarding the above-mentioned code snippet:

function createFragmentNodes(vdom, parentEl, index) {
  const { children } = vdom
  vdom.el = parentEl

  children.forEach((child, i) =>
    mountDOM(child, parentEl, index ? index + i : null)
  )
}

Let's consider the following example:

  const vdom = hFragment([
    h("p", {}, ["three"]),
    hFragment([
      h("p", {}, ["four"]),
      h("p", {}, ["five"])
    ])
  ])
  mountDOM(vdom, document.body, 0);

<body>
  <p>one</p>
  <p>two</p>
  <p>six</p>
</body>

Here, as the index argument is 0 for the mountDom function, in this line:

mountDOM(child, parentEl, index ? index + i : null)

For its children the index becomes null and we get the following result for the HTML:

<body>
  <p>one</p>
  <p>two</p>
  <p>six</p>
  <p>three</p>
  <p>four</p>
  <p>five</p>
</body>

This change:

mountDOM(child, parentEl, (index !== null) ? index + i : null)

corrects it and we get:

<body>
  <p>three</p>
  <p>four</p>
  <p>five</p>
  <p>one</p>
  <p>two</p>
  <p>six</p>
</body>

0 replies

sabatavdgiridze · 2024-03-10T15:25:52Z

sabatavdgiridze
Mar 10, 2024
Author

Also, let's see another example

const vdom = hFragment([
  hFragment([
    h("p", {}, ["three"]),
    h("p", {}, ["four"])
  ]),
  h("p", {}, ["five"])
])

mountDOM(vdom, document.body, 2);

Here, we have the nested fragment nodes in the first slot (that is difference with your example)

We get the following result:

<body>
  <p>one</p>
  <p>two</p>
  <p>three</p> 
  <p>five</p>  // those two are swapped
  <p>four</p> // ----^
  <p>six</p>
</body>

I changed the createFragmentNodes function in this way and it corrected the result:

function createFragmentNodes(vdom, parentEl, index) {

  const { children } = vdom;
  vdom.el = parentEl;
  
  let index_count = index;
  if (index === null) {
    children.forEach((child, i) => mountDOM(child, parentEl, null));
  } else {
    for (let idx = 0; idx < children.length; idx++) {
      console.log(index_count);
      mountDOM(children[idx], parentEl, index_count);
      if (children[idx].type == DOM_TYPES.FRAGMENT) {
        index_count += extractChildren(children[idx]).length;
      } else {
        index_count += 1;
      }
    }
  }
}

The idea is expressed in the following picture below:

1 reply

angelsolaorbaiceta Mar 10, 2024
Maintainer

You are completely right! Thanks to your very detailed explanation and your awesome diagram (kudos for the effort), I was able to understand the problem and come up with a solution.

Pull request #238 fixes this issue.

In essence, the logic that increments the index variable changes depending on the type of node.
Here's the fix:

function createFragmentNodes(vdom, parentEl, index, hostComponent) {
  const { children } = vdom
  vdom.el = parentEl

  for (const child of children) {
    mountDOM(child, parentEl, index, hostComponent)

    if (index == null) {
      continue
    }

    switch (child.type) {
      case DOM_TYPES.FRAGMENT:
        index += child.children.length
        break
      case DOM_TYPES.COMPONENT:
        index += child.component.elements.length
        break
      default:
        index++
    }
  }
}

Note

The component DOM type might be new to you if you haven't reached the chapter where stateful components are introduced.
In that case, only the fragment and the default cases are relevant for you as of now.

I added tests to cover all the possible cases (up to two levels of nested fragments) so this is covered.

Thank you very much for bringing this up, and for the effort you made in explaining the problem.
Let me know your thoughts and if you spot something else in the new logic which doesn't add up.

Best,
Angel

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Nested fragment nodes and correct implementation of mounDOM #237

Uh oh!

{{title}}

Uh oh!

Replies: 3 comments 1 reply

Uh oh!

{{title}}

Uh oh!

Uh oh!

{{title}}

Uh oh!

Uh oh!

{{title}}

Uh oh!

Uh oh!

{{title}}

Uh oh!

Uh oh!

{{editor}}'s edit

{{editor}}'s edit

Uh oh!

Select a reply

Uh oh!

Nested fragment nodes and correct implementation of mounDOM #237

Uh oh!

sabatavdgiridze Mar 10, 2024

Replies: 3 comments · 1 reply

Uh oh!

angelsolaorbaiceta Mar 10, 2024 Maintainer

Exercise

Explanation

Uh oh!

sabatavdgiridze Mar 10, 2024 Author

Uh oh!

sabatavdgiridze Mar 10, 2024 Author

Uh oh!

Uh oh!

angelsolaorbaiceta Mar 10, 2024 Maintainer

sabatavdgiridze
Mar 10, 2024

Replies: 3 comments 1 reply

angelsolaorbaiceta
Mar 10, 2024
Maintainer

sabatavdgiridze
Mar 10, 2024
Author

sabatavdgiridze
Mar 10, 2024
Author

angelsolaorbaiceta Mar 10, 2024
Maintainer