chore: change vSiblings from array to map

Varixo · Varixo · commit 8da28840d599 · 2025-03-13T19:14:50.000+01:00
diff --git a/packages/qwik/src/core/client/vnode-diff.ts b/packages/qwik/src/core/client/vnode-diff.ts
@@ -133,6 +133,12 @@ export const vnode_diff = (
   /// and is not connected to the tree.
   let vNewNode: VNode | null = null;
 
+  /// When elements have keys they can be consumed out of order and therefore we can't use nextSibling.
+  /// In such a case this array will contain the elements after the current location.
+  /// The array even indices will contains keys and odd indices the vNode.
+  let vSiblings: Map<string, VNode> | null = null;
+  let vSiblingsArray: Array<string | VNode | null> | null = null;
+
   /// Current set of JSX children.
   let jsxChildren: JSXChildren[] = null!;
   // Current JSX child.
@@ -315,6 +321,8 @@ export const vnode_diff = (
     stackPush(children, descendVNode);
     if (descendVNode) {
       assertDefined(vCurrent || vNewNode, 'Expecting vCurrent to be defined.');
+      vSiblings = null;
+      vSiblingsArray = null;
       vParent = vNewNode || vCurrent!;
       vCurrent = vnode_getFirstChild(vParent);
       vNewNode = null;
@@ -325,6 +333,8 @@ export const vnode_diff = (
   function ascend() {
     const descendVNode = stack.pop(); // boolean: descendVNode
     if (descendVNode) {
+      vSiblings = stack.pop();
+      vSiblingsArray = stack.pop();
       vNewNode = stack.pop();
       vCurrent = stack.pop();
       vParent = stack.pop();
@@ -339,7 +349,7 @@ export const vnode_diff = (
   function stackPush(children: JSXChildren, descendVNode: boolean) {
     stack.push(jsxChildren, jsxIdx, jsxCount, jsxValue);
     if (descendVNode) {
-      stack.push(vParent, vCurrent, vNewNode);
+      stack.push(vParent, vCurrent, vNewNode, vSiblingsArray, vSiblings);
     }
     stack.push(descendVNode);
     if (Array.isArray(children)) {
@@ -820,7 +830,12 @@ export const vnode_diff = (
         value = trackSignalAndAssignHost(value, vnode, key, container, signalData);
       }
 
-      vnode_setAttr(journal, vnode, key, serializeAttribute(key, value, scopedStyleIdPrefix));
+      vnode_setAttr(
+        journal,
+        vnode,
+        key,
+        value !== null ? serializeAttribute(key, value, scopedStyleIdPrefix) : null
+      );
       if (value === null) {
         // if we set `null` than attribute was removed and we need to shorten the dstLength
         dstLength = dstAttrs.length;
@@ -926,21 +941,59 @@ export const vnode_diff = (
     }
   }
 
-  /** Retrieve the child with the given key. */
+  /**
+   * This function is used to retrieve the child with the given key. If the child is not found, it
+   * will return null.
+   *
+   * After finding the first child with the given key we will create a map of all the keyed siblings
+   * and an array of non-keyed siblings. This is done to optimize the search for the next child with
+   * the specified key.
+   *
+   * @param nodeName - The name of the node.
+   * @param key - The key of the node.
+   * @returns The child with the given key or null if not found.
+   */
   function retrieveChildWithKey(
     nodeName: string | null,
     key: string | null
   ): ElementVNode | VirtualVNode | null {
     let vNodeWithKey: ElementVNode | VirtualVNode | null = null;
-    let vNode = vCurrent;
-    while (vNode) {
-      const name = vnode_isElementVNode(vNode) ? vnode_getElementName(vNode) : null;
-      const vKey = getKey(vNode) || getComponentHash(vNode, container.$getObjectById$);
-      if (vKey == key && name == nodeName) {
-        vNodeWithKey = vNode as ElementVNode | VirtualVNode;
-        break;
+    if (vSiblings === null) {
+      // it is not materialized; so materialize it.
+      vSiblings = new Map<string, VNode>();
+      vSiblingsArray = [];
+      let vNode = vCurrent;
+      while (vNode) {
+        const name = vnode_isElementVNode(vNode) ? vnode_getElementName(vNode) : null;
+        const vKey = getKey(vNode) || getComponentHash(vNode, container.$getObjectById$);
+        if (vNodeWithKey === null && vKey == key && name == nodeName) {
+          vNodeWithKey = vNode as ElementVNode | VirtualVNode;
+        } else {
+          if (vKey === null) {
+            vSiblingsArray.push(name, vNode);
+          } else {
+            // we only add the elements which we did not find yet.
+            vSiblings.set(name + ':' + vKey, vNode);
+          }
+        }
+        vNode = vnode_getNextSibling(vNode);
+      }
+    } else {
+      if (key === null) {
+        for (let i = 0; i < vSiblingsArray!.length; i += 2) {
+          if (vSiblingsArray![i] === nodeName) {
+            vNodeWithKey = vSiblingsArray![i + 1] as ElementVNode | VirtualVNode;
+            vSiblingsArray!.splice(i, 2);
+            break;
+          }
+        }
+      } else {
+        const vSibling = vSiblings.get(nodeName + ':' + key);
+        if (vSibling) {
+          vNodeWithKey = vSibling as ElementVNode | VirtualVNode;
+          vSiblings.delete(nodeName + ':' + key);
+        }
       }
-      vNode = vnode_getNextSibling(vNode);
     }
     return vNodeWithKey;
   }
diff --git a/packages/qwik/src/core/client/vnode-diff.unit.tsx b/packages/qwik/src/core/client/vnode-diff.unit.tsx
@@ -409,16 +409,18 @@ describe('vNode-diff', () => {
   describe('keys', () => {
     it('should not reuse element because old has a key and new one does not', () => {
       const { vNode, vParent, document } = vnode_fromJSX(
-        <test key="KA_6">
-          <b {...{ 'q:key': '1' }}>old</b>
-        </test>
+        _jsxSorted('test', {}, null, [_jsxSorted('b', {}, null, 'old', 0, '1')], 0, 'KA_6')
       );
-      const test = (
-        <test key="KA_6">
-          <b>new</b>
-        </test>
+      const test = _jsxSorted(
+        'test',
+        {},
+        null,
+        [_jsxSorted('b', {}, null, 'new', 0, null)],
+        0,
+        'KA_6'
       );
-      const bOriginal = document.querySelector('b[key=1]')!;
+      const bOriginal = document.querySelector('b[q\\:key=1]')!;
+      expect(bOriginal).toBeDefined();
       vnode_diff(
         { $journal$: journal, $scheduler$: scheduler, document } as any,
         test,
@@ -428,32 +430,46 @@ describe('vNode-diff', () => {
       vnode_applyJournal(journal);
       expect(vNode).toMatchVDOM(test);
       const bSecond = document.querySelector('b')!;
+      expect(bSecond).toBeDefined();
       expect(bSecond).not.toBe(bOriginal);
     });
     it('should reuse elements if keys match', () => {
       const { vNode, vParent, document } = vnode_fromJSX(
-        <test key="KA_6">
-          <b {...{ 'q:key': '1' }}>1</b>
-          <b {...{ 'q:key': '2' }}>2</b>
-        </test>
-      );
-      const test = (
-        <test key="KA_6">
-          <b>before</b>
-          <b key="2">2</b>
-          <b key="3">3</b>
-          <b>in</b>
-          <b key="1">1</b>
-          <b>after</b>
-        </test>
+        _jsxSorted(
+          'test',
+          {},
+          null,
+          [_jsxSorted('b', {}, null, '1', 0, '1'), _jsxSorted('b', {}, null, '2', 0, '2')],
+          0,
+          'KA_6'
+        )
       );
-      const b1 = document.querySelector('b[key=1]')!;
-      const b2 = document.querySelector('b[key=1]')!;
+      const test = _jsxSorted(
+        'test',
+        {},
+        null,
+        [
+          _jsxSorted('b', {}, null, 'before', 0, null),
+          _jsxSorted('b', {}, null, '2', 0, '2'),
+          _jsxSorted('b', {}, null, '3', 0, '3'),
+          _jsxSorted('b', {}, null, 'in', 0, null),
+          _jsxSorted('b', {}, null, '1', 0, '1'),
+          _jsxSorted('b', {}, null, 'after', 0, null),
+        ],
+        0,
+        'KA_6'
+      );
+      const selectB1 = () => document.querySelector('b[q\\:key=1]')!;
+      const selectB2 = () => document.querySelector('b[q\\:key=2]')!;
+      const b1 = selectB1();
+      const b2 = selectB2();
+      expect(b1).toBeDefined();
+      expect(b2).toBeDefined();
       vnode_diff({ $journal$: journal, document } as any, test, vParent, null);
       vnode_applyJournal(journal);
       expect(vNode).toMatchVDOM(test);
-      expect(b1).toBe(document.querySelector('b[key=1]')!);
-      expect(b2).toBe(document.querySelector('b[key=2]')!);
+      expect(b1).toBe(selectB1());
+      expect(b2).toBe(selectB2());
     });
   });
   describe.todo('fragments', () => {});
diff --git a/packages/qwik/src/testing/vdom-diff.unit-util.ts b/packages/qwik/src/testing/vdom-diff.unit-util.ts
@@ -440,7 +440,6 @@ export function vnode_fromJSX(jsx: JSXOutput) {
         const child = vnode_newUnMaterializedElement(doc.createElement(type));
         vnode_insertBefore(journal, vParent, child, null);
 
-        // TODO(hack): jsx.props is an empty object
         const props = jsx.varProps;
         for (const key in props) {
           if (Object.prototype.hasOwnProperty.call(props, key)) {
