Avoid cloning fragments and arrays of VNodes (#2)

Author: skirtles-code

docs/api.md

@@ -18,9 +18,9 @@ Adds props to 'top-level' element and component VNodes in the passed array. Node
 
 The [`options`](#iterationoptions) object can be set to `{ component: true }` or `{ element: true }` to limit iteration to just components or elements respectively.
 
-Eligible VNodes will be passed to the provided callback. The callback should return an object containing props that need to be added to the VNode. The VNode itself will not be changed, it will be cloned using Vue's built-in `cloneVNode` helper.
+Eligible VNodes will be passed to the provided callback. The callback should return an object containing props that need to be added to the VNode. The VNode itself will not be changed, it will be cloned using Vue's built-in `cloneVNode()` helper. Any ancestor fragment nodes will be cloned as required.
 
-A new array of VNodes is returned.
+The passed array will not be modified, but if no changes were required then the same array may be returned.
 
 ### See also
 
@@ -53,7 +53,7 @@ If the callback returns `null` or `undefined` (or an empty array) then no change
 
 The exact position of the newly inserted nodes within the tree is an implementation detail and should not be relied upon. The current pair of nodes might be in different fragments, or they might already have other nodes between them that are being skipped by the `options`. No guarantees are made about the positions of the inserted nodes relative to other nodes, only that they will be somewhere between the pair passed to the callback.
 
-A new array will be returned and the passed array and its contents should be left unmodified. Any fragment nodes will be cloned as required to avoid mutating the input nodes. The returned array may contain some of the same nodes as the input array, as nodes are not cloned in cases where it can be avoided.
+The passed array and its contents will be left unmodified. Any fragment nodes will be cloned as required to avoid mutating the input nodes. The returned array may contain some of the same nodes as the input array, as nodes are not cloned in cases where it can be avoided. If no nodes are inserted then the original array may be returned.
 
 ### See also
 
@@ -399,7 +399,7 @@ The callback will be passed the VNodes in tree order. If any of the children are
 
 If the callback returns `null` or `undefined`, the current node will be left in its current position in the VNode tree. If the callback returns a single VNode, it will replace the original VNode in the tree. If the callback returns an array, all the VNodes in the array will be used to replace the current node. The current VNode can be included in the returned array, allowing for nodes to be added around the current node. An empty array can be used to remove the current VNode.
 
-A new array will be returned and the passed array and its contents should be left unmodified. Any fragment nodes will be cloned as required to avoid mutating the input nodes. The returned array may contain some of the same nodes of the input array, as nodes are not cloned in cases where it can be avoided.
+The passed array and its contents will be left unmodified. Any fragment nodes will be cloned as required to avoid mutating the input nodes. The returned array may contain some of the same nodes of the input array, as nodes are not cloned in cases where it can be avoided. If no changes are required then the original array may be returned.
 
 ### See also
 

src/__tests__/vue-vnode-utils.spec.ts

@@ -224,10 +224,12 @@ describe('addProps', () => {
     expect(child.type).toBe('div')
     expect(child.props?.class).toBe('red')
 
-    // TODO
-    // expect(startNodes.length).toBe(1)
-    // expect(startNodes[0]).toBe(startNode)
-    // expect(startNode.props).toBe(null)
+    expect(startNodes.length).toBe(1)
+    expect(startNodes[0]).toBe(fragNode)
+    expect(fragNode.props).toBe(null)
+    expect(fragNode.children?.length).toBe(1)
+    expect((fragNode.children as VNodeArrayChildren)[0]).toBe(divNode)
+    expect(divNode.props).toBe(null)
   })
 
   it('addProps - 3de9', () => {
@@ -380,14 +382,9 @@ describe('addProps', () => {
     compareChildren(nullNodes, referenceNodes)
     compareChildren(emptyNodes, referenceNodes)
 
-    expect(undefinedNodes[0]).toBe(startNodes[0])
-    expect(undefinedNodes[1]).toBe(startNodes[1])
-
-    expect(nullNodes[0]).toBe(startNodes[0])
-    expect(nullNodes[1]).toBe(startNodes[1])
-
-    expect(emptyNodes[0]).toBe(startNodes[0])
-    expect(emptyNodes[1]).toBe(startNodes[1])
+    expect(undefinedNodes).toBe(startNodes)
+    expect(nullNodes).toBe(startNodes)
+    expect(emptyNodes).toBe(startNodes)
   })
 
   it('addProps - a934', () => {
@@ -428,6 +425,33 @@ describe('addProps', () => {
     expect((nodes[0] as VNode).props?.class).toBe(undefined)
     expect((nodes[1] as VNode).props?.class).toBe('red')
   })
+
+  it('addProps - 510f', () => {
+    let count = 0
+
+    const spanNode = h('span')
+    const fragment = [spanNode]
+    const startNodes = [h('div'), fragment]
+
+    const nodes = addProps(startNodes, (vnode) => {
+      count++
+
+      if (vnode.type === 'div') {
+        return {
+          class: 'red'
+        }
+      }
+    })
+
+    expect(count).toBe(2)
+
+    expect(nodes.length).toBe(2)
+    expect((nodes[0] as VNode).props?.class).toBe('red')
+    expect(nodes[1]).toBe(fragment)
+    expect(fragment.length).toBe(1)
+    expect(fragment[0]).toBe(spanNode)
+    expect(spanNode.props).toBe(null)
+  })
 })
 
 describe('replaceChildren', () => {
@@ -450,9 +474,9 @@ describe('replaceChildren', () => {
     expect(count).toBe(1)
     expect(Array.isArray(nodes)).toBe(true)
     expect(nodes).toHaveLength(1)
+    expect(nodes).toBe(startNodes)
 
     compareChildren(startNodes, [h('div')])
-    compareChildren(nodes, [h('div')])
   })
 
   it('replaceChildren - 7c8a', () => {
@@ -476,6 +500,7 @@ describe('replaceChildren', () => {
     expect(nodes).toHaveLength(0)
 
     compareChildren(startNodes, [h('div')])
+    expect(startNodes[0]).toBe(startNode)
   })
 
   it('replaceChildren - 1d16', () => {
@@ -601,6 +626,37 @@ describe('replaceChildren', () => {
     compareChildren(startNodes, [h('div'), 'Text', [h('span'), 'More text']])
     compareChildren(nodes, [h('div'), '(Text)', [h('span'), '(More text)']])
   })
+
+  it('replaceChildren - e076', () => {
+    let count = 0
+
+    const startNodes = ['Text']
+
+    const nodes = replaceChildren(startNodes, () => {
+      count++
+    })
+
+    expect(count).toBe(1)
+    expect(Array.isArray(nodes)).toBe(true)
+    expect(nodes).toHaveLength(1)
+    expect(isVNode(nodes[0])).toBe(true)
+
+    expect(startNodes).toHaveLength(1)
+    expect(startNodes[0]).toBe('Text')
+
+    // Do the same thing with a text VNode
+    const startVNodes = [createTextVNode('Text')]
+
+    count = 0
+
+    const nodesOut = replaceChildren(startVNodes, () => {
+      count++
+    })
+
+    expect(count).toBe(1)
+    expect(nodesOut).toBe(startVNodes)
+    expect(nodesOut).toHaveLength(1)
+  })
 })
 
 describe('betweenChildren', () => {
@@ -615,17 +671,11 @@ describe('betweenChildren', () => {
     })
 
     expect(count).toBe(0)
-    expect(Array.isArray(nodes)).toBe(true)
-    expect(nodes.length).toBe(1)
-
-    const node = nodes[0] as VNode
+    expect(nodes).toBe(startNodes)
 
-    expect(isElement(node)).toBe(true)
-    expect(node.type).toBe('div')
-    expect(node.props).toBe(null)
-
-    expect(startNodes.length).toBe(1)
+    expect(startNodes).toHaveLength(1)
     expect(startNodes[0]).toBe(startNode)
+    expect(startNode.type).toBe('div')
     expect(startNode.props).toBe(null)
   })
 
@@ -649,10 +699,8 @@ describe('betweenChildren', () => {
     })
 
     expect(count).toBe(1)
-    expect(Array.isArray(nodes)).toBe(true)
-    expect(nodes.length).toBe(2)
+    expect(nodes).toBe(startNodes)
 
-    compareChildren(nodes, [h('div'), h('span')])
     compareChildren(startNodes, [h('div'), h('span')])
   })
 
@@ -677,10 +725,8 @@ describe('betweenChildren', () => {
     })
 
     expect(count).toBe(1)
-    expect(Array.isArray(nodes)).toBe(true)
-    expect(nodes.length).toBe(2)
+    expect(nodes).toBe(startNodes)
 
-    compareChildren(nodes, [h('div'), h('span')])
     compareChildren(startNodes, [h('div'), h('span')])
   })
 
@@ -1171,6 +1217,32 @@ describe('betweenChildren', () => {
       ]
     ])
   })
+
+  it('betweenChildren - 2bea', () => {
+    let count = 0
+
+    const startNodes = [['Text'], [createTextVNode('Text')]]
+
+    const nodes = betweenChildren(startNodes, (before, after) => {
+      count++
+
+      expect(isVNode(before)).toBe(true)
+      expect(isVNode(after)).toBe(true)
+
+      expect(getText(before)).toBe('Text')
+      expect(getText(after)).toBe('Text')
+    })
+
+    expect(count).toBe(1)
+
+    expect(nodes).toHaveLength(2)
+    expect(Array.isArray(nodes[0])).toBe(true)
+    expect(nodes[0]).toHaveLength(1)
+    expect(isVNode((nodes[0] as VNodeArrayChildren)[0])).toBe(true)
+    expect(nodes[1]).toBe(startNodes[1])
+
+    expect(startNodes[0][0]).toBe('Text')
+  })
 })
 
 describe('someChild', () => {

src/vue-vnode-utils.ts

@@ -164,18 +164,6 @@ const getFragmentChildren = (fragmentVNode: VNode | VNodeArrayChildren): VNodeAr
   return []
 }
 
-const setFragmentChildren = (fragment: VNode | VNodeArrayChildren, children: VNodeArrayChildren): (VNode | VNodeArrayChildren) => {
-  if (Array.isArray(fragment)) {
-    return children
-  }
-
-  const newNode = cloneVNode(fragment)
-
-  newNode.children = children
-
-  return newNode
-}
-
 export type IterationOptions = {
   element?: boolean
   component?: boolean
@@ -232,23 +220,7 @@ export const addProps = (
     checkArguments('addProps', [children, callback, options], ['array', 'function', 'object'])
   }
 
-  return children.map(child => addPropsToChild(child, callback, options))
-}
-
-const addPropsToChild = (
-  child: VNodeChild,
-  callback: (vnode: VNode) => (Record<string, unknown> | null | void),
-  options: IterationOptions
-): VNodeChild => {
-  if (isFragment(child)) {
-    const newChildren = addProps(getFragmentChildren(child), callback, options)
-
-    return setFragmentChildren(child, newChildren)
-  }
-
-  const vnode = promoteToVNode(child, options)
-
-  if (vnode) {
+  return replaceChildren(children, (vnode) => {
     const props = callback(vnode)
 
     if (DEV) {
@@ -262,9 +234,7 @@ const addPropsToChild = (
     if (props && !isEmptyObject(props)) {
       return cloneVNode(vnode, props)
     }
-  }
-
-  return child
+  }, options)
 }
 
 export const replaceChildren = (
@@ -276,13 +246,30 @@ export const replaceChildren = (
     checkArguments('replaceChildren', [children, callback, options], ['array', 'function', 'object'])
   }
 
-  const nc: VNodeArrayChildren = []
+  let nc: VNodeArrayChildren | null = null
+
+  for (let index = 0; index < children.length; ++index) {
+    const child = children[index]
 
-  for (const child of children) {
     if (isFragment(child)) {
-      const newChildren = replaceChildren(getFragmentChildren(child), callback, options)
+      const oldFragmentChildren = getFragmentChildren(child)
+      const newFragmentChildren = replaceChildren(oldFragmentChildren, callback, options)
+
+      let newChild: VNodeChild = child
+
+      if (oldFragmentChildren !== newFragmentChildren) {
+        nc ??= children.slice(0, index)
 
-      nc.push(setFragmentChildren(child, newChildren))
+        if (Array.isArray(child)) {
+          newChild = newFragmentChildren
+        } else {
+          newChild = cloneVNode(child)
+
+          newChild.children = newFragmentChildren
+        }
+      }
+
+      nc && nc.push(newChild)
     } else {
       const vnode = promoteToVNode(child, options)
 
@@ -297,18 +284,22 @@ export const replaceChildren = (
           }
         }
 
+        if (newNodes !== child) {
+          nc ??= children.slice(0, index)
+        }
+
         if (Array.isArray(newNodes)) {
-          nc.push(...newNodes)
+          nc && nc.push(...newNodes)
         } else {
-          nc.push(newNodes)
+          nc && nc.push(newNodes)
         }
       } else {
-        nc.push(child)
+        nc && nc.push(child)
       }
     }
   }
 
-  return nc
+  return nc ?? children
 }
 
 export const betweenChildren = (