Merge pull request #7401 from QwikDev/v2-remove-v-siblings

chore: refactor vSiblings

Author: wmertens

packages/qwik/src/core/client/vnode-diff.ts

@@ -136,8 +136,8 @@ export const vnode_diff = (
   /// 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: Array<string | null | VNode> | null = null; // See: `SiblingsArray`
-  let vSiblingsIdx = -1;
+  let vSiblings: Map<string, VNode> | null = null;
+  let vSiblingsArray: Array<string | VNode | null> | null = null;
 
   /// Current set of JSX children.
   let jsxChildren: JSXChildren[] = null!;
@@ -278,16 +278,9 @@ export const vnode_diff = (
    * order and can't rely on `vnode_getNextSibling` and instead we need to go by `vSiblings`.
    */
   function peekNextSibling() {
-    if (vSiblings !== null) {
-      // We came across a key, and we moved nodes around. This means we can no longer use
-      // `vnode_getNextSibling` to look at next node and instead we have to go by `vSiblings`.
-      const idx = vSiblingsIdx + SiblingsArray.NextVNode;
-      return idx < vSiblings.length ? (vSiblings[idx] as any) : null;
-    } else {
-      // If we don't have a `vNewNode`, than that means we just reconciled the current node.
-      // So advance it.
-      return vCurrent ? vnode_getNextSibling(vCurrent) : null;
-    }
+    // If we don't have a `vNewNode`, than that means we just reconciled the current node.
+    // So advance it.
+    return vCurrent ? vnode_getNextSibling(vCurrent) : null;
   }
 
   /**
@@ -299,9 +292,6 @@ export const vnode_diff = (
    */
   function advanceToNextSibling() {
     vCurrent = peekNextSibling();
-    if (vSiblings !== null) {
-      vSiblingsIdx += SiblingsArray.Size; // advance;
-    }
   }
 
   /**
@@ -332,7 +322,7 @@ export const vnode_diff = (
     if (descendVNode) {
       assertDefined(vCurrent || vNewNode, 'Expecting vCurrent to be defined.');
       vSiblings = null;
-      vSiblingsIdx = -1;
+      vSiblingsArray = null;
       vParent = vNewNode || vCurrent!;
       vCurrent = vnode_getFirstChild(vParent);
       vNewNode = null;
@@ -343,8 +333,8 @@ export const vnode_diff = (
   function ascend() {
     const descendVNode = stack.pop(); // boolean: descendVNode
     if (descendVNode) {
-      vSiblingsIdx = stack.pop();
       vSiblings = stack.pop();
+      vSiblingsArray = stack.pop();
       vNewNode = stack.pop();
       vCurrent = stack.pop();
       vParent = stack.pop();
@@ -359,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, vSiblings, vSiblingsIdx);
+      stack.push(vParent, vCurrent, vNewNode, vSiblingsArray, vSiblings);
     }
     stack.push(descendVNode);
     if (Array.isArray(children)) {
@@ -384,9 +374,6 @@ export const vnode_diff = (
   function getInsertBefore() {
     if (vNewNode) {
       return vCurrent;
-    } else if (vSiblings !== null) {
-      const nextIdx = vSiblingsIdx + SiblingsArray.NextVNode;
-      return nextIdx < vSiblings.length ? (vSiblings[nextIdx] as VNode) : null;
     } else {
       return peekNextSibling();
     }
@@ -751,10 +738,6 @@ export const vnode_diff = (
         vCurrent = vNewNode;
         // We need to clean up the vNewNode, because we don't want to skip advance to next sibling (see `advance` function).
         vNewNode = null;
-        // We need also to go back to the previous sibling, because we assigned previous sibling to the vCurrent.
-        if (vSiblings !== null) {
-          vSiblingsIdx -= SiblingsArray.Size;
-        }
       }
     }
     // reconcile attributes
@@ -959,60 +942,56 @@ export const vnode_diff = (
   }
 
   /**
-   * Retrieve the child with the given key.
-   *
-   * By retrieving the child with the given key we are effectively removing it from the list of
-   * future elements. This means that we can't just use `vnode_getNextSibling` to find the next
-   * instead we have to keep track of the elements we have already seen.
-   *
-   * We call this materializing the elements.
-   *
-   * `vSiblingsIdx`:
-   *
-   * - -1: Not materialized
-   * - Positive number - the index of the next element in the `vSiblings` array.
+   * This function is used to retrieve the child with the given key. If the child is not found, it
+   * will return null.
    *
-   * By retrieving the child with the given key we are effectively removing it from the list (hence
-   * we need to splice the `vSiblings` array).
+   * 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
-   * @param key
-   * @returns Array where: (see: `SiblingsArray`)
-   *
-   *   - Idx%3 == 0 nodeName
-   *   - Idx%3 == 1 key
-   *   - Idx%3 == 2 vNode
+   * @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;
-    if (vSiblingsIdx === -1) {
+    if (vSiblings === null) {
       // it is not materialized; so materialize it.
-      vSiblings = [];
-      vSiblingsIdx = 0;
+      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 {
-          // we only add the elements which we did not find yet.
-          vSiblings.push(name, vKey, vNode);
+          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 {
-      for (let idx = vSiblingsIdx; idx < vSiblings!.length; idx += SiblingsArray.Size) {
-        const name = vSiblings![idx + SiblingsArray.Name];
-        const vKey = vSiblings![idx + SiblingsArray.Key];
-        if (vKey === key && name === nodeName) {
-          vNodeWithKey = vSiblings![idx + SiblingsArray.VNode] as any;
-          // remove the node from the siblings array
-          vSiblings?.splice(idx, SiblingsArray.Size);
-          break;
+      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);
         }
       }
     }
@@ -1451,10 +1430,3 @@ function markVNodeAsDeleted(vCursor: VNode) {
  */
 export const HANDLER_PREFIX = ':';
 let count = 0;
-const enum SiblingsArray {
-  Name = 0,
-  Key = 1,
-  VNode = 2,
-  Size = 3,
-  NextVNode = Size + VNode,
-}

packages/qwik/src/core/client/vnode-diff.unit.tsx

@@ -412,16 +412,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,
@@ -431,32 +433,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', () => {});

packages/qwik/src/core/tests/component.spec.tsx

@@ -823,6 +823,52 @@ describe.each([
     );
   });
 
+  it('should preserve the same elements', async () => {
+    const Cmp = component$(() => {
+      const keys = useSignal(['A', 'B', 'C']);
+
+      return (
+        <div onClick$={() => (keys.value = ['B', 'C', 'A'])}>
+          {keys.value.map((key) => (
+            <span key={key} id={key}>
+              {key}
+            </span>
+          ))}
+        </div>
+      );
+    });
+
+    const { vNode, document } = await render(<Cmp />, { debug });
+    expect(vNode).toMatchVDOM(
+      <Component>
+        <div>
+          <span id="A">A</span>
+          <span id="B">B</span>
+          <span id="C">C</span>
+        </div>
+      </Component>
+    );
+    const a1 = document.getElementById('A');
+    const b1 = document.getElementById('B');
+    const c1 = document.getElementById('C');
+    await trigger(document.body, 'div', 'click');
+    expect(vNode).toMatchVDOM(
+      <Component>
+        <div>
+          <span id="B">B</span>
+          <span id="C">C</span>
+          <span id="A">A</span>
+        </div>
+      </Component>
+    );
+    const a2 = document.getElementById('A');
+    const b2 = document.getElementById('B');
+    const c2 = document.getElementById('C');
+    expect(a1).toBe(a2);
+    expect(b1).toBe(b2);
+    expect(c1).toBe(c2);
+  });
+
   it('should render correctly component only with text node and node sibling', async () => {
     const Child = component$(() => {
       return <>0</>;

starters/apps/e2e/src/components/attributes/attributes.tsx

@@ -18,6 +18,7 @@ export const Attributes = component$(() => {
       >
         Rerender
       </button>
+      <span id="render-count">{render.value}</span>
       <AttributesChild v={render.value} key={render.value} />
       <ProgressParent />
     </>

starters/apps/e2e/src/components/context/context.tsx

@@ -28,6 +28,7 @@ export const ContextRoot = component$(() => {
       <button id="btn-rerender" onClick$={() => count.value++}>
         Client Rerender
       </button>
+      <span id="render-count">{count.value}</span>
       <ContextApp key={count.value} />
     </div>
   );

starters/e2e/e2e.attributes.e2e.ts

@@ -325,11 +325,8 @@ test.describe("attributes", () => {
   test.describe("client rerender", () => {
     test.beforeEach(async ({ page }) => {
       const toggleRender = page.locator("#force-rerender");
-      const v = Number(await toggleRender.getAttribute("data-v"));
       await toggleRender.click();
-      await expect(page.locator("#renderCount")).toHaveText(`Render ${v}`);
-      // Without this the tests still fail?
-      await page.waitForTimeout(50);
+      await expect(page.locator("#render-count")).toHaveText("1");
     });
     tests();
   });

starters/e2e/e2e.context.e2e.ts

@@ -189,7 +189,7 @@ test.describe("context", () => {
     test.beforeEach(async ({ page }) => {
       const rerender = page.locator("#btn-rerender");
       await rerender.click();
-      await page.waitForTimeout(100);
+      await expect(page.locator("#render-count")).toHaveText("1");
     });
     tests(false);
   });